diff mbox

[v3,03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers

Message ID 1467618039-7457-4-git-send-email-gnuiyl@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ying Liu July 4, 2016, 7:40 a.m. UTC
Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
transitional atomic helpers.  The crtc->mode_set_nofb callback is added
so that the primary plane is no longer tied to the CRTC.  Check/update
logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
are always successful.  Also, some necessary logics are tweaked for a smooth
transition.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
v2->v3:
* A minor change to simplify the way we find crtc->enabled in
  drm_plane_helper_funcs->atomic_check.

v1->v2:
* Get the overlay ipu plane resource when initializing the relevant CRTC
  and do not get ipu plane resource any more when updating plane to avoid
  resource allocation failure.
* Remove obsolete comments from ipu_crtc_enable/disable().

 drivers/gpu/drm/imx/ipuv3-crtc.c  | 197 ++++++++-------
 drivers/gpu/drm/imx/ipuv3-plane.c | 511 +++++++++++++++++++++++---------------
 drivers/gpu/drm/imx/ipuv3-plane.h |  16 +-
 drivers/gpu/ipu-v3/ipu-dc.c       |   5 +-
 drivers/gpu/ipu-v3/ipu-di.c       |   3 -
 5 files changed, 410 insertions(+), 322 deletions(-)

Comments

Russell King (Oracle) Aug. 13, 2016, 10:11 a.m. UTC | #1
On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote:
> Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> transitional atomic helpers.  The crtc->mode_set_nofb callback is added
> so that the primary plane is no longer tied to the CRTC.  Check/update
> logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> are always successful.  Also, some necessary logics are tweaked for a smooth
> transition.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>

This patch causes a regression with my xorg ddx driver:

[    29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument

I'm not sure why (yet).
Russell King (Oracle) Aug. 13, 2016, 10:45 a.m. UTC | #2
On Sat, Aug 13, 2016 at 11:11:54AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote:
> > Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> > transitional atomic helpers.  The crtc->mode_set_nofb callback is added
> > so that the primary plane is no longer tied to the CRTC.  Check/update
> > logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> > are always successful.  Also, some necessary logics are tweaked for a smooth
> > transition.
> > 
> > Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> 
> This patch causes a regression with my xorg ddx driver:
> 
> [    29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument
> 
> I'm not sure why (yet).

Hi,

Enabling DRM debugging doesn't seem to provide much in the way of clues:

[drm:drm_ioctl] pid=1015, dev=0xe200, auth=1, DRM_IOCTL_MODE_SETCRTC
[drm:drm_mode_setcrtc] [CRTC:24:crtc-0]
[drm:drm_mode_setcrtc] [CONNECTOR:34:HDMI-A-1]
[drm:drm_crtc_helper_set_config]
[drm:drm_crtc_helper_set_config] [CRTC:24:crtc-0] [FB:48] #connectors=1 (x y) (0 0)
[drm:drm_crtc_helper_set_config] [CONNECTOR:34:HDMI-A-1] to [CRTC:24:crtc-0]
[drm:drm_crtc_helper_set_config] attempting to set mode from userspace
[drm:drm_mode_debug_printmodeline] Modeline 49:"1920x1080" 0 148500 1920 2448 2492 2640 1080 1084 1089 1125 0x0 0x5
[drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
[drm:drm_vblank_off] crtc 0, vblank enabled 0, inmodeset 0
[drm:drm_update_vblank_count] updating vblank count on crtc 0: current=2, diff=0, hw=0 hw_last=0
[drm:drm_mode_object_reference] OBJ ID: 51 (1)
[drm:drm_mode_object_unreference] OBJ ID: 51 (2)
[drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81ee00
[drm:drm_mode_object_reference] OBJ ID: 47 (3)
[drm:drm_mode_object_unreference] OBJ ID: 47 (4)
[drm:drm_mode_object_reference] OBJ ID: 48 (2)
[drm:drm_atomic_set_fb_for_plane] Set [FB:48] for plane state ed632d00
[drm:drm_mode_object_unreference] OBJ ID: 48 (3)
[drm:drm_mode_object_unreference] OBJ ID: 51 (1)
[drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
[drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
[drm:drm_mode_object_reference] OBJ ID: 52 (1)
[drm:drm_mode_object_unreference] OBJ ID: 52 (2)
[drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81f600
[drm:drm_mode_object_reference] OBJ ID: 47 (3)
[drm:drm_mode_object_unreference] OBJ ID: 47 (4)
[drm:drm_mode_object_reference] OBJ ID: 47 (3)
[drm:drm_atomic_set_fb_for_plane] Set [FB:47] for plane state ed632d00
[drm:drm_mode_object_unreference] OBJ ID: 47 (4)
[drm:drm_mode_object_unreference] OBJ ID: 52 (1)
[drm:drm_crtc_helper_set_mode] [ENCODER:33:TMDS-33] set [MODE:46:1920x1080]
[drm:drm_vblank_on] crtc 0, vblank enabled 0, inmodeset 1
[drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 2640, vtotal 1125, vdisplay 1080
[drm:drm_calc_timestamping_constants] crtc 24: clock 148500 kHz framedur 20000000 linedur 17777
[drm:drm_mode_object_reference] OBJ ID: 47 (3)
[drm:drm_mode_object_unreference] OBJ ID: 47 (4)
[drm:drm_mode_object_unreference] OBJ ID: 48 (2)
[drm:drm_mode_object_unreference] OBJ ID: 34 (4)
[drm:drm_ioctl] ret = -22

With a bit more debugging, this is what's failing:

ipu_plane_atomic_check:442: fail
[drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]

        if (old_fb && (state->src_w != old_state->src_w ||
                              state->src_h != old_state->src_h ||
                              fb->pixel_format != old_fb->pixel_format)) {
                printk("%s:%d: fail\n", __func__, __LINE__);
                return -EINVAL;
        }

This test is stupid as it stands - it means we can _never_ change the
format or size of _any_ plane, something that the old code allowed:

-       if (ipu_plane->enabled) {
-               if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
-                   fb->pixel_format != ipu_plane->base.fb->pixel_format)
-                       return -EINVAL;

This used to work because we'd call ipu_crtc_prepare()->ipu_fb_disable()
before the mode set, which would clear ipu_plane->enabled, causing this
test to be skipped.  However, in the new code, we merely test for the
presence of the previous framebuffer, which is really not the same thing
at all.

The old functionality needs to be restored because significantly
changing the displayed mode is something which must be supported with
HDMI.

Thanks.
Russell King (Oracle) Aug. 13, 2016, 11:29 a.m. UTC | #3
On Sat, Aug 13, 2016 at 11:45:31AM +0100, Russell King - ARM Linux wrote:
> On Sat, Aug 13, 2016 at 11:11:54AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote:
> > > Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> > > transitional atomic helpers.  The crtc->mode_set_nofb callback is added
> > > so that the primary plane is no longer tied to the CRTC.  Check/update
> > > logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> > > are always successful.  Also, some necessary logics are tweaked for a smooth
> > > transition.
> > > 
> > > Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> > 
> > This patch causes a regression with my xorg ddx driver:
> > 
> > [    29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument
> > 
> > I'm not sure why (yet).
> 
> Hi,
> 
> Enabling DRM debugging doesn't seem to provide much in the way of clues:
> 
> [drm:drm_ioctl] pid=1015, dev=0xe200, auth=1, DRM_IOCTL_MODE_SETCRTC
> [drm:drm_mode_setcrtc] [CRTC:24:crtc-0]
> [drm:drm_mode_setcrtc] [CONNECTOR:34:HDMI-A-1]
> [drm:drm_crtc_helper_set_config]
> [drm:drm_crtc_helper_set_config] [CRTC:24:crtc-0] [FB:48] #connectors=1 (x y) (0 0)
> [drm:drm_crtc_helper_set_config] [CONNECTOR:34:HDMI-A-1] to [CRTC:24:crtc-0]
> [drm:drm_crtc_helper_set_config] attempting to set mode from userspace
> [drm:drm_mode_debug_printmodeline] Modeline 49:"1920x1080" 0 148500 1920 2448 2492 2640 1080 1084 1089 1125 0x0 0x5
> [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> [drm:drm_vblank_off] crtc 0, vblank enabled 0, inmodeset 0
> [drm:drm_update_vblank_count] updating vblank count on crtc 0: current=2, diff=0, hw=0 hw_last=0
> [drm:drm_mode_object_reference] OBJ ID: 51 (1)
> [drm:drm_mode_object_unreference] OBJ ID: 51 (2)
> [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81ee00
> [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> [drm:drm_mode_object_reference] OBJ ID: 48 (2)
> [drm:drm_atomic_set_fb_for_plane] Set [FB:48] for plane state ed632d00
> [drm:drm_mode_object_unreference] OBJ ID: 48 (3)
> [drm:drm_mode_object_unreference] OBJ ID: 51 (1)
> [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> [drm:drm_mode_object_reference] OBJ ID: 52 (1)
> [drm:drm_mode_object_unreference] OBJ ID: 52 (2)
> [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81f600
> [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> [drm:drm_atomic_set_fb_for_plane] Set [FB:47] for plane state ed632d00
> [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> [drm:drm_mode_object_unreference] OBJ ID: 52 (1)
> [drm:drm_crtc_helper_set_mode] [ENCODER:33:TMDS-33] set [MODE:46:1920x1080]
> [drm:drm_vblank_on] crtc 0, vblank enabled 0, inmodeset 1
> [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 2640, vtotal 1125, vdisplay 1080
> [drm:drm_calc_timestamping_constants] crtc 24: clock 148500 kHz framedur 20000000 linedur 17777
> [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> [drm:drm_mode_object_unreference] OBJ ID: 48 (2)
> [drm:drm_mode_object_unreference] OBJ ID: 34 (4)
> [drm:drm_ioctl] ret = -22
> 
> With a bit more debugging, this is what's failing:
> 
> ipu_plane_atomic_check:442: fail
> [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> 
>         if (old_fb && (state->src_w != old_state->src_w ||
>                               state->src_h != old_state->src_h ||
>                               fb->pixel_format != old_fb->pixel_format)) {
>                 printk("%s:%d: fail\n", __func__, __LINE__);
>                 return -EINVAL;
>         }
> 
> This test is stupid as it stands - it means we can _never_ change the
> format or size of _any_ plane, something that the old code allowed:
> 
> -       if (ipu_plane->enabled) {
> -               if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
> -                   fb->pixel_format != ipu_plane->base.fb->pixel_format)
> -                       return -EINVAL;
> 
> This used to work because we'd call ipu_crtc_prepare()->ipu_fb_disable()
> before the mode set, which would clear ipu_plane->enabled, causing this
> test to be skipped.  However, in the new code, we merely test for the
> presence of the previous framebuffer, which is really not the same thing
> at all.
> 
> The old functionality needs to be restored because significantly
> changing the displayed mode is something which must be supported with
> HDMI.

Bypassing the above check also shows that:

        if (old_fb && fb->pitches[0] != old_fb->pitches[0]) {
                printk("%s:%d: fail\n", __func__, __LINE__);
                return -EINVAL;
        }

also fails.  Disabling this as well results in loss of sync on the
HDMI display, although the mode set now succeeds.

The more I think about this, the more I come to one of two possible
conclusions:

1. These checks were not appropriate with the old code as we were
   always disabling the display channel prior to making changes.

   We need atomic mode set to work in exactly the same way as the
   previous code: as a series of disable-modify-enable set of steps,
   so that we don't need these restrictive and regression causing
   checks.

or

2. imx-drm has these particular restrictions which make it inappropriate
   to be converted to atomic mode set, as we always need to go through
   a disable-modify-enable series of steps - which means the atomic
   mode set changes for imx-drm need to be reverted back to this patch.

I'm pretty sure (2) doesn't really apply, because I see no reason why
we can't disable the display channel while we reconfigure it.  That,
to me, seems to be an entirely reasonable thing to do.
Daniel Vetter Aug. 14, 2016, 9:44 a.m. UTC | #4
On Sat, Aug 13, 2016 at 12:29:47PM +0100, Russell King - ARM Linux wrote:
> On Sat, Aug 13, 2016 at 11:45:31AM +0100, Russell King - ARM Linux wrote:
> > On Sat, Aug 13, 2016 at 11:11:54AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote:
> > > > Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> > > > transitional atomic helpers.  The crtc->mode_set_nofb callback is added
> > > > so that the primary plane is no longer tied to the CRTC.  Check/update
> > > > logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> > > > are always successful.  Also, some necessary logics are tweaked for a smooth
> > > > transition.
> > > > 
> > > > Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> > > 
> > > This patch causes a regression with my xorg ddx driver:
> > > 
> > > [    29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument
> > > 
> > > I'm not sure why (yet).
> > 
> > Hi,
> > 
> > Enabling DRM debugging doesn't seem to provide much in the way of clues:
> > 
> > [drm:drm_ioctl] pid=1015, dev=0xe200, auth=1, DRM_IOCTL_MODE_SETCRTC
> > [drm:drm_mode_setcrtc] [CRTC:24:crtc-0]
> > [drm:drm_mode_setcrtc] [CONNECTOR:34:HDMI-A-1]
> > [drm:drm_crtc_helper_set_config]
> > [drm:drm_crtc_helper_set_config] [CRTC:24:crtc-0] [FB:48] #connectors=1 (x y) (0 0)
> > [drm:drm_crtc_helper_set_config] [CONNECTOR:34:HDMI-A-1] to [CRTC:24:crtc-0]
> > [drm:drm_crtc_helper_set_config] attempting to set mode from userspace
> > [drm:drm_mode_debug_printmodeline] Modeline 49:"1920x1080" 0 148500 1920 2448 2492 2640 1080 1084 1089 1125 0x0 0x5
> > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> > [drm:drm_vblank_off] crtc 0, vblank enabled 0, inmodeset 0
> > [drm:drm_update_vblank_count] updating vblank count on crtc 0: current=2, diff=0, hw=0 hw_last=0
> > [drm:drm_mode_object_reference] OBJ ID: 51 (1)
> > [drm:drm_mode_object_unreference] OBJ ID: 51 (2)
> > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81ee00
> > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > [drm:drm_mode_object_reference] OBJ ID: 48 (2)
> > [drm:drm_atomic_set_fb_for_plane] Set [FB:48] for plane state ed632d00
> > [drm:drm_mode_object_unreference] OBJ ID: 48 (3)
> > [drm:drm_mode_object_unreference] OBJ ID: 51 (1)
> > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> > [drm:drm_mode_object_reference] OBJ ID: 52 (1)
> > [drm:drm_mode_object_unreference] OBJ ID: 52 (2)
> > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81f600
> > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > [drm:drm_atomic_set_fb_for_plane] Set [FB:47] for plane state ed632d00
> > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > [drm:drm_mode_object_unreference] OBJ ID: 52 (1)
> > [drm:drm_crtc_helper_set_mode] [ENCODER:33:TMDS-33] set [MODE:46:1920x1080]
> > [drm:drm_vblank_on] crtc 0, vblank enabled 0, inmodeset 1
> > [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 2640, vtotal 1125, vdisplay 1080
> > [drm:drm_calc_timestamping_constants] crtc 24: clock 148500 kHz framedur 20000000 linedur 17777
> > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > [drm:drm_mode_object_unreference] OBJ ID: 48 (2)
> > [drm:drm_mode_object_unreference] OBJ ID: 34 (4)
> > [drm:drm_ioctl] ret = -22
> > 
> > With a bit more debugging, this is what's failing:
> > 
> > ipu_plane_atomic_check:442: fail
> > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> > 
> >         if (old_fb && (state->src_w != old_state->src_w ||
> >                               state->src_h != old_state->src_h ||
> >                               fb->pixel_format != old_fb->pixel_format)) {
> >                 printk("%s:%d: fail\n", __func__, __LINE__);
> >                 return -EINVAL;
> >         }
> > 
> > This test is stupid as it stands - it means we can _never_ change the
> > format or size of _any_ plane, something that the old code allowed:
> > 
> > -       if (ipu_plane->enabled) {
> > -               if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
> > -                   fb->pixel_format != ipu_plane->base.fb->pixel_format)
> > -                       return -EINVAL;
> > 
> > This used to work because we'd call ipu_crtc_prepare()->ipu_fb_disable()
> > before the mode set, which would clear ipu_plane->enabled, causing this
> > test to be skipped.  However, in the new code, we merely test for the
> > presence of the previous framebuffer, which is really not the same thing
> > at all.
> > 
> > The old functionality needs to be restored because significantly
> > changing the displayed mode is something which must be supported with
> > HDMI.
> 
> Bypassing the above check also shows that:
> 
>         if (old_fb && fb->pitches[0] != old_fb->pitches[0]) {
>                 printk("%s:%d: fail\n", __func__, __LINE__);
>                 return -EINVAL;
>         }
> 
> also fails.  Disabling this as well results in loss of sync on the
> HDMI display, although the mode set now succeeds.
> 
> The more I think about this, the more I come to one of two possible
> conclusions:
> 
> 1. These checks were not appropriate with the old code as we were
>    always disabling the display channel prior to making changes.
> 
>    We need atomic mode set to work in exactly the same way as the
>    previous code: as a series of disable-modify-enable set of steps,
>    so that we don't need these restrictive and regression causing
>    checks.
> 
> or
> 
> 2. imx-drm has these particular restrictions which make it inappropriate
>    to be converted to atomic mode set, as we always need to go through
>    a disable-modify-enable series of steps - which means the atomic
>    mode set changes for imx-drm need to be reverted back to this patch.
> 
> I'm pretty sure (2) doesn't really apply, because I see no reason why
> we can't disable the display channel while we reconfigure it.  That,
> to me, seems to be an entirely reasonable thing to do.

Already ddiscussed this at lenght on irc with Peter Senna. If you have
plane update restrictions where you need to cycle the crtc completely,
the right thing to do is to set crtc_state->mode_changed instead of return
-EINVAL. The helper/core will then convert that into an EINVAL if
userspace doesn't set ALLOW_MODESET. And since the helper function to map
set_config to atomic does this, it should all just work.

Note: There's about 3 such conditions in imx' atomic_check function which
needs this change.
-Daniel
Daniel Vetter Aug. 14, 2016, 10:46 a.m. UTC | #5
On Sun, Aug 14, 2016 at 11:44:14AM +0200, Daniel Vetter wrote:
> On Sat, Aug 13, 2016 at 12:29:47PM +0100, Russell King - ARM Linux wrote:
> > On Sat, Aug 13, 2016 at 11:45:31AM +0100, Russell King - ARM Linux wrote:
> > > On Sat, Aug 13, 2016 at 11:11:54AM +0100, Russell King - ARM Linux wrote:
> > > > On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote:
> > > > > Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> > > > > transitional atomic helpers.  The crtc->mode_set_nofb callback is added
> > > > > so that the primary plane is no longer tied to the CRTC.  Check/update
> > > > > logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> > > > > are always successful.  Also, some necessary logics are tweaked for a smooth
> > > > > transition.
> > > > >
> > > > > Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> > > >
> > > > This patch causes a regression with my xorg ddx driver:
> > > >
> > > > [    29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument
> > > >
> > > > I'm not sure why (yet).
> > >
> > > Hi,
> > >
> > > Enabling DRM debugging doesn't seem to provide much in the way of clues:
> > >
> > > [drm:drm_ioctl] pid=1015, dev=0xe200, auth=1, DRM_IOCTL_MODE_SETCRTC
> > > [drm:drm_mode_setcrtc] [CRTC:24:crtc-0]
> > > [drm:drm_mode_setcrtc] [CONNECTOR:34:HDMI-A-1]
> > > [drm:drm_crtc_helper_set_config]
> > > [drm:drm_crtc_helper_set_config] [CRTC:24:crtc-0] [FB:48] #connectors=1 (x y) (0 0)
> > > [drm:drm_crtc_helper_set_config] [CONNECTOR:34:HDMI-A-1] to [CRTC:24:crtc-0]
> > > [drm:drm_crtc_helper_set_config] attempting to set mode from userspace
> > > [drm:drm_mode_debug_printmodeline] Modeline 49:"1920x1080" 0 148500 1920 2448 2492 2640 1080 1084 1089 1125 0x0 0x5
> > > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> > > [drm:drm_vblank_off] crtc 0, vblank enabled 0, inmodeset 0
> > > [drm:drm_update_vblank_count] updating vblank count on crtc 0: current=2, diff=0, hw=0 hw_last=0
> > > [drm:drm_mode_object_reference] OBJ ID: 51 (1)
> > > [drm:drm_mode_object_unreference] OBJ ID: 51 (2)
> > > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81ee00
> > > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > > [drm:drm_mode_object_reference] OBJ ID: 48 (2)
> > > [drm:drm_atomic_set_fb_for_plane] Set [FB:48] for plane state ed632d00
> > > [drm:drm_mode_object_unreference] OBJ ID: 48 (3)
> > > [drm:drm_mode_object_unreference] OBJ ID: 51 (1)
> > > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> > > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0]
> > > [drm:drm_mode_object_reference] OBJ ID: 52 (1)
> > > [drm:drm_mode_object_unreference] OBJ ID: 52 (2)
> > > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81f600
> > > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > > [drm:drm_atomic_set_fb_for_plane] Set [FB:47] for plane state ed632d00
> > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > > [drm:drm_mode_object_unreference] OBJ ID: 52 (1)
> > > [drm:drm_crtc_helper_set_mode] [ENCODER:33:TMDS-33] set [MODE:46:1920x1080]
> > > [drm:drm_vblank_on] crtc 0, vblank enabled 0, inmodeset 1
> > > [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 2640, vtotal 1125, vdisplay 1080
> > > [drm:drm_calc_timestamping_constants] crtc 24: clock 148500 kHz framedur 20000000 linedur 17777
> > > [drm:drm_mode_object_reference] OBJ ID: 47 (3)
> > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4)
> > > [drm:drm_mode_object_unreference] OBJ ID: 48 (2)
> > > [drm:drm_mode_object_unreference] OBJ ID: 34 (4)
> > > [drm:drm_ioctl] ret = -22
> > >
> > > With a bit more debugging, this is what's failing:
> > >
> > > ipu_plane_atomic_check:442: fail
> > > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0]
> > >
> > >         if (old_fb && (state->src_w != old_state->src_w ||
> > >                               state->src_h != old_state->src_h ||
> > >                               fb->pixel_format != old_fb->pixel_format)) {
> > >                 printk("%s:%d: fail\n", __func__, __LINE__);
> > >                 return -EINVAL;
> > >         }
> > >
> > > This test is stupid as it stands - it means we can _never_ change the
> > > format or size of _any_ plane, something that the old code allowed:
> > >
> > > -       if (ipu_plane->enabled) {
> > > -               if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
> > > -                   fb->pixel_format != ipu_plane->base.fb->pixel_format)
> > > -                       return -EINVAL;
> > >
> > > This used to work because we'd call ipu_crtc_prepare()->ipu_fb_disable()
> > > before the mode set, which would clear ipu_plane->enabled, causing this
> > > test to be skipped.  However, in the new code, we merely test for the
> > > presence of the previous framebuffer, which is really not the same thing
> > > at all.
> > >
> > > The old functionality needs to be restored because significantly
> > > changing the displayed mode is something which must be supported with
> > > HDMI.
> >
> > Bypassing the above check also shows that:
> >
> >         if (old_fb && fb->pitches[0] != old_fb->pitches[0]) {
> >                 printk("%s:%d: fail\n", __func__, __LINE__);
> >                 return -EINVAL;
> >         }
> >
> > also fails.  Disabling this as well results in loss of sync on the
> > HDMI display, although the mode set now succeeds.
> >
> > The more I think about this, the more I come to one of two possible
> > conclusions:
> >
> > 1. These checks were not appropriate with the old code as we were
> >    always disabling the display channel prior to making changes.
> >
> >    We need atomic mode set to work in exactly the same way as the
> >    previous code: as a series of disable-modify-enable set of steps,
> >    so that we don't need these restrictive and regression causing
> >    checks.
> >
> > or
> >
> > 2. imx-drm has these particular restrictions which make it inappropriate
> >    to be converted to atomic mode set, as we always need to go through
> >    a disable-modify-enable series of steps - which means the atomic
> >    mode set changes for imx-drm need to be reverted back to this patch.
> >
> > I'm pretty sure (2) doesn't really apply, because I see no reason why
> > we can't disable the display channel while we reconfigure it.  That,
> > to me, seems to be an entirely reasonable thing to do.
>
> Already ddiscussed this at lenght on irc with Peter Senna. If you have
> plane update restrictions where you need to cycle the crtc completely,
> the right thing to do is to set crtc_state->mode_changed instead of return
> -EINVAL. The helper/core will then convert that into an EINVAL if
> userspace doesn't set ALLOW_MODESET. And since the helper function to map
> set_config to atomic does this, it should all just work.
>
> Note: There's about 3 such conditions in imx' atomic_check function which
> needs this change.

You also need to update the overall atomic_check to re-run the modeset
checks after the plane checks again (since plane checks can update
mode_changed). Peter Senna has the full diff, but apparently it doesn't
work either. So there's probably more bugs somewhere.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index fc04041..ba880fa 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -73,7 +73,7 @@  struct ipu_crtc {
 
 #define to_ipu_crtc(x) container_of(x, struct ipu_crtc, base)
 
-static void ipu_fb_enable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_enable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
@@ -81,30 +81,30 @@  static void ipu_fb_enable(struct ipu_crtc *ipu_crtc)
 		return;
 
 	ipu_dc_enable(ipu);
-	ipu_plane_enable(ipu_crtc->plane[0]);
-	/* Start DC channel and DI after IDMAC */
 	ipu_dc_enable_channel(ipu_crtc->dc);
 	ipu_di_enable(ipu_crtc->di);
-	drm_crtc_vblank_on(&ipu_crtc->base);
-
 	ipu_crtc->enabled = 1;
+
+	/*
+	 * In order not to be warned on enabling vblank failure,
+	 * we should call drm_crtc_vblank_on() after ->enabled is set to 1.
+	 */
+	drm_crtc_vblank_on(&ipu_crtc->base);
 }
 
-static void ipu_fb_disable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_disable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
 	if (!ipu_crtc->enabled)
 		return;
 
-	/* Stop DC channel and DI before IDMAC */
 	ipu_dc_disable_channel(ipu_crtc->dc);
 	ipu_di_disable(ipu_crtc->di);
-	ipu_plane_disable(ipu_crtc->plane[0]);
 	ipu_dc_disable(ipu);
-	drm_crtc_vblank_off(&ipu_crtc->base);
-
 	ipu_crtc->enabled = 0;
+
+	drm_crtc_vblank_off(&ipu_crtc->base);
 }
 
 static void ipu_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -115,12 +115,12 @@  static void ipu_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 	switch (mode) {
 	case DRM_MODE_DPMS_ON:
-		ipu_fb_enable(ipu_crtc);
+		ipu_crtc_enable(ipu_crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		ipu_fb_disable(ipu_crtc);
+		ipu_crtc_disable(ipu_crtc);
 		break;
 	}
 }
@@ -234,79 +234,6 @@  static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.page_flip = ipu_page_flip,
 };
 
-static int ipu_crtc_mode_set(struct drm_crtc *crtc,
-			       struct drm_display_mode *orig_mode,
-			       struct drm_display_mode *mode,
-			       int x, int y,
-			       struct drm_framebuffer *old_fb)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_encoder *encoder;
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-	struct ipu_di_signal_cfg sig_cfg = {};
-	unsigned long encoder_types = 0;
-	int ret;
-
-	dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__,
-			mode->hdisplay);
-	dev_dbg(ipu_crtc->dev, "%s: mode->vdisplay: %d\n", __func__,
-			mode->vdisplay);
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
-			encoder_types |= BIT(encoder->encoder_type);
-
-	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
-		__func__, encoder_types);
-
-	/*
-	 * If we have DAC or LDB, then we need the IPU DI clock to be
-	 * the same as the LDB DI clock. For TVDAC, derive the IPU DI
-	 * clock from 27 MHz TVE_DI clock, but allow to divide it.
-	 */
-	if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) |
-			     BIT(DRM_MODE_ENCODER_LVDS)))
-		sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT;
-	else if (encoder_types & BIT(DRM_MODE_ENCODER_TVDAC))
-		sig_cfg.clkflags = IPU_DI_CLKMODE_EXT;
-	else
-		sig_cfg.clkflags = 0;
-
-	sig_cfg.enable_pol = !(ipu_crtc->bus_flags & DRM_BUS_FLAG_DE_LOW);
-	/* Default to driving pixel data on negative clock edges */
-	sig_cfg.clk_pol = !!(ipu_crtc->bus_flags &
-			     DRM_BUS_FLAG_PIXDATA_POSEDGE);
-	sig_cfg.bus_format = ipu_crtc->bus_format;
-	sig_cfg.v_to_h_sync = 0;
-	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
-	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
-
-	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
-
-	ret = ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
-			       mode->flags & DRM_MODE_FLAG_INTERLACE,
-			       ipu_crtc->bus_format, mode->hdisplay);
-	if (ret) {
-		dev_err(ipu_crtc->dev,
-				"initializing display controller failed with %d\n",
-				ret);
-		return ret;
-	}
-
-	ret = ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
-	if (ret) {
-		dev_err(ipu_crtc->dev,
-				"initializing panel failed with %d\n", ret);
-		return ret;
-	}
-
-	return ipu_plane_mode_set(ipu_crtc->plane[0], crtc, mode,
-				  crtc->primary->fb,
-				  0, 0, mode->hdisplay, mode->vdisplay,
-				  x, y, mode->hdisplay, mode->vdisplay,
-				  mode->flags & DRM_MODE_FLAG_INTERLACE);
-}
-
 static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
 {
 	unsigned long flags;
@@ -330,8 +257,7 @@  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
 	if (ipu_crtc->flip_state == IPU_FLIP_SUBMITTED) {
 		struct ipu_plane *plane = ipu_crtc->plane[0];
 
-		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb,
-				   plane->x, plane->y);
+		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb);
 		ipu_crtc_handle_pageflip(ipu_crtc);
 		queue_work(ipu_crtc->flip_queue,
 			   &ipu_crtc->flip_work->unref_work);
@@ -355,6 +281,9 @@  static bool ipu_crtc_mode_fixup(struct drm_crtc *crtc,
 	if (ret)
 		return false;
 
+	if ((vm.vsync_len == 0) || (vm.hsync_len == 0))
+		return false;
+
 	drm_display_mode_from_videomode(&vm, adjusted_mode);
 
 	return true;
@@ -364,28 +293,95 @@  static void ipu_crtc_prepare(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
-	ipu_fb_disable(ipu_crtc);
+	ipu_crtc_disable(ipu_crtc);
 }
 
 static void ipu_crtc_commit(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
-	ipu_fb_enable(ipu_crtc);
+	ipu_crtc_enable(ipu_crtc);
+}
+
+static int ipu_crtc_atomic_check(struct drm_crtc *crtc,
+				 struct drm_crtc_state *state)
+{
+	return 0;
+}
+
+static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_encoder *encoder;
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	struct ipu_di_signal_cfg sig_cfg = {};
+	unsigned long encoder_types = 0;
+
+	dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__,
+			mode->hdisplay);
+	dev_dbg(ipu_crtc->dev, "%s: mode->vdisplay: %d\n", __func__,
+			mode->vdisplay);
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
+		if (encoder->crtc == crtc)
+			encoder_types |= BIT(encoder->encoder_type);
+
+	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
+		__func__, encoder_types);
+
+	/*
+	 * If we have DAC or LDB, then we need the IPU DI clock to be
+	 * the same as the LDB DI clock. For TVDAC, derive the IPU DI
+	 * clock from 27 MHz TVE_DI clock, but allow to divide it.
+	 */
+	if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) |
+			     BIT(DRM_MODE_ENCODER_LVDS)))
+		sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT;
+	else if (encoder_types & BIT(DRM_MODE_ENCODER_TVDAC))
+		sig_cfg.clkflags = IPU_DI_CLKMODE_EXT;
+	else
+		sig_cfg.clkflags = 0;
+
+	sig_cfg.enable_pol = !(ipu_crtc->bus_flags & DRM_BUS_FLAG_DE_LOW);
+	/* Default to driving pixel data on negative clock edges */
+	sig_cfg.clk_pol = !!(ipu_crtc->bus_flags &
+			     DRM_BUS_FLAG_PIXDATA_POSEDGE);
+	sig_cfg.bus_format = ipu_crtc->bus_format;
+	sig_cfg.v_to_h_sync = 0;
+	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
+	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
+
+	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
+
+	ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
+			 mode->flags & DRM_MODE_FLAG_INTERLACE,
+			 ipu_crtc->bus_format, mode->hdisplay);
+	ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
 }
 
 static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
 	.dpms = ipu_crtc_dpms,
 	.mode_fixup = ipu_crtc_mode_fixup,
-	.mode_set = ipu_crtc_mode_set,
+	.mode_set = drm_helper_crtc_mode_set,
+	.mode_set_nofb = ipu_crtc_mode_set_nofb,
 	.prepare = ipu_crtc_prepare,
 	.commit = ipu_crtc_commit,
+	.atomic_check = ipu_crtc_atomic_check,
 };
 
 static int ipu_enable_vblank(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
+	/*
+	 * ->commit is done after ->mode_set in drm_crtc_helper_set_mode(),
+	 * so waiting for vblank in drm_plane_helper_commit() will timeout.
+	 * Check the state here to avoid the waiting.
+	 */
+	if (!ipu_crtc->enabled)
+		return -EINVAL;
+
 	enable_irq(ipu_crtc->irq);
 
 	return 0;
@@ -496,8 +492,16 @@  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 						IPU_DP_FLOW_SYNC_FG,
 						drm_crtc_mask(&ipu_crtc->base),
 						DRM_PLANE_TYPE_OVERLAY);
-		if (IS_ERR(ipu_crtc->plane[1]))
+		if (IS_ERR(ipu_crtc->plane[1])) {
 			ipu_crtc->plane[1] = NULL;
+		} else {
+			ret = ipu_plane_get_resources(ipu_crtc->plane[1]);
+			if (ret) {
+				dev_err(ipu_crtc->dev, "getting plane 1 "
+					"resources failed with %d.\n", ret);
+				goto err_put_plane0_res;
+			}
+		}
 	}
 
 	ipu_crtc->irq = ipu_plane_irq(ipu_crtc->plane[0]);
@@ -505,7 +509,7 @@  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 			"imx_drm", ipu_crtc);
 	if (ret < 0) {
 		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
-		goto err_put_plane_res;
+		goto err_put_plane1_res;
 	}
 	/* Only enable IRQ when we actually need it to trigger work. */
 	disable_irq(ipu_crtc->irq);
@@ -514,7 +518,10 @@  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 
 	return 0;
 
-err_put_plane_res:
+err_put_plane1_res:
+	if (ipu_crtc->plane[1])
+		ipu_plane_put_resources(ipu_crtc->plane[1]);
+err_put_plane0_res:
 	ipu_plane_put_resources(ipu_crtc->plane[0]);
 err_remove_crtc:
 	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
@@ -554,8 +561,10 @@  static void ipu_drm_unbind(struct device *dev, struct device *master,
 	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
 
 	destroy_workqueue(ipu_crtc->flip_queue);
-	ipu_plane_put_resources(ipu_crtc->plane[0]);
 	ipu_put_resources(ipu_crtc);
+	if (ipu_crtc->plane[1])
+		ipu_plane_put_resources(ipu_crtc->plane[1]);
+	ipu_plane_put_resources(ipu_crtc->plane[0]);
 }
 
 static const struct component_ops ipu_crtc_ops = {
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 02701de..59f2353 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -16,6 +16,7 @@ 
 #include <drm/drmP.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_plane_helper.h>
 
 #include "video/imx-ipu-v3.h"
 #include "ipuv3-plane.h"
@@ -53,12 +54,15 @@  int ipu_plane_irq(struct ipu_plane *ipu_plane)
 				     IPU_IRQ_EOF);
 }
 
-int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
-		       int x, int y)
+int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb)
 {
-	struct drm_gem_cma_object *cma_obj[3];
-	unsigned long eba, ubo, vbo;
+	struct drm_gem_cma_object *cma_obj[3], *old_cma_obj[3];
+	struct drm_plane_state *state = ipu_plane->base.state;
+	struct drm_framebuffer *old_fb = state->fb;
+	unsigned long eba, ubo, vbo, old_eba, old_ubo, old_vbo;
 	int active, i;
+	int x = state->src_x >> 16;
+	int y = state->src_y >> 16;
 
 	for (i = 0; i < drm_format_num_planes(fb->pixel_format); i++) {
 		cma_obj[i] = drm_fb_cma_get_gem_obj(fb, i);
@@ -68,6 +72,14 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		}
 	}
 
+	for (i = 0; i < drm_format_num_planes(old_fb->pixel_format); i++) {
+		old_cma_obj[i] = drm_fb_cma_get_gem_obj(old_fb, i);
+		if (!old_cma_obj[i]) {
+			DRM_DEBUG_KMS("plane %d entry is null.\n", i);
+			return -EFAULT;
+		}
+	}
+
 	eba = cma_obj[0]->paddr + fb->offsets[0] +
 	      fb->pitches[0] * y + (fb->bits_per_pixel >> 3) * x;
 
@@ -81,13 +93,11 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		return -EINVAL;
 	}
 
-	if (ipu_plane->enabled && fb->pitches[0] != ipu_plane->stride[0]) {
+	if (fb->pitches[0] != old_fb->pitches[0]) {
 		DRM_DEBUG_KMS("pitches must not change while plane is enabled.\n");
 		return -EINVAL;
 	}
 
-	ipu_plane->stride[0] = fb->pitches[0];
-
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
@@ -104,6 +114,14 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		vbo = cma_obj[2]->paddr + fb->offsets[2] +
 		      fb->pitches[2] * y / 2 + x / 2 - eba;
 
+		old_eba = old_cma_obj[0]->paddr + old_fb->offsets[0] +
+		      old_fb->pitches[0] * y +
+		      (old_fb->bits_per_pixel >> 3) * x;
+		old_ubo = old_cma_obj[1]->paddr + old_fb->offsets[1] +
+		      old_fb->pitches[1] * y / 2 + x / 2 - old_eba;
+		old_vbo = old_cma_obj[2]->paddr + old_fb->offsets[2] +
+		      old_fb->pitches[2] * y / 2 + x / 2 - old_eba;
+
 		if ((ubo & 0x7) || (vbo & 0x7)) {
 			DRM_DEBUG_KMS("U/V buffer offsets must be a multiple of 8.\n");
 			return -EINVAL;
@@ -114,8 +132,7 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 			return -EINVAL;
 		}
 
-		if (ipu_plane->enabled && ((ipu_plane->u_offset != ubo) ||
-					   (ipu_plane->v_offset != vbo))) {
+		if (old_ubo != ubo || old_vbo != vbo) {
 			DRM_DEBUG_KMS("U/V buffer offsets must not change while plane is enabled.\n");
 			return -EINVAL;
 		}
@@ -130,16 +147,11 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 			return -EINVAL;
 		}
 
-		if (ipu_plane->enabled &&
-		    (ipu_plane->stride[1] != fb->pitches[1])) {
+		if (old_fb->pitches[1] != fb->pitches[1]) {
 			DRM_DEBUG_KMS("U/V pitches must not change while plane is enabled.\n");
 			return -EINVAL;
 		}
 
-		ipu_plane->u_offset = ubo;
-		ipu_plane->v_offset = vbo;
-		ipu_plane->stride[1] = fb->pitches[1];
-
 		dev_dbg(ipu_plane->base.dev->dev,
 			"phys = %pad %pad %pad, x = %d, y = %d",
 			&cma_obj[0]->paddr, &cma_obj[1]->paddr,
@@ -151,164 +163,111 @@  int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		break;
 	}
 
-	if (ipu_plane->enabled) {
-		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
-		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
-	} else {
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
-	}
-
-	/* cache offsets for subsequent pageflips */
-	ipu_plane->x = x;
-	ipu_plane->y = y;
+	active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
+	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
+	ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
 
 	return 0;
 }
 
-int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
-		       struct drm_display_mode *mode,
-		       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, bool interlaced)
+static inline unsigned long
+drm_plane_state_to_eba(struct drm_plane_state *state)
 {
-	struct device *dev = ipu_plane->base.dev->dev;
-	int ret;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
 
-	/* no scaling */
-	if (src_w != crtc_w || src_h != crtc_h)
-		return -EINVAL;
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	BUG_ON(!cma_obj);
 
-	if (ipu_plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
-		/* full plane doesn't support partial off screen */
-		if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
-			crtc_h != mode->vdisplay)
-			return -EINVAL;
+	return cma_obj->paddr + fb->offsets[0] +
+	       fb->pitches[0] * (state->src_y >> 16) +
+	       (fb->bits_per_pixel >> 3) * (state->src_x >> 16);
+}
 
-		/* full plane minimum width is 13 pixels */
-		if (crtc_w < 13)
-			return -EINVAL;
-	} else if (ipu_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
-		/* clip to crtc bounds */
-		if (crtc_x < 0) {
-			if (-crtc_x > crtc_w)
-				return -EINVAL;
-			src_x += -crtc_x;
-			src_w -= -crtc_x;
-			crtc_w -= -crtc_x;
-			crtc_x = 0;
-		}
-		if (crtc_y < 0) {
-			if (-crtc_y > crtc_h)
-				return -EINVAL;
-			src_y += -crtc_y;
-			src_h -= -crtc_y;
-			crtc_h -= -crtc_y;
-			crtc_y = 0;
-		}
-		if (crtc_x + crtc_w > mode->hdisplay) {
-			if (crtc_x > mode->hdisplay)
-				return -EINVAL;
-			crtc_w = mode->hdisplay - crtc_x;
-			src_w = crtc_w;
-		}
-		if (crtc_y + crtc_h > mode->vdisplay) {
-			if (crtc_y > mode->vdisplay)
-				return -EINVAL;
-			crtc_h = mode->vdisplay - crtc_y;
-			src_h = crtc_h;
-		}
-	} else
-		return -EINVAL;
-	if (crtc_h < 2)
-		return -EINVAL;
+static inline unsigned long
+drm_plane_state_to_ubo(struct drm_plane_state *state)
+{
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
+	unsigned long eba = drm_plane_state_to_eba(state);
 
-	/*
-	 * since we cannot touch active IDMAC channels, we do not support
-	 * resizing the enabled plane or changing its format
-	 */
-	if (ipu_plane->enabled) {
-		if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
-		    fb->pixel_format != ipu_plane->base.fb->pixel_format)
-			return -EINVAL;
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
+	BUG_ON(!cma_obj);
 
-		return ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
-	}
+	return cma_obj->paddr + fb->offsets[1] +
+	       fb->pitches[1] * (state->src_y >> 16) / 2 +
+	       (state->src_x >> 16) / 2 - eba;
+}
 
-	switch (ipu_plane->dp_flow) {
-	case IPU_DP_FLOW_SYNC_BG:
-		ret = ipu_dp_setup_channel(ipu_plane->dp,
-				IPUV3_COLORSPACE_RGB,
-				IPUV3_COLORSPACE_RGB);
-		if (ret) {
-			dev_err(dev,
-				"initializing display processor failed with %d\n",
-				ret);
-			return ret;
-		}
-		ipu_dp_set_global_alpha(ipu_plane->dp, true, 0, true);
-		break;
-	case IPU_DP_FLOW_SYNC_FG:
-		ipu_dp_setup_channel(ipu_plane->dp,
-				ipu_drm_fourcc_to_colorspace(fb->pixel_format),
-				IPUV3_COLORSPACE_UNKNOWN);
-		ipu_dp_set_window_pos(ipu_plane->dp, crtc_x, crtc_y);
-		/* Enable local alpha on partial plane */
-		switch (fb->pixel_format) {
-		case DRM_FORMAT_ARGB1555:
-		case DRM_FORMAT_ABGR1555:
-		case DRM_FORMAT_RGBA5551:
-		case DRM_FORMAT_BGRA5551:
-		case DRM_FORMAT_ARGB4444:
-		case DRM_FORMAT_ARGB8888:
-		case DRM_FORMAT_ABGR8888:
-		case DRM_FORMAT_RGBA8888:
-		case DRM_FORMAT_BGRA8888:
-			ipu_dp_set_global_alpha(ipu_plane->dp, false, 0, false);
-			break;
-		default:
+static inline unsigned long
+drm_plane_state_to_vbo(struct drm_plane_state *state)
+{
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
+	unsigned long eba = drm_plane_state_to_eba(state);
+
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
+	BUG_ON(!cma_obj);
+
+	return cma_obj->paddr + fb->offsets[2] +
+	       fb->pitches[2] * (state->src_y >> 16) / 2 +
+	       (state->src_x >> 16) / 2 - eba;
+}
+
+void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
+			       struct drm_plane_state *old_state)
+{
+	struct drm_plane *plane = &ipu_plane->base;
+	struct drm_plane_state *state = plane->state;
+	struct drm_framebuffer *fb = state->fb;
+	unsigned long eba, ubo, vbo;
+	int active;
+
+	eba = drm_plane_state_to_eba(state);
+
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		if (old_state->fb)
 			break;
-		}
-	}
 
-	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, crtc_w);
+		/*
+		 * Multiplanar formats have to meet the following restrictions:
+		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
+		 * - EBA, UBO and VBO are a multiple of 8
+		 * - UBO and VBO are unsigned and not larger than 0xfffff8
+		 * - Only EBA may be changed while scanout is active
+		 * - The strides of U and V planes must be identical.
+		 */
+		ubo = drm_plane_state_to_ubo(state);
+		vbo = drm_plane_state_to_vbo(state);
 
-	ipu_cpmem_zero(ipu_plane->ipu_ch);
-	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, src_w, src_h);
-	ret = ipu_cpmem_set_fmt(ipu_plane->ipu_ch, fb->pixel_format);
-	if (ret < 0) {
-		dev_err(dev, "unsupported pixel format 0x%08x\n",
-			fb->pixel_format);
-		return ret;
-	}
-	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
-	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
-	ipu_cpmem_set_stride(ipu_plane->ipu_ch, fb->pitches[0]);
+		if (fb->pixel_format == DRM_FORMAT_YUV420)
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], ubo, vbo);
+		else
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], vbo, ubo);
 
-	ret = ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
-	if (ret < 0)
-		return ret;
-	if (interlaced)
-		ipu_cpmem_interlaced_scan(ipu_plane->ipu_ch, fb->pitches[0]);
-
-	if (fb->pixel_format == DRM_FORMAT_YUV420) {
-		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-					      ipu_plane->stride[1],
-					      ipu_plane->u_offset,
-					      ipu_plane->v_offset);
-	} else if (fb->pixel_format == DRM_FORMAT_YVU420) {
-		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-					      ipu_plane->stride[1],
-					      ipu_plane->v_offset,
-					      ipu_plane->u_offset);
-	}
+		dev_dbg(ipu_plane->base.dev->dev,
+			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
+			state->src_x >> 16, state->src_y >> 16);
+		break;
+	default:
+		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
+			eba, state->src_x >> 16, state->src_y >> 16);
 
-	ipu_plane->w = src_w;
-	ipu_plane->h = src_h;
+		break;
+	}
 
-	return 0;
+	if (old_state->fb) {
+		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
+		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
+	} else {
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
+	}
 }
 
 void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
@@ -355,7 +314,7 @@  err_out:
 	return ret;
 }
 
-void ipu_plane_enable(struct ipu_plane *ipu_plane)
+static void ipu_plane_enable(struct ipu_plane *ipu_plane)
 {
 	if (ipu_plane->dp)
 		ipu_dp_enable(ipu_plane->ipu);
@@ -363,14 +322,10 @@  void ipu_plane_enable(struct ipu_plane *ipu_plane)
 	ipu_idmac_enable_channel(ipu_plane->ipu_ch);
 	if (ipu_plane->dp)
 		ipu_dp_enable_channel(ipu_plane->dp);
-
-	ipu_plane->enabled = true;
 }
 
-void ipu_plane_disable(struct ipu_plane *ipu_plane)
+static void ipu_plane_disable(struct ipu_plane *ipu_plane)
 {
-	ipu_plane->enabled = false;
-
 	ipu_idmac_wait_busy(ipu_plane->ipu_ch, 50);
 
 	if (ipu_plane->dp)
@@ -381,74 +336,216 @@  void ipu_plane_disable(struct ipu_plane *ipu_plane)
 		ipu_dp_disable(ipu_plane->ipu);
 }
 
-/*
- * drm_plane API
- */
-
-static int ipu_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)
+static int ipu_disable_plane(struct drm_plane *plane)
 {
 	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
-	int ret = 0;
 
-	DRM_DEBUG_KMS("plane - %p\n", plane);
-
-	if (!ipu_plane->enabled)
-		ret = ipu_plane_get_resources(ipu_plane);
-	if (ret < 0)
-		return ret;
-
-	ret = ipu_plane_mode_set(ipu_plane, crtc, &crtc->hwmode, fb,
-			crtc_x, crtc_y, crtc_w, crtc_h,
-			src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
-			false);
-	if (ret < 0) {
-		ipu_plane_put_resources(ipu_plane);
-		return ret;
-	}
-
-	if (crtc != plane->crtc)
-		dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n",
-				plane->crtc, crtc);
+	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
 
-	if (!ipu_plane->enabled)
-		ipu_plane_enable(ipu_plane);
+	ipu_plane_disable(ipu_plane);
 
 	return 0;
 }
 
-static int ipu_disable_plane(struct drm_plane *plane)
+static void ipu_plane_destroy(struct drm_plane *plane)
 {
 	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
 
 	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
 
-	if (ipu_plane->enabled)
-		ipu_plane_disable(ipu_plane);
+	ipu_disable_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(ipu_plane);
+}
 
