Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1281843

Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

Show all headers | View raw

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?

>>>          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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC PATCH 0/9] drm/rockchip: covert to support atomic API Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:30 +0100
  [RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_* Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:30 +0100
    Re: [RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_* Daniel Stone <daniel@fooishbar.org> - 2015-12-01 09:00 +0100
      Re: [RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_* Mark yao <mark.yao@rock-chips.com> - 2015-12-01 09:40 +0100
        Re: [RFC PATCH 2/9] drm/rockchip: Use new vblank api  drm_crtc_vblank_* Daniel Vetter <daniel@ffwll.ch> - 2015-12-01 10:10 +0100
          Re: [RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_* Mark yao <mark.yao@rock-chips.com> - 2015-12-01 10:50 +0100
  [RFC PATCH 4/9] drm/rockchip: support atomic asynchronous commit Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:30 +0100
  [RFC PATCH 1/9] drm/rockchip: vop: replace dpms with enable/disable Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:30 +0100
  [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:30 +0100
    Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Daniel Stone <daniel@fooishbar.org> - 2015-12-01 09:20 +0100
      Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Mark yao <mark.yao@rock-chips.com> - 2015-12-01 10:30 +0100
      Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Mark yao <mark.yao@rock-chips.com> - 2015-12-01 10:40 +0100
        Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Daniel Stone <daniel@fooishbar.org> - 2015-12-02 15:20 +0100
          Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Daniel Stone <daniel@fooishbar.org> - 2015-12-02 15:30 +0100
          Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API Mark yao <mark.yao@rock-chips.com> - 2015-12-11 07:30 +0100
  [RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:40 +0100
    Re: [RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API Daniel Vetter <daniel@ffwll.ch> - 2015-12-01 08:30 +0100
      Re: [RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic  API Mark yao <mark.yao@rock-chips.com> - 2015-12-01 09:10 +0100
    [PATCH] drm: bridge/dw_hdmi: add atomic API support Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 09:20 +0100
  [RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:40 +0100
    Re: [RFC PATCH 7/9] drm/rockchip: force enable vop when do mode  setting Thierry Reding <thierry.reding@gmail.com> - 2015-12-02 18:00 +0100
      Re: [RFC PATCH 7/9] drm/rockchip: force enable vop when do mode  setting Daniel Vetter <daniel@ffwll.ch> - 2015-12-02 23:20 +0100
        Re: [RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting Mark yao <mark.yao@rock-chips.com> - 2015-12-03 03:00 +0100
  [RFC PATCH 6/9] drm/rockchip: direct config connecter gate and out_mode Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:40 +0100
  [RFC PATCH 9/9] drm/rockchip: dw_hdmi: use encoder enable function Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:40 +0100
  [RFC PATCH 5/9] drm/rockchip: Optimization vop mode set Mark Yao <mark.yao@rock-chips.com> - 2015-12-01 04:40 +0100

csiph-web