[RFC,3/9] drm/rockchip: Convert to support atomic API
diff mbox

Message ID 1448940391-23333-4-git-send-email-mark.yao@rock-chips.com
State New
Headers show

Commit Message

yao mark Dec. 1, 2015, 3:26 a.m. UTC
Rockchip vop not support hw vblank counter, needed check the committed
register if it's really take effect.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |    5 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |    2 +
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   65 +++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  603 ++++++++++-----------------
 4 files changed, 301 insertions(+), 374 deletions(-)

Comments

Daniel Stone Dec. 1, 2015, 8:18 a.m. UTC | #1
Hi Mark,

On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
> +static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
> +{
> +       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc *crtc;
> +       int i;
> +
> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +               if (!crtc->state->active)
> +                       continue;
> +
> +               WARN_ON(drm_crtc_vblank_get(crtc));
> +       }
> +
> +       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.

> +struct vop_plane_state {
> +       struct drm_plane_state base;
> +       dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];

Can you get rid of this by just using the pointer to a
rockchip_gem_object already provided?

> -static int vop_update_plane_event(struct drm_plane *plane,
> -                                 struct drm_crtc *crtc,
> -                                 struct drm_framebuffer *fb, int crtc_x,
> -                                 int crtc_y, unsigned int crtc_w,
> -                                 unsigned int crtc_h, uint32_t src_x,
> -                                 uint32_t src_y, uint32_t src_w,
> -                                 uint32_t src_h,
> -                                 struct drm_pending_vblank_event *event)
> +static int vop_plane_atomic_check(struct drm_plane *plane,
> +                          struct drm_plane_state *state)
>  {
> +       struct drm_crtc *crtc = state->crtc;
> +       struct drm_framebuffer *fb = state->fb;
>         struct vop_win *vop_win = to_vop_win(plane);
> +       struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
>         const struct vop_win_data *win = vop_win->data;
> -       struct vop *vop = to_vop(crtc);
>         struct drm_gem_object *obj;
>         struct rockchip_gem_object *rk_obj;
> -       struct drm_gem_object *uv_obj;
> -       struct rockchip_gem_object *rk_uv_obj;
>         unsigned long offset;
> -       unsigned int actual_w;
> -       unsigned int actual_h;
> -       unsigned int dsp_stx;
> -       unsigned int dsp_sty;
> -       unsigned int y_vir_stride;
> -       unsigned int uv_vir_stride = 0;
> -       dma_addr_t yrgb_mst;
> -       dma_addr_t uv_mst = 0;
> -       enum vop_data_format format;
> -       uint32_t val;
> -       bool is_alpha;
> -       bool rb_swap;
>         bool is_yuv;
>         bool visible;
>         int ret;
> -       struct drm_rect dest = {
> -               .x1 = crtc_x,
> -               .y1 = crtc_y,
> -               .x2 = crtc_x + crtc_w,
> -               .y2 = crtc_y + crtc_h,
> -       };
> -       struct drm_rect src = {
> -               /* 16.16 fixed point */
> -               .x1 = src_x,
> -               .y1 = src_y,
> -               .x2 = src_x + src_w,
> -               .y2 = src_y + src_h,
> -       };
> -       const struct drm_rect clip = {
> -               .x2 = crtc->mode.hdisplay,
> -               .y2 = crtc->mode.vdisplay,
> -       };
> -       bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
> +       struct drm_rect *dest = &vop_plane_state->dest;
> +       struct drm_rect *src = &vop_plane_state->src;
> +       struct drm_rect clip;
>         int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
>         int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
>
> -       ret = drm_plane_helper_check_update(plane, crtc, fb,
> -                                           &src, &dest, &clip,
> +       crtc = crtc ? crtc : plane->state->crtc;
> +       /*
> +        * Both crtc or plane->state->crtc can be null.
> +        */
> +       if (!crtc || !fb)
> +               goto out_disable;
> +       src->x1 = state->src_x;
> +       src->y1 = state->src_y;
> +       src->x2 = state->src_x + state->src_w;
> +       src->y2 = state->src_y + state->src_h;
> +       dest->x1 = state->crtc_x;
> +       dest->y1 = state->crtc_y;
> +       dest->x2 = state->crtc_x + state->crtc_w;
> +       dest->y2 = state->crtc_y + state->crtc_h;
> +
> +       clip.x1 = 0;
> +       clip.y1 = 0;
> +       clip.x2 = crtc->mode.hdisplay;
> +       clip.y2 = crtc->mode.vdisplay;
> +
> +       ret = drm_plane_helper_check_update(plane, crtc, state->fb,
> +                                           src, dest, &clip,
>                                             min_scale,
>                                             max_scale,
> -                                           can_position, false, &visible);
> +                                           true, true, &visible);
>         if (ret)
>                 return ret;
>
>         if (!visible)
> -               return 0;
> -
> -       is_alpha = is_alpha_support(fb->pixel_format);
> -       rb_swap = has_rb_swapped(fb->pixel_format);
> -       is_yuv = is_yuv_support(fb->pixel_format);
> +               goto out_disable;
>
> -       format = vop_convert_format(fb->pixel_format);
> -       if (format < 0)
> -               return format;
> +       vop_plane_state->format = vop_convert_format(fb->pixel_format);
> +       if (vop_plane_state->format < 0)
> +               return vop_plane_state->format;
>
> -       obj = rockchip_fb_get_gem_obj(fb, 0);
> -       if (!obj) {
> -               DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
> -               return -EINVAL;
> -       }
> -
> -       rk_obj = to_rockchip_obj(obj);
> +       is_yuv = is_yuv_support(fb->pixel_format);
>
>         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.

> -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.

> -static void vop_win_state_complete(struct vop_win *vop_win,
> -                                  struct vop_win_state *state)
> -{
> -       struct vop *vop = vop_win->vop;
> -       struct drm_crtc *crtc = &vop->crtc;
> -       struct drm_device *drm = crtc->dev;
> -       unsigned long flags;
> -
> -       if (state->event) {
> -               spin_lock_irqsave(&drm->event_lock, flags);
> -               drm_crtc_send_vblank_event(crtc, state->event);
> -               spin_unlock_irqrestore(&drm->event_lock, flags);
> -       }

Ah, I see this is based on top of the patches I sent to fix pageflips?
If they have been merged, I would like to send you some others to
merge as well, which fix an OOPS when you close Weston. I didn't send
them yet as I didn't get a response to the previous patchset, so did
not know you had merged it. There are a lot of other correctness fixes
I had in there (e.g. using the CRTC vblank functions, some locking
fixes), but if this code is going to be thrown away then I will just
discard most of them as well.

Still, I would like to prepare another series for you to merge through
drm-fixes, to be able to run Weston (the same-fb-flip patch I sent
along with the drm_crtc_send_vblank_event conversion, also adding a
preclose hook to discard events when clients exit).

Cheers,
Daniel
yao mark Dec. 1, 2015, 9:21 a.m. UTC | #2
On 2015?12?01? 16:18, Daniel Stone wrote:
> Hi Mark,
>
> On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
>> +static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
>> +{
>> +       struct drm_crtc_state *crtc_state;
>> +       struct drm_crtc *crtc;
>> +       int i;
>> +
>> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +               if (!crtc->state->active)
>> +                       continue;
>> +
>> +               WARN_ON(drm_crtc_vblank_get(crtc));
>> +       }
>> +
>> +       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.
>
>> +struct vop_plane_state {
>> +       struct drm_plane_state base;
>> +       dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];
> Can you get rid of this by just using the pointer to a
> rockchip_gem_object already provided?

Good idea, I will do that.

>> -static int vop_update_plane_event(struct drm_plane *plane,
>> -                                 struct drm_crtc *crtc,
>> -                                 struct drm_framebuffer *fb, int crtc_x,
>> -                                 int crtc_y, unsigned int crtc_w,
>> -                                 unsigned int crtc_h, uint32_t src_x,
>> -                                 uint32_t src_y, uint32_t src_w,
>> -                                 uint32_t src_h,
>> -                                 struct drm_pending_vblank_event *event)
>> +static int vop_plane_atomic_check(struct drm_plane *plane,
>> +                          struct drm_plane_state *state)
>>   {
>> +       struct drm_crtc *crtc = state->crtc;
>> +       struct drm_framebuffer *fb = state->fb;
>>          struct vop_win *vop_win = to_vop_win(plane);
>> +       struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
>>          const struct vop_win_data *win = vop_win->data;
>> -       struct vop *vop = to_vop(crtc);
>>          struct drm_gem_object *obj;
>>          struct rockchip_gem_object *rk_obj;
>> -       struct drm_gem_object *uv_obj;
>> -       struct rockchip_gem_object *rk_uv_obj;
>>          unsigned long offset;
>> -       unsigned int actual_w;
>> -       unsigned int actual_h;
>> -       unsigned int dsp_stx;
>> -       unsigned int dsp_sty;
>> -       unsigned int y_vir_stride;
>> -       unsigned int uv_vir_stride = 0;
>> -       dma_addr_t yrgb_mst;
>> -       dma_addr_t uv_mst = 0;
>> -       enum vop_data_format format;
>> -       uint32_t val;
>> -       bool is_alpha;
>> -       bool rb_swap;
>>          bool is_yuv;
>>          bool visible;
>>          int ret;
>> -       struct drm_rect dest = {
>> -               .x1 = crtc_x,
>> -               .y1 = crtc_y,
>> -               .x2 = crtc_x + crtc_w,
>> -               .y2 = crtc_y + crtc_h,
>> -       };
>> -       struct drm_rect src = {
>> -               /* 16.16 fixed point */
>> -               .x1 = src_x,
>> -               .y1 = src_y,
>> -               .x2 = src_x + src_w,
>> -               .y2 = src_y + src_h,
>> -       };
>> -       const struct drm_rect clip = {
>> -               .x2 = crtc->mode.hdisplay,
>> -               .y2 = crtc->mode.vdisplay,
>> -       };
>> -       bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
>> +       struct drm_rect *dest = &vop_plane_state->dest;
>> +       struct drm_rect *src = &vop_plane_state->src;
>> +       struct drm_rect clip;
>>          int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>>                                          DRM_PLANE_HELPER_NO_SCALING;
>>          int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
>>                                          DRM_PLANE_HELPER_NO_SCALING;
>>
>> -       ret = drm_plane_helper_check_update(plane, crtc, fb,
>> -                                           &src, &dest, &clip,
>> +       crtc = crtc ? crtc : plane->state->crtc;
>> +       /*
>> +        * Both crtc or plane->state->crtc can be null.
>> +        */
>> +       if (!crtc || !fb)
>> +               goto out_disable;
>> +       src->x1 = state->src_x;
>> +       src->y1 = state->src_y;
>> +       src->x2 = state->src_x + state->src_w;
>> +       src->y2 = state->src_y + state->src_h;
>> +       dest->x1 = state->crtc_x;
>> +       dest->y1 = state->crtc_y;
>> +       dest->x2 = state->crtc_x + state->crtc_w;
>> +       dest->y2 = state->crtc_y + state->crtc_h;
>> +
>> +       clip.x1 = 0;
>> +       clip.y1 = 0;
>> +       clip.x2 = crtc->mode.hdisplay;
>> +       clip.y2 = crtc->mode.vdisplay;
>> +
>> +       ret = drm_plane_helper_check_update(plane, crtc, state->fb,
>> +                                           src, dest, &clip,
>>                                              min_scale,
>>                                              max_scale,
>> -                                           can_position, false, &visible);
>> +                                           true, true, &visible);
>>          if (ret)
>>                  return ret;
>>
>>          if (!visible)
>> -               return 0;
>> -
>> -       is_alpha = is_alpha_support(fb->pixel_format);
>> -       rb_swap = has_rb_swapped(fb->pixel_format);
>> -       is_yuv = is_yuv_support(fb->pixel_format);
>> +               goto out_disable;
>>
>> -       format = vop_convert_format(fb->pixel_format);
>> -       if (format < 0)
>> -               return format;
>> +       vop_plane_state->format = vop_convert_format(fb->pixel_format);
>> +       if (vop_plane_state->format < 0)
>> +               return vop_plane_state->format;
>>
>> -       obj = rockchip_fb_get_gem_obj(fb, 0);
>> -       if (!obj) {
>> -               DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       rk_obj = to_rockchip_obj(obj);
>> +       is_yuv = is_yuv_support(fb->pixel_format);
>>
>>          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.

>> -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.

>> -static void vop_win_state_complete(struct vop_win *vop_win,
>> -                                  struct vop_win_state *state)
>> -{
>> -       struct vop *vop = vop_win->vop;
>> -       struct drm_crtc *crtc = &vop->crtc;
>> -       struct drm_device *drm = crtc->dev;
>> -       unsigned long flags;
>> -
>> -       if (state->event) {
>> -               spin_lock_irqsave(&drm->event_lock, flags);
>> -               drm_crtc_send_vblank_event(crtc, state->event);
>> -               spin_unlock_irqrestore(&drm->event_lock, flags);
>> -       }
> Ah, I see this is based on top of the patches I sent to fix pageflips?
> If they have been merged, I would like to send you some others to
> merge as well, which fix an OOPS when you close Weston. I didn't send
> them yet as I didn't get a response to the previous patchset, so did
> not know you had merged it. There are a lot of other correctness fixes
> I had in there (e.g. using the CRTC vblank functions, some locking
> fixes), but if this code is going to be thrown away then I will just
> discard most of them as well.
>
> Still, I would like to prepare another series for you to merge through
> drm-fixes, to be able to run Weston (the same-fb-flip patch I sent
> along with the drm_crtc_send_vblank_event conversion, also adding a
> preclose hook to discard events when clients exit).
>
> Cheers,
> Daniel
>
>
>
Hi Daniel
     Can you share your Weston environment to me, I'm interesting to 
test drm rockchip on weston.
yao mark Dec. 1, 2015, 9:31 a.m. UTC | #3
On 2015?12?01? 16:18, Daniel Stone wrote:
> Hi Mark,
>
> On 1 December 2015 at 03:26, Mark Yao<mark.yao@rock-chips.com>  wrote:
>> >+static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
>> >+{
>> >+       struct drm_crtc_state *crtc_state;
>> >+       struct drm_crtc *crtc;
>> >+       int i;
>> >+
>> >+       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> >+               if (!crtc->state->active)
>> >+                       continue;
>> >+
>> >+               WARN_ON(drm_crtc_vblank_get(crtc));
>> >+       }
>> >+
>> >+       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?
Daniel Stone Dec. 2, 2015, 2:18 p.m. UTC | #4
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
Daniel Stone Dec. 2, 2015, 2:22 p.m. UTC | #5
Hi Mark,

On 2 December 2015 at 14:18, Daniel Stone <daniel@fooishbar.org> wrote:
> On 1 December 2015 at 09:31, Mark yao <mark.yao@rock-chips.com> wrote:
>>     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.

Sorry, left one thing out. When running, if you do not have
GBM/Wayland enabled for Mali (or no Mali support), you should run
with:
weston --use-pixman
or:
weston-launch -- --use-pixman

Launching from SSH is a bit more complicated:
sudo weston-launch --user=$username --tty=/dev/tty3 -- --use-pixman

(Replace tty3 with the TTY of your choice: it must be in text mode.)

Once you have done this, you will find three issues:
  - passing -1 to drm_send_vblank event causes OOPS - now fixed in your tree
  - not sending pageflip events for same-fb flips causes Weston hang -
fixed with my patch, and I believe now fixed in atomic (though it may
still have some timing issues; I hope to get to review this early next
week)
  - not having a preclose hook causes OOPS when Weston exits in the
middle of rendering - fixed in
https://git.collabora.com/cgit/user/daniels/linux.git/commit/?h=wip/4.4.x/rockchip-drm-fixes&id=d14f21bcd7e7a1b9ca129c411a9da9c911037965
and the preceding commit, which I hope to re-send for 4.4 -fixes in
the next couple of days

Hope this helps.

Cheers,
Daniel
yao mark Dec. 11, 2015, 6:26 a.m. UTC | #6
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
>
>
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index ccd46f2..5de51fd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -214,6 +214,8 @@  static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
 	 */
 	drm_dev->vblank_disable_allowed = true;
 
+	drm_mode_config_reset(drm_dev);
+
 	ret = rockchip_drm_fbdev_init(drm_dev);
 	if (ret)
 		goto err_vblank_cleanup;
@@ -277,7 +279,8 @@  const struct vm_operations_struct rockchip_drm_vm_ops = {
 };
 
 static struct drm_driver rockchip_drm_driver = {
-	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM |
+				  DRIVER_PRIME | DRIVER_ATOMIC,
 	.load			= rockchip_drm_load,
 	.unload			= rockchip_drm_unload,
 	.lastclose		= rockchip_drm_lastclose,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 069d6d4..513ff98 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -18,6 +18,7 @@ 
 #define _ROCKCHIP_DRM_DRV_H
 
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem.h>
 
 #include <linux/module.h>
@@ -63,5 +64,6 @@  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 				    struct device *dev);
+void rockchip_crtc_wait_for_update(struct drm_crtc *crtc);
 
 #endif /* _ROCKCHIP_DRM_DRV_H_ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 002645b..c86d93a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -15,6 +15,7 @@ 
 #include <linux/kernel.h>
 #include <drm/drm.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 
@@ -166,9 +167,73 @@  static void rockchip_drm_output_poll_changed(struct drm_device *dev)
 		drm_fb_helper_hotplug_event(fb_helper);
 }
 
+static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		WARN_ON(drm_crtc_vblank_get(crtc));
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		rockchip_crtc_wait_for_update(crtc);
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+int rockchip_drm_atomic_commit(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       bool async)
+{
+	int ret;
+
+	if (async)
+		return -EBUSY;
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (ret)
+		return ret;
+
+	drm_atomic_helper_swap_state(dev, state);
+
+	/*
+	 * TODO: do fence wait here.
+	 */
+
+	drm_atomic_helper_commit_modeset_disables(dev, state);
+
+	drm_atomic_helper_commit_planes(dev, state, false);
+
+	drm_atomic_helper_commit_modeset_enables(dev, state);
+
+	rockchip_atomic_wait_for_complete(state);
+
+	drm_atomic_helper_cleanup_planes(dev, state);
+
+	drm_atomic_state_free(state);
+
+	return 0;
+}
+
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
 	.fb_create = rockchip_user_fb_create,
 	.output_poll_changed = rockchip_drm_output_poll_changed,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = rockchip_drm_atomic_commit,
 };
 
 struct drm_framebuffer *
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 3d16e70..a28e255 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -14,6 +14,7 @@ 
 
 #include <drm/drm.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -63,12 +64,16 @@ 
 
 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
-
-struct vop_win_state {
-	struct list_head head;
-	struct drm_framebuffer *fb;
-	dma_addr_t yrgb_mst;
-	struct drm_pending_vblank_event *event;
+#define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)
+
+struct vop_plane_state {
+	struct drm_plane_state base;
+	dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];
+	uint32_t vir_stride[ROCKCHIP_MAX_FB_BUFFER];
+	int format;
+	struct drm_rect src;
+	struct drm_rect dest;
+	bool enable;
 };
 
 struct vop_win {
@@ -76,8 +81,7 @@  struct vop_win {
 	const struct vop_win_data *data;
 	struct vop *vop;
 
-	struct list_head pending;
-	struct vop_win_state *active;
+	struct vop_plane_state state;
 };
 
 struct vop {
@@ -93,6 +97,7 @@  struct vop {
 	struct mutex vsync_mutex;
 	bool vsync_work_pending;
 	struct completion dsp_hold_completion;
+	struct completion wait_update_complete;
 
 	const struct vop_data *data;
 
@@ -745,148 +750,99 @@  static void vop_crtc_disable(struct drm_crtc *crtc)
 	pm_runtime_put(vop->dev);
 }
 
-/*
- * Caller must hold vsync_mutex.
- */
-static struct drm_framebuffer *vop_win_last_pending_fb(struct vop_win *vop_win)
-{
-	struct vop_win_state *last;
-	struct vop_win_state *active = vop_win->active;
-
-	if (list_empty(&vop_win->pending))
-		return active ? active->fb : NULL;
-
-	last = list_last_entry(&vop_win->pending, struct vop_win_state, head);
-	return last ? last->fb : NULL;
-}
-
-/*
- * Caller must hold vsync_mutex.
- */
-static int vop_win_queue_fb(struct vop_win *vop_win,
-			    struct drm_framebuffer *fb, dma_addr_t yrgb_mst,
-			    struct drm_pending_vblank_event *event)
+static void vop_plane_destroy(struct drm_plane *plane)
 {
-	struct vop_win_state *state;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (!state)
-		return -ENOMEM;
-
-	state->fb = fb;
-	state->yrgb_mst = yrgb_mst;
-	state->event = event;
-
-	list_add_tail(&state->head, &vop_win->pending);
-
-	return 0;
+	drm_plane_cleanup(plane);
 }
 
-static int vop_update_plane_event(struct drm_plane *plane,
-				  struct drm_crtc *crtc,
-				  struct drm_framebuffer *fb, int crtc_x,
-				  int crtc_y, unsigned int crtc_w,
-				  unsigned int crtc_h, uint32_t src_x,
-				  uint32_t src_y, uint32_t src_w,
-				  uint32_t src_h,
-				  struct drm_pending_vblank_event *event)
+static int vop_plane_atomic_check(struct drm_plane *plane,
+			   struct drm_plane_state *state)
 {
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
 	struct vop_win *vop_win = to_vop_win(plane);
+	struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
 	const struct vop_win_data *win = vop_win->data;
-	struct vop *vop = to_vop(crtc);
 	struct drm_gem_object *obj;
 	struct rockchip_gem_object *rk_obj;
-	struct drm_gem_object *uv_obj;
-	struct rockchip_gem_object *rk_uv_obj;
 	unsigned long offset;
-	unsigned int actual_w;
-	unsigned int actual_h;
-	unsigned int dsp_stx;
-	unsigned int dsp_sty;
-	unsigned int y_vir_stride;
-	unsigned int uv_vir_stride = 0;
-	dma_addr_t yrgb_mst;
-	dma_addr_t uv_mst = 0;
-	enum vop_data_format format;
-	uint32_t val;
-	bool is_alpha;
-	bool rb_swap;
 	bool is_yuv;
 	bool visible;
 	int ret;
-	struct drm_rect dest = {
-		.x1 = crtc_x,
-		.y1 = crtc_y,
-		.x2 = crtc_x + crtc_w,
-		.y2 = crtc_y + crtc_h,
-	};
-	struct drm_rect src = {
-		/* 16.16 fixed point */
-		.x1 = src_x,
-		.y1 = src_y,
-		.x2 = src_x + src_w,
-		.y2 = src_y + src_h,
-	};
-	const struct drm_rect clip = {
-		.x2 = crtc->mode.hdisplay,
-		.y2 = crtc->mode.vdisplay,
-	};
-	bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
+	struct drm_rect *dest = &vop_plane_state->dest;
+	struct drm_rect *src = &vop_plane_state->src;
+	struct drm_rect clip;
 	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
 	int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
 					DRM_PLANE_HELPER_NO_SCALING;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    &src, &dest, &clip,
+	crtc = crtc ? crtc : plane->state->crtc;
+	/*
+	 * Both crtc or plane->state->crtc can be null.
+	 */
+	if (!crtc || !fb)
+		goto out_disable;
+	src->x1 = state->src_x;
+	src->y1 = state->src_y;
+	src->x2 = state->src_x + state->src_w;
+	src->y2 = state->src_y + state->src_h;
+	dest->x1 = state->crtc_x;
+	dest->y1 = state->crtc_y;
+	dest->x2 = state->crtc_x + state->crtc_w;
+	dest->y2 = state->crtc_y + state->crtc_h;
+
+	clip.x1 = 0;
+	clip.y1 = 0;
+	clip.x2 = crtc->mode.hdisplay;
+	clip.y2 = crtc->mode.vdisplay;
+
+	ret = drm_plane_helper_check_update(plane, crtc, state->fb,
+					    src, dest, &clip,
 					    min_scale,
 					    max_scale,
-					    can_position, false, &visible);
+					    true, true, &visible);
 	if (ret)
 		return ret;
 
 	if (!visible)
-		return 0;
-
-	is_alpha = is_alpha_support(fb->pixel_format);
-	rb_swap = has_rb_swapped(fb->pixel_format);
-	is_yuv = is_yuv_support(fb->pixel_format);
+		goto out_disable;
 
-	format = vop_convert_format(fb->pixel_format);
-	if (format < 0)
-		return format;
+	vop_plane_state->format = vop_convert_format(fb->pixel_format);
+	if (vop_plane_state->format < 0)
+		return vop_plane_state->format;
 
-	obj = rockchip_fb_get_gem_obj(fb, 0);
-	if (!obj) {
-		DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
-		return -EINVAL;
-	}
-
-	rk_obj = to_rockchip_obj(obj);
+	is_yuv = is_yuv_support(fb->pixel_format);
 
 	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;
 	}
 
-	actual_w = (src.x2 - src.x1) >> 16;
-	actual_h = (src.y2 - src.y1) >> 16;
+	offset = (src->x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
+	offset += (src->y1 >> 16) * fb->pitches[0];
 
-	dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start;
-	dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
+	obj = rockchip_fb_get_gem_obj(fb, 0);
+	if (!obj) {
+		DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
+		return -EINVAL;
+	}
 
-	offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
-	offset += (src.y1 >> 16) * fb->pitches[0];
+	rk_obj = to_rockchip_obj(obj);
 
-	yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
-	y_vir_stride = fb->pitches[0] >> 2;
+	vop_plane_state->dma_addr[0] = rk_obj->dma_addr + offset +
+					fb->offsets[0];
+	vop_plane_state->vir_stride[0] = fb->pitches[0] >> 2;
 
 	if (is_yuv) {
+		struct drm_gem_object *uv_obj;
+		struct rockchip_gem_object *rk_uv_obj;
 		int hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
 		int vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
 		int bpp = drm_format_plane_cpp(fb->pixel_format, 1);
@@ -897,72 +853,89 @@  static int vop_update_plane_event(struct drm_plane *plane,
 			return -EINVAL;
 		}
 		rk_uv_obj = to_rockchip_obj(uv_obj);
-		uv_vir_stride = fb->pitches[1] >> 2;
+		vop_plane_state->vir_stride[1] = fb->pitches[1] >> 2;
 
-		offset = (src.x1 >> 16) * bpp / hsub;
-		offset += (src.y1 >> 16) * fb->pitches[1] / vsub;
+		offset = (src->x1 >> 16) * bpp / hsub;
+		offset += (src->y1 >> 16) * fb->pitches[1] / vsub;
 
-		uv_mst = rk_uv_obj->dma_addr + offset + fb->offsets[1];
+		vop_plane_state->dma_addr[1] = rk_uv_obj->dma_addr + offset +
+						fb->offsets[1];
 	}
+	vop_plane_state->enable = true;
+
+	return 0;
+
+out_disable:
+	vop_plane_state->enable = false;
+	return 0;
+}
+
+static void vop_plane_atomic_update(struct drm_plane *plane,
+		struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = plane->state;
+	struct drm_crtc *crtc = state->crtc;
+	struct vop_win *vop_win = to_vop_win(plane);
+	struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
+	const struct vop_win_data *win = vop_win->data;
+	struct vop *vop = to_vop(state->crtc);
+	struct drm_framebuffer *fb = state->fb;
+	unsigned int actual_w, actual_h;
+	unsigned int dsp_stx, dsp_sty;
+	uint32_t act_info, dsp_info, dsp_st;
+	struct drm_rect *src = &vop_plane_state->src;
+	struct drm_rect *dest = &vop_plane_state->dest;
+	uint32_t val;
+	bool rb_swap;
 
 	/*
-	 * If this plane update changes the plane's framebuffer, (or more
-	 * precisely, if this update has a different framebuffer than the last
-	 * update), enqueue it so we can track when it completes.
-	 *
-	 * Only when we discover that this update has completed, can we
-	 * unreference any previous framebuffers.
+	 * can't update plane when vop is disabled.
 	 */
-	mutex_lock(&vop->vsync_mutex);
-	if (fb != vop_win_last_pending_fb(vop_win)) {
-		ret = drm_crtc_vblank_get(crtc);
-		if (ret) {
-			DRM_ERROR("failed to get vblank, %d\n", ret);
-			mutex_unlock(&vop->vsync_mutex);
-			return ret;
-		}
+	if (!crtc || !vop->is_enabled)
+		return;
 
-		drm_framebuffer_reference(fb);
+	if (!vop_plane_state->enable) {
+		spin_lock(&vop->reg_lock);
 
-		ret = vop_win_queue_fb(vop_win, fb, yrgb_mst, event);
-		if (ret) {
-			drm_crtc_vblank_put(crtc);
-			mutex_unlock(&vop->vsync_mutex);
-			return ret;
-		}
+		VOP_WIN_SET(vop, win, enable, 0);
 
-		vop->vsync_work_pending = true;
+		spin_unlock(&vop->reg_lock);
+		return;
 	}
-	mutex_unlock(&vop->vsync_mutex);
+	actual_w = drm_rect_width(src) >> 16;
+	actual_h = drm_rect_height(src) >> 16;
+	act_info = (actual_h - 1) << 16 | ((actual_w - 1) & 0xffff);
+
+	dsp_info = (drm_rect_height(dest) - 1) << 16;
+	dsp_info |= (drm_rect_width(dest) - 1) & 0xffff;
+
+	dsp_stx = dest->x1 + crtc->mode.htotal - crtc->mode.hsync_start;
+	dsp_sty = dest->y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
+	dsp_st = dsp_sty << 16 | (dsp_stx & 0xffff);
 
 	spin_lock(&vop->reg_lock);
 
-	VOP_WIN_SET(vop, win, format, format);
-	VOP_WIN_SET(vop, win, yrgb_vir, y_vir_stride);
-	VOP_WIN_SET(vop, win, yrgb_mst, yrgb_mst);
-	if (is_yuv) {
-		VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
-		VOP_WIN_SET(vop, win, uv_mst, uv_mst);
+	VOP_WIN_SET(vop, win, format, vop_plane_state->format);
+	VOP_WIN_SET(vop, win, yrgb_vir, vop_plane_state->vir_stride[0]);
+	VOP_WIN_SET(vop, win, yrgb_mst, vop_plane_state->dma_addr[0]);
+	if (is_yuv_support(fb->pixel_format)) {
+		VOP_WIN_SET(vop, win, uv_vir, vop_plane_state->vir_stride[1]);
+		VOP_WIN_SET(vop, win, uv_mst, vop_plane_state->dma_addr[1]);
 	}
 
 	if (win->phy->scl)
 		scl_vop_cal_scl_fac(vop, win, actual_w, actual_h,
-				    dest.x2 - dest.x1, dest.y2 - dest.y1,
+				    drm_rect_width(dest), drm_rect_height(dest),
 				    fb->pixel_format);
 
-	val = (actual_h - 1) << 16;
-	val |= (actual_w - 1) & 0xffff;
-	VOP_WIN_SET(vop, win, act_info, val);
+	VOP_WIN_SET(vop, win, act_info, act_info);
+	VOP_WIN_SET(vop, win, dsp_info, dsp_info);
+	VOP_WIN_SET(vop, win, dsp_st, dsp_st);
 
-	val = (dest.y2 - dest.y1 - 1) << 16;
-	val |= (dest.x2 - dest.x1 - 1) & 0xffff;
-	VOP_WIN_SET(vop, win, dsp_info, val);
-	val = dsp_sty << 16;
-	val |= dsp_stx & 0xffff;
-	VOP_WIN_SET(vop, win, dsp_st, val);
+	rb_swap = has_rb_swapped(fb->pixel_format);
 	VOP_WIN_SET(vop, win, rb_swap, rb_swap);
 
-	if (is_alpha) {
+	if (is_alpha_support(fb->pixel_format)) {
 		VOP_WIN_SET(vop, win, dst_alpha_ctl,
 			    DST_FACTOR_M0(ALPHA_SRC_INVERSE));
 		val = SRC_ALPHA_EN(1) | SRC_COLOR_M0(ALPHA_SRC_PRE_MUL) |
@@ -976,86 +949,78 @@  static int vop_update_plane_event(struct drm_plane *plane,
 	}
 
 	VOP_WIN_SET(vop, win, enable, 1);
-
-	vop_cfg_done(vop);
 	spin_unlock(&vop->reg_lock);
-
-	return 0;
 }
 
-static int vop_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-			    struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			    unsigned int crtc_w, unsigned int crtc_h,
-			    uint32_t src_x, uint32_t src_y, uint32_t src_w,
-			    uint32_t src_h)
-{
-	return vop_update_plane_event(plane, crtc, fb, crtc_x, crtc_y, crtc_w,
-				      crtc_h, src_x, src_y, src_w, src_h,
-				      NULL);
-}
+static const struct drm_plane_helper_funcs plane_helper_funcs = {
+	.atomic_check = vop_plane_atomic_check,
+	.atomic_update = vop_plane_atomic_update,
+};
 
-static int vop_update_primary_plane(struct drm_crtc *crtc,
-				    struct drm_pending_vblank_event *event)
+void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 {
-	unsigned int crtc_w, crtc_h;
-
-	crtc_w = crtc->primary->fb->width - crtc->x;
-	crtc_h = crtc->primary->fb->height - crtc->y;
+	struct vop *vop = to_vop(crtc);
 
-	return vop_update_plane_event(crtc->primary, crtc, crtc->primary->fb,
-				      0, 0, crtc_w, crtc_h, crtc->x << 16,
-				      crtc->y << 16, crtc_w << 16,
-				      crtc_h << 16, event);
+	reinit_completion(&vop->wait_update_complete);
+	WARN_ON(!wait_for_completion_timeout(&vop->wait_update_complete, 100));
 }
 
-static int vop_disable_plane(struct drm_plane *plane)
+void vop_atomic_plane_reset(struct drm_plane *plane)
 {
-	struct vop_win *vop_win = to_vop_win(plane);
-	const struct vop_win_data *win = vop_win->data;
-	struct vop *vop;
-	int ret;
+	struct vop_plane_state *vop_plane_state =
+					to_vop_plane_state(plane->state);
 
-	if (!plane->crtc)
-		return 0;
+	if (plane->state && plane->state->fb)
+		drm_framebuffer_unreference(plane->state->fb);
 
-	vop = to_vop(plane->crtc);
+	kfree(vop_plane_state);
+	vop_plane_state = kzalloc(sizeof(*vop_plane_state), GFP_KERNEL);
+	if (!vop_plane_state)
+		return;
 
-	ret = drm_crtc_vblank_get(plane->crtc);
-	if (ret) {
-		DRM_ERROR("failed to get vblank, %d\n", ret);
-		return ret;
-	}
+	plane->state = &vop_plane_state->base;
+	plane->state->plane = plane;
+}
 
-	mutex_lock(&vop->vsync_mutex);
+struct drm_plane_state *
+vop_atomic_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct vop_plane_state *old_vop_plane_state;
+	struct vop_plane_state *vop_plane_state;
 
-	ret = vop_win_queue_fb(vop_win, NULL, 0, NULL);
-	if (ret) {
-		drm_crtc_vblank_put(plane->crtc);
-		mutex_unlock(&vop->vsync_mutex);
-		return ret;
-	}
+	if (WARN_ON(!plane->state))
+		return NULL;
 
-	vop->vsync_work_pending = true;
-	mutex_unlock(&vop->vsync_mutex);
+	old_vop_plane_state = to_vop_plane_state(plane->state);
+	vop_plane_state = kmemdup(old_vop_plane_state,
+				  sizeof(*vop_plane_state), GFP_KERNEL);
+	if (!vop_plane_state)
+		return NULL;
 
-	spin_lock(&vop->reg_lock);
-	VOP_WIN_SET(vop, win, enable, 0);
-	vop_cfg_done(vop);
-	spin_unlock(&vop->reg_lock);
+	if (vop_plane_state->base.fb)
+		drm_framebuffer_reference(vop_plane_state->base.fb);
 
-	return 0;
+	return &vop_plane_state->base;
 }
 
-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);
 }
 
 static const struct drm_plane_funcs vop_plane_funcs = {
-	.update_plane = vop_update_plane,
-	.disable_plane = vop_disable_plane,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy = vop_plane_destroy,
+	.reset = vop_atomic_plane_reset,
+	.atomic_duplicate_state = vop_atomic_plane_duplicate_state,
+	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
 int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
@@ -1116,29 +1081,10 @@  static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static int vop_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-				  struct drm_framebuffer *old_fb)
-{
-	int ret;
-
-	crtc->x = x;
-	crtc->y = y;
-
-	ret = vop_update_primary_plane(crtc, NULL);
-	if (ret < 0) {
-		DRM_ERROR("fail to update plane\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static int vop_crtc_mode_set(struct drm_crtc *crtc,
-			     struct drm_display_mode *mode,
-			     struct drm_display_mode *adjusted_mode,
-			     int x, int y, struct drm_framebuffer *fb)
+static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	struct drm_display_mode *adjusted_mode = &crtc->state->adjusted_mode;
 	u16 hsync_len = adjusted_mode->hsync_end - adjusted_mode->hsync_start;
 	u16 hdisplay = adjusted_mode->hdisplay;
 	u16 htotal = adjusted_mode->htotal;
@@ -1149,7 +1095,6 @@  static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	u16 vsync_len = adjusted_mode->vsync_end - adjusted_mode->vsync_start;
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
-	int ret, ret_clk;
 	uint32_t val;
 
 	/*
@@ -1171,7 +1116,6 @@  static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	default:
 		DRM_ERROR("unsupport connector_type[%d]\n",
 			  vop->connector_type);
-		ret = -EINVAL;
 		goto out;
 	};
 	VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode);
@@ -1193,9 +1137,6 @@  static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	VOP_CTRL_SET(vop, vact_st_end, val);
 	VOP_CTRL_SET(vop, vpost_st_end, val);
 
-	ret = vop_crtc_mode_set_base(crtc, x, y, fb);
-	if (ret)
-		goto out;
 
 	/*
 	 * reset dclk, take all mode config affect, so the clk would run in
@@ -1207,64 +1148,33 @@  static int vop_crtc_mode_set(struct drm_crtc *crtc,
 
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
 out:
-	ret_clk = clk_enable(vop->dclk);
-	if (ret_clk < 0) {
+	if (clk_enable(vop->dclk) < 0)
 		dev_err(vop->dev, "failed to enable dclk - %d\n", ret_clk);
-		return ret_clk;
-	}
 
-	return ret;
 }
 
-static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
-	.enable = vop_crtc_enable,
-	.disable = vop_crtc_disable,
-	.mode_fixup = vop_crtc_mode_fixup,
-	.mode_set = vop_crtc_mode_set,
-	.mode_set_base = vop_crtc_mode_set_base,
-};
-
-static int vop_crtc_page_flip(struct drm_crtc *crtc,
-			      struct drm_framebuffer *fb,
-			      struct drm_pending_vblank_event *event,
-			      uint32_t page_flip_flags)
+static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
+				  struct drm_crtc_state *old_crtc_state)
 {
 	struct vop *vop = to_vop(crtc);
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	int ret;
 
-	/* when the page flip is requested, crtc should be on */
-	if (!vop->is_enabled) {
-		DRM_DEBUG("page flip request rejected because crtc is off.\n");
-		return 0;
-	}
+	if (!vop->is_enabled)
+		return;
 
-	crtc->primary->fb = fb;
+	spin_lock(&vop->reg_lock);
 
-	ret = vop_update_primary_plane(crtc, event);
-	if (ret)
-		crtc->primary->fb = old_fb;
+	vop_cfg_done(vop);
 
-	return ret;
+	spin_unlock(&vop->reg_lock);
 }
 
-static void vop_win_state_complete(struct vop_win *vop_win,
-				   struct vop_win_state *state)
-{
-	struct vop *vop = vop_win->vop;
-	struct drm_crtc *crtc = &vop->crtc;
-	struct drm_device *drm = crtc->dev;
-	unsigned long flags;
-
-	if (state->event) {
-		spin_lock_irqsave(&drm->event_lock, flags);
-		drm_crtc_send_vblank_event(crtc, state->event);
-		spin_unlock_irqrestore(&drm->event_lock, flags);
-	}
-
-	list_del(&state->head);
-	drm_crtc_vblank_put(crtc);
-}
+static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
+	.enable = vop_crtc_enable,
+	.disable = vop_crtc_disable,
+	.mode_fixup = vop_crtc_mode_fixup,
+	.mode_set_nofb = vop_crtc_mode_set_nofb,
+	.atomic_flush = vop_crtc_atomic_flush,
+};
 
 static void vop_crtc_destroy(struct drm_crtc *crtc)
 {
@@ -1272,107 +1182,51 @@  static void vop_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs vop_crtc_funcs = {
-	.set_config = drm_crtc_helper_set_config,
-	.page_flip = vop_crtc_page_flip,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
 	.destroy = vop_crtc_destroy,
+	.reset = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
-static bool vop_win_state_is_active(struct vop_win *vop_win,
-				    struct vop_win_state *state)
+static bool vop_win_pending_is_complete(struct vop_win *vop_win)
 {
-	bool active = false;
-
-	if (state->fb) {
-		dma_addr_t yrgb_mst;
-
-		/* check yrgb_mst to tell if pending_fb is now front */
-		yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
-
-		active = (yrgb_mst == state->yrgb_mst);
-	} else {
-		bool enabled;
-
-		/* if enable bit is clear, plane is now disabled */
-		enabled = VOP_WIN_GET(vop_win->vop, vop_win->data, enable);
-
-		active = (enabled == 0);
-	}
-
-	return active;
-}
+	struct drm_plane *plane = &vop_win->base;
+	struct vop_plane_state *state = to_vop_plane_state(plane->state);
+	dma_addr_t yrgb_mst;
 
-static void vop_win_state_destroy(struct vop_win_state *state)
-{
-	struct drm_framebuffer *fb = state->fb;
+	if (!state->enable)
+		return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
 
-	if (fb)
-		drm_framebuffer_unreference(fb);
+	yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
 
-	kfree(state);
+	return yrgb_mst == state->dma_addr[0];
 }
 
-static void vop_win_update_state(struct vop_win *vop_win)
+static void vop_handle_vblank(struct vop *vop)
 {
-	struct vop_win_state *state, *n, *new_active = NULL;
-
-	/* Check if any pending states are now active */
-	list_for_each_entry(state, &vop_win->pending, head)
-		if (vop_win_state_is_active(vop_win, state)) {
-			new_active = state;
-			break;
-		}
-
-	if (!new_active)
-		return;
+	struct drm_device *drm = vop->drm_dev;
+	struct drm_crtc *crtc = &vop->crtc;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	unsigned long flags;
+	int i;
 
-	/*
-	 * Destroy any 'skipped' pending states - states that were queued
-	 * before the newly active state.
-	 */
-	list_for_each_entry_safe(state, n, &vop_win->pending, head) {
-		if (state == new_active)
-			break;
-		vop_win_state_complete(vop_win, state);
-		vop_win_state_destroy(state);
+	for (i = 0; i < vop->data->win_size; i++) {
+		if (!vop_win_pending_is_complete(&vop->win[i]))
+			return;
 	}
 
-	vop_win_state_complete(vop_win, new_active);
-
-	if (vop_win->active)
-		vop_win_state_destroy(vop_win->active);
-	vop_win->active = new_active;
-}
-
-static bool vop_win_has_pending_state(struct vop_win *vop_win)
-{
-	return !list_empty(&vop_win->pending);
-}
-
-static irqreturn_t vop_isr_thread(int irq, void *data)
-{
-	struct vop *vop = data;
-	const struct vop_data *vop_data = vop->data;
-	unsigned int i;
-
-	mutex_lock(&vop->vsync_mutex);
-
-	if (!vop->vsync_work_pending)
-		goto done;
+	if (event) {
+		spin_lock_irqsave(&drm->event_lock, flags);
 
-	vop->vsync_work_pending = false;
+		drm_crtc_send_vblank_event(crtc, event);
+		crtc->state->event = NULL;
 
-	for (i = 0; i < vop_data->win_size; i++) {
-		struct vop_win *vop_win = &vop->win[i];
-
-		vop_win_update_state(vop_win);
-		if (vop_win_has_pending_state(vop_win))
-			vop->vsync_work_pending = true;
+		spin_unlock_irqrestore(&drm->event_lock, flags);
 	}
-
-done:
-	mutex_unlock(&vop->vsync_mutex);
-
-	return IRQ_HANDLED;
+	if (!completion_done(&vop->wait_update_complete))
+		complete(&vop->wait_update_complete);
 }
 
 static irqreturn_t vop_isr(int irq, void *data)
@@ -1408,8 +1262,9 @@  static irqreturn_t vop_isr(int irq, void *data)
 
 	if (active_irqs & FS_INTR) {
 		drm_crtc_handle_vblank(crtc);
+		vop_handle_vblank(vop);
 		active_irqs &= ~FS_INTR;
-		ret = (vop->vsync_work_pending) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
 
 	/* Unhandled irqs are spurious. */
@@ -1454,6 +1309,7 @@  static int vop_create_crtc(struct vop *vop)
 		}
 
 		plane = &vop_win->base;
+		drm_plane_helper_add(plane, &plane_helper_funcs);
 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
 			primary = plane;
 		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
@@ -1489,6 +1345,7 @@  static int vop_create_crtc(struct vop *vop)
 			DRM_ERROR("failed to initialize overlay plane\n");
 			goto err_cleanup_crtc;
 		}
+		drm_plane_helper_add(&vop_win->base, &plane_helper_funcs);
 	}
 
 	port = of_get_child_by_name(dev->of_node, "port");
@@ -1499,6 +1356,7 @@  static int vop_create_crtc(struct vop *vop)
 	}
 
 	init_completion(&vop->dsp_hold_completion);
+	init_completion(&vop->wait_update_complete);
 	crtc->port = port;
 	rockchip_register_crtc_funcs(crtc, &private_crtc_funcs);
 
@@ -1632,7 +1490,6 @@  static void vop_win_init(struct vop *vop)
 
 		vop_win->data = win_data;
 		vop_win->vop = vop;
-		INIT_LIST_HEAD(&vop_win->pending);
 	}
 }
 
@@ -1693,8 +1550,8 @@  static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	mutex_init(&vop->vsync_mutex);
 
-	ret = devm_request_threaded_irq(dev, vop->irq, vop_isr, vop_isr_thread,
-					IRQF_SHARED, dev_name(dev), vop);
+	ret = devm_request_irq(dev, vop->irq, vop_isr,
+			       IRQF_SHARED, dev_name(dev), vop);
 	if (ret)
 		return ret;