-	ipu_plane_put_resources(ipu_plane);
+static const struct drm_plane_funcs ipu_plane_funcs = {
+	.update_plane	= drm_plane_helper_update,
+	.disable_plane	= drm_plane_helper_disable,
+	.destroy	= ipu_plane_destroy,
+};
+
+static int ipu_plane_atomic_check(struct drm_plane *plane,
+				  struct drm_plane_state *state)
+{
+	struct drm_plane_state *old_state = plane->state;
+	struct drm_crtc_state *crtc_state;
+	struct device *dev = plane->dev->dev;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
+	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
+
+	/* Ok to disable */
+	if (!fb)
+		return old_fb ? 0 : -EINVAL;
+
+	/* CRTC should be enabled */
+	if (!state->crtc->enabled)
+		return -EINVAL;
+
+	/* no scaling */
+	if (state->src_w >> 16 != state->crtc_w ||
+	    state->src_h >> 16 != state->crtc_h)
+		return -EINVAL;
+
+	crtc_state = state->crtc->state;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		/* full plane doesn't support partial off screen */
+		if (state->crtc_x || state->crtc_y ||
+		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
+		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
+			return -EINVAL;
+
+		/* full plane minimum width is 13 pixels */
+		if (state->crtc_w < 13)
+			return -EINVAL;
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		if (state->crtc_x < 0 || state->crtc_y < 0)
+			return -EINVAL;
+
+		if (state->crtc_x + state->crtc_w >
+		    crtc_state->adjusted_mode.hdisplay)
+			return -EINVAL;
+		if (state->crtc_y + state->crtc_h >
+		    crtc_state->adjusted_mode.vdisplay)
+			return -EINVAL;
+		break;
+	default:
+		dev_warn(dev, "Unsupported plane type\n");
+		return -EINVAL;
+	}
+
+	if (state->crtc_h < 2)
+		return -EINVAL;
+
+	/*
+	 * since we cannot touch active IDMAC channels, we do not support
+	 * resizing the enabled plane or changing its format
+	 */
+	if (old_fb && (state->src_w != old_state->src_w ||
+			      state->src_h != old_state->src_h ||
+			      fb->pixel_format != old_fb->pixel_format))
+		return -EINVAL;
+
+	eba = drm_plane_state_to_eba(state);
+
+	if (eba & 0x7)
+		return -EINVAL;
+
+	if (fb->pitches[0] < 1 || fb->pitches[0] > 16384)
+		return -EINVAL;
+
+	if (old_fb && fb->pitches[0] != old_fb->pitches[0])
+		return -EINVAL;
+
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		/*
+		 * Multiplanar formats have to meet the following restrictions:
+		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
+		 * - EBA, UBO and VBO are a multiple of 8
+		 * - UBO and VBO are unsigned and not larger than 0xfffff8
+		 * - Only EBA may be changed while scanout is active
+		 * - The strides of U and V planes must be identical.
+		 */
+		ubo = drm_plane_state_to_ubo(state);
+		vbo = drm_plane_state_to_vbo(state);
+
+		if ((ubo & 0x7) || (vbo & 0x7))
+			return -EINVAL;
+
+		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
+			return -EINVAL;
+
+		if (old_fb) {
+			old_ubo = drm_plane_state_to_ubo(old_state);
+			old_vbo = drm_plane_state_to_vbo(old_state);
+			if (ubo != old_ubo || vbo != old_vbo)
+				return -EINVAL;
+		}
+
+		if (fb->pitches[1] != fb->pitches[2])
+			return -EINVAL;
+
+		if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
+			return -EINVAL;
+
+		if (old_fb && old_fb->pitches[1] != fb->pitches[1])
+			return -EINVAL;
+	}
 
 	return 0;
 }
 
-static void ipu_plane_destroy(struct drm_plane *plane)
+static void ipu_plane_atomic_disable(struct drm_plane *plane,
+				     struct drm_plane_state *old_state)
+{
+	ipu_disable_plane(plane);
+}
+
+static void ipu_plane_atomic_update(struct drm_plane *plane,
+				    struct drm_plane_state *old_state)
 {
 	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+	struct drm_plane_state *state = plane->state;
+	enum ipu_color_space ics;
 
-	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
+	if (old_state->fb) {
+		ipu_plane_atomic_set_base(ipu_plane, old_state);
+		return;
+	}
 
-	ipu_disable_plane(plane);
-	drm_plane_cleanup(plane);
-	kfree(ipu_plane);
+	switch (ipu_plane->dp_flow) {
+	case IPU_DP_FLOW_SYNC_BG:
+		ipu_dp_setup_channel(ipu_plane->dp,
+					IPUV3_COLORSPACE_RGB,
+					IPUV3_COLORSPACE_RGB);
+		ipu_dp_set_global_alpha(ipu_plane->dp, true, 0, true);
+		break;
+	case IPU_DP_FLOW_SYNC_FG:
+		ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
+		ipu_dp_setup_channel(ipu_plane->dp, ics,
+					IPUV3_COLORSPACE_UNKNOWN);
+		ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
+					state->crtc_y);
+		/* Enable local alpha on partial plane */
+		switch (state->fb->pixel_format) {
+		case DRM_FORMAT_ARGB1555:
+		case DRM_FORMAT_ABGR1555:
+		case DRM_FORMAT_RGBA5551:
+		case DRM_FORMAT_BGRA5551:
+		case DRM_FORMAT_ARGB4444:
+		case DRM_FORMAT_ARGB8888:
+		case DRM_FORMAT_ABGR8888:
+		case DRM_FORMAT_RGBA8888:
+		case DRM_FORMAT_BGRA8888:
+			ipu_dp_set_global_alpha(ipu_plane->dp, false, 0, false);
+			break;
+		default:
+			break;
+		}
+	}
+
+	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
+
+	ipu_cpmem_zero(ipu_plane->ipu_ch);
+	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
+					state->src_h >> 16);
+	ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
+	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
+	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
+	ipu_cpmem_set_stride(ipu_plane->ipu_ch, state->fb->pitches[0]);
+	ipu_plane_atomic_set_base(ipu_plane, old_state);
+	ipu_plane_enable(ipu_plane);
 }
 
-static const struct drm_plane_funcs ipu_plane_funcs = {
-	.update_plane	= ipu_update_plane,
-	.disable_plane	= ipu_disable_plane,
-	.destroy	= ipu_plane_destroy,
+static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = {
+	.atomic_check = ipu_plane_atomic_check,
+	.atomic_disable = ipu_plane_atomic_disable,
+	.atomic_update = ipu_plane_atomic_update,
 };
 
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
@@ -481,5 +578,7 @@  struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 		return ERR_PTR(ret);
 	}
 
+	drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
+
 	return ipu_plane;
 }
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
index 4448fd4..c51a44b 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.h
+++ b/drivers/gpu/drm/imx/ipuv3-plane.h
@@ -23,17 +23,6 @@  struct ipu_plane {
 
 	int			dma;
 	int			dp_flow;
-
-	int			x;
-	int			y;
-	int			w;
-	int			h;
-
-	unsigned int		u_offset;
-	unsigned int		v_offset;
-	unsigned int		stride[2];
-
-	bool			enabled;
 };
 
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
@@ -48,10 +37,7 @@  int ipu_plane_mode_set(struct ipu_plane *plane, struct drm_crtc *crtc,
 		       uint32_t src_x, uint32_t src_y, uint32_t src_w,
 		       uint32_t src_h, bool interlaced);
 
-void ipu_plane_enable(struct ipu_plane *plane);
-void ipu_plane_disable(struct ipu_plane *plane);
-int ipu_plane_set_base(struct ipu_plane *plane, struct drm_framebuffer *fb,
-		       int x, int y);
+int ipu_plane_set_base(struct ipu_plane *plane, struct drm_framebuffer *fb);
 
 int ipu_plane_get_resources(struct ipu_plane *plane);
 void ipu_plane_put_resources(struct ipu_plane *plane);
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 2f29780..cd72dad 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -178,10 +178,7 @@  int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
 	dc->di = ipu_di_get_num(di);
 
 	map = ipu_bus_format_to_map(bus_format);
-	if (map < 0) {
-		dev_dbg(priv->dev, "IPU_DISP: No MAP\n");
-		return map;
-	}
+	BUG_ON(map < 0);
 
 	/*
 	 * In interlaced mode we need more counters to create the asymmetric
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index 359268e..a8d87dd 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -572,9 +572,6 @@  int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	dev_dbg(di->ipu->dev, "disp %d: panel size = %d x %d\n",
 		di->id, sig->mode.hactive, sig->mode.vactive);
 
-	if ((sig->mode.vsync_len == 0) || (sig->mode.hsync_len == 0))
-		return -EINVAL;
-
 	dev_dbg(di->ipu->dev, "Clocks: IPU %luHz DI %luHz Needed %luHz\n",
 		clk_get_rate(di->clk_ipu),
 		clk_get_rate(di->clk_di),