From: mark.yao@rock-chips•com (Mark yao)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API
Date: Fri, 11 Dec 2015 14:26:31 +0800 [thread overview]
Message-ID: <566A6C97.4020207@rock-chips.com> (raw)
In-Reply-To: <CAPj87rO9_sXZHidTpNs_ubFaUX8ZtytK=bLpQSQwSy5G2-JGEA@mail.gmail.com>
On 2015?12?02? 22:18, Daniel Stone wrote:
> Hi Mark,
> Thanks for getting back to this.
>
> On 1 December 2015 at 09:31, Mark yao <mark.yao@rock-chips•com> wrote:
>> On 2015?12?01? 16:18, Daniel Stone wrote:
>>> On 1 December 2015 at 03:26, Mark Yao<mark.yao@rock-chips•com> wrote:
>>>>> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>>> + if (!crtc->state->active)
>>>>> + continue;
>>>>> +
>>>>> + rockchip_crtc_wait_for_update(crtc);
>>>>> + }
>>> I'd be much more comfortable if this passed in an explicit pointer to
>>> state, or an address to wait for, rather than have wait_for_complete
>>> dig out state with no locking. The latter is potentially racy for
>>> async operations.
>>>
>> Hi Daniel
>> "if this passed in an explicit pointer to state, or an address to wait
>> for", I don't understand, can you point how it work?
> Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc
> pointer, and establishes the state from that (e.g.
> crtc->primary->state). This can easily lead to confusion in async
> contexts, as the states attached to a drm_crtc and a drm_plane can
> change here while you wait for it.
>
> It would be better if the call was:
>
> for_each_plane_in_state(state, plane, plane_state, i) {
> if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> rockchip_crtc_wait_for_update(plane_state->crtc, plane_state);
> }
>
> This way it is very clear, and there is no confusion as to which
> request we are waiting to complete.
>
> In general, using crtc->state or plane->state is a very bad idea,
> _except_ in the atomic_check function where you are calculating
> changes (e.g. if (plane_state->fb->pitches[0] !=
> plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed =
> true). After atomic_check, you should always pass around pointers to
> the plane state explicitly, and avoid using the pointers from drm_crtc
> and drm_plane.
>
> Does that help?
Hi Daniel
Sorry, I don't actually understand why crtc->state or plane->state is no
recommended
except atomic_check, on atomic_update, we need use plane->state, is that
a problem?
I guess that, drm_atomic_helper_swap_state would race with async operations,
so use crtc->state on async stack is not safe. is it correct?
I think we can make asynchronous commit serialize as tegra drm done to
avoid this problem:
86 /* serialize outstanding asynchronous commits */
87 mutex_lock(&tegra->commit.lock);
88 flush_work(&tegra->commit.work);
89
90 /*
91 * This is the point of no return - everything below never
fails except
92 * when the hw goes bonghits. Which means we can commit
the new state on
93 * the software side now.
94 */
95
96 drm_atomic_helper_swap_state(drm, state);
97
98 if (async)
99 tegra_atomic_schedule(tegra, state);
100 else
101 tegra_atomic_complete(tegra, state);
102
103 mutex_unlock(&tegra->commit.lock);
>
>>>> if (is_yuv) {
>>>> /*
>>>> * Src.x1 can be odd when do clip, but yuv plane start
>>>> point
>>>> * need align with 2 pixel.
>>>> */
>>>> - val = (src.x1 >> 16) % 2;
>>>> - src.x1 += val << 16;
>>>> - src.x2 += val << 16;
>>>> + uint32_t temp = (src->x1 >> 16) % 2;
>>>> +
>>>> + src->x1 += temp << 16;
>>>> + src->x2 += temp << 16;
>>>> }
>>> I know this isn't new, but moving the plane around is bad. If the user
>>> gives you a pixel boundary that you can't actually use, please reject
>>> the configuration rather than silently trying to fix it up.
>> the origin src.x1 would align with 2 pixel, but when we move the dest
>> window, and do clip by output, the src.x1 may be clipped to odd.
>> regect this configuration may let user confuse, sometimes good, sometimes
>> bad.
> For me, it is more confusing when the display shows something
> different to what I have requested. In some media usecases, doing this
> is a showstopper and will result in products failing acceptance
> testing. Userspace can make a policy decision to try different
> alignments to get _something_ to show (even if it's not what was
> explicitly requested), but doing this in the kernel is inappropriate:
> please just reject it, and have userspace fall back to another
> composition method (e.g. GL) in these cases.
>
>>>> -static void vop_plane_destroy(struct drm_plane *plane)
>>>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
>>>> + struct drm_plane_state *state)
>>>> {
>>>> - vop_disable_plane(plane);
>>>> - drm_plane_cleanup(plane);
>>>> + struct vop_plane_state *vop_state = to_vop_plane_state(state);
>>>> +
>>>> + if (state->fb)
>>>> + drm_framebuffer_unreference(state->fb);
>>>> +
>>>> + kfree(vop_state);
>>>> }
>>> You can replace this hook with drm_atomic_helper_plane_destroy_state.
>>
>> Hmm, only can hook with __drm_atomic_helper_plane_destroy_state.
> Ah yes, you're right. But still, using that would be better than duplicating it.
>
>> Can you share your Weston environment to me, I'm interesting to test drm
>> rockchip on weston.
> Of course. You can download Weston from http://wayland.freedesktop.org
> - the most interesting dependencies are libevdev, libinput, and
> wayland itself. If you are building newer Weston from git, you'll need
> the wayland-protocols repository as well, from
> anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me
> know privately if you need some more help with building these, but
> they should be quite straightforward.
>
> Cheers,
> Daniel
>
>
>
--
?ark Yao
next prev parent reply other threads:[~2015-12-11 6:26 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-01 3:26 [RFC PATCH 0/9] drm/rockchip: covert to support atomic API Mark Yao
2015-12-01 3:26 ` [RFC PATCH 1/9] drm/rockchip: vop: replace dpms with enable/disable Mark Yao
2015-12-01 3:26 ` [RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_* Mark Yao
2015-12-01 7:56 ` Daniel Stone
2015-12-01 8:33 ` Mark yao
2015-12-01 9:01 ` Daniel Vetter
2015-12-01 9:43 ` Mark yao
2015-12-01 3:26 ` [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Mark Yao
2015-12-01 8:18 ` Daniel Stone
2015-12-01 9:21 ` Mark yao
2015-12-01 9:31 ` Mark yao
2015-12-02 14:18 ` Daniel Stone
2015-12-02 14:22 ` Daniel Stone
2015-12-11 6:26 ` Mark yao [this message]
2015-12-01 3:26 ` [RFC PATCH 4/9] drm/rockchip: support atomic asynchronous commit Mark Yao
2015-12-01 3:28 ` [RFC PATCH 5/9] drm/rockchip: Optimization vop mode set Mark Yao
2015-12-01 3:30 ` [RFC PATCH 6/9] drm/rockchip: direct config connecter gate and out_mode Mark Yao
2015-12-01 3:32 ` [RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting Mark Yao
2015-12-02 16:55 ` Thierry Reding
2015-12-02 22:17 ` Daniel Vetter
2015-12-03 1:54 ` Mark yao
2015-12-01 3:35 ` [RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API Mark Yao
2015-12-01 7:21 ` Daniel Vetter
2015-12-01 8:07 ` Mark yao
2015-12-01 8:17 ` [PATCH] drm: bridge/dw_hdmi: add atomic API support Mark Yao
2015-12-01 3:37 ` [RFC PATCH 9/9] drm/rockchip: dw_hdmi: use encoder enable function Mark Yao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=566A6C97.4020207@rock-chips.com \
--to=mark.yao@rock-chips$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox