Re: Plane transitional helper prototypes mismatch their methods

Re: Plane transitional helper prototypes mismatch their methods

[Russell King - ARM Linux]

Ping.

On Mon, Jun 25, 2018 at 04:52:16PM +0100, Russell King - ARM Linux wrote:
> Hi,
> 
> Currently, the transitional plane helpers have prototypes that
> do not have the drm_modeset_acquire_ctx argument:
> 
> int drm_plane_helper_update(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);
> int drm_plane_helper_disable(struct drm_plane *plane);
> 
> However, the helper methods have this as the last argument:
> 
>         int (*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,
>                             struct drm_modeset_acquire_ctx *ctx);
>         int (*disable_plane)(struct drm_plane *plane,
>                              struct drm_modeset_acquire_ctx *ctx);
> 
> Are these transitional helpers supposed to be plugged into these
> methods directly?  Looking back in the history, the following commit
> to i915 seems to suggest that they were designed to be plugged
> directly into these methods:
> 
> commit ea2c67bb4affa84080c616920f3899f123786e56
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Tue Dec 23 10:41:52 2014 -0800
> 
>     drm/i915: Move to atomic plane helpers (v9)
> 
> It seems easy to add this argument to drm_plane_helper_update() as
> it has no current users, but the drm_plane_helper_disable()
> transitional helper seems to be used extensively by what are
> otherwise fully atomic modeset drivers, despite being described as
> only a transitional helper.
> 
> drivers/gpu/drm/arm/hdlcd_crtc.c:   drm_plane_helper_disable(plane);
> drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c: drm_plane_helper_disable(plane);
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c: drm_plane_helper_disable(plane);
> drivers/gpu/drm/arc/arcpgu_crtc.c:  drm_plane_helper_disable(plane);
> drivers/gpu/drm/sti/sti_hqvdp.c:    drm_plane_helper_disable(drm_plane);
> drivers/gpu/drm/sti/sti_gdp.c:      drm_plane_helper_disable(drm_plane);
> drivers/gpu/drm/sti/sti_cursor.c:   drm_plane_helper_disable(drm_plane);
> drivers/gpu/drm/vc4/vc4_plane.c:    drm_plane_helper_disable(plane);
> drivers/gpu/drm/zte/zx_plane.c:     drm_plane_helper_disable(plane);
> 
> Are these drivers buggy using the transitional helper, which won't do
> anything but change the state of that single plane, leaving the CRTC,
> encoders, bridges, etc all active?
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plane transitional helper prototypes mismatch their methods

[Daniel Vetter]

On Sun, Jul 01, 2018 at 04:34:02PM +0100, Russell King - ARM Linux wrote:
> Ping.
> 
> On Mon, Jun 25, 2018 at 04:52:16PM +0100, Russell King - ARM Linux wrote:
> > Hi,
> > 
> > Currently, the transitional plane helpers have prototypes that
> > do not have the drm_modeset_acquire_ctx argument:
> > 
> > int drm_plane_helper_update(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);
> > int drm_plane_helper_disable(struct drm_plane *plane);
> > 
> > However, the helper methods have this as the last argument:
> > 
> >         int (*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,
> >                             struct drm_modeset_acquire_ctx *ctx);
> >         int (*disable_plane)(struct drm_plane *plane,
> >                              struct drm_modeset_acquire_ctx *ctx);
> > 
> > Are these transitional helpers supposed to be plugged into these
> > methods directly?  Looking back in the history, the following commit
> > to i915 seems to suggest that they were designed to be plugged
> > directly into these methods:
> > 
> > commit ea2c67bb4affa84080c616920f3899f123786e56
> > Author: Matt Roper <matthew.d.roper@intel.com>
> > Date:   Tue Dec 23 10:41:52 2014 -0800
> > 
> >     drm/i915: Move to atomic plane helpers (v9)
> > 
> > It seems easy to add this argument to drm_plane_helper_update() as
> > it has no current users, but the drm_plane_helper_disable()
> > transitional helper seems to be used extensively by what are
> > otherwise fully atomic modeset drivers, despite being described as
> > only a transitional helper.

Yes, I forgotten to add the acquire_ctx to them to make them match up
again. Upfront Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the
patch to re-add that.

> > drivers/gpu/drm/arm/hdlcd_crtc.c:   drm_plane_helper_disable(plane);
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c: drm_plane_helper_disable(plane);
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c: drm_plane_helper_disable(plane);
> > drivers/gpu/drm/arc/arcpgu_crtc.c:  drm_plane_helper_disable(plane);
> > drivers/gpu/drm/sti/sti_hqvdp.c:    drm_plane_helper_disable(drm_plane);
> > drivers/gpu/drm/sti/sti_gdp.c:      drm_plane_helper_disable(drm_plane);
> > drivers/gpu/drm/sti/sti_cursor.c:   drm_plane_helper_disable(drm_plane);
> > drivers/gpu/drm/vc4/vc4_plane.c:    drm_plane_helper_disable(plane);
> > drivers/gpu/drm/zte/zx_plane.c:     drm_plane_helper_disable(plane);
> > 
> > Are these drivers buggy using the transitional helper, which won't do
> > anything but change the state of that single plane, leaving the CRTC,
> > encoders, bridges, etc all active?

Yes that's all buggy usage. I've started sprinkling
WARN_ON(drm_drv_uses_adomit_modeset) on such legacy functions, but
drm_plane_helper_disable() is used a bit too much really.

It's all the same cargo-cult though in the plane->destroy hook. What
drivers should do instead is:
- Shut down the hw before cleaning up their modeset objects using
  something like drm_atomic_helper_shutdown().
- Remove the drm_plane_helper_disable().

But for your patch to add the ctx argument everywhere I think just passing
NULL is ok too. It's not going to make things worse :-) Since that patch
needs to touch a bunch of drivers probably best if we get it landed
through drm-misc-next.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plane transitional helper prototypes mismatch their methods

[Russell King - ARM Linux]

On Mon, Jul 02, 2018 at 10:47:03AM +0200, Daniel Vetter wrote:
> On Sun, Jul 01, 2018 at 04:34:02PM +0100, Russell King - ARM Linux wrote:
> > > drivers/gpu/drm/arm/hdlcd_crtc.c:   drm_plane_helper_disable(plane);
> > > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c: drm_plane_helper_disable(plane);
> > > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c: drm_plane_helper_disable(plane);
> > > drivers/gpu/drm/arc/arcpgu_crtc.c:  drm_plane_helper_disable(plane);
> > > drivers/gpu/drm/sti/sti_hqvdp.c:    drm_plane_helper_disable(drm_plane);
> > > drivers/gpu/drm/sti/sti_gdp.c:      drm_plane_helper_disable(drm_plane);
> > > drivers/gpu/drm/sti/sti_cursor.c:   drm_plane_helper_disable(drm_plane);
> > > drivers/gpu/drm/vc4/vc4_plane.c:    drm_plane_helper_disable(plane);
> > > drivers/gpu/drm/zte/zx_plane.c:     drm_plane_helper_disable(plane);
> > > 
> > > Are these drivers buggy using the transitional helper, which won't do
> > > anything but change the state of that single plane, leaving the CRTC,
> > > encoders, bridges, etc all active?
> 
> Yes that's all buggy usage. I've started sprinkling
> WARN_ON(drm_drv_uses_adomit_modeset) on such legacy functions, but
> drm_plane_helper_disable() is used a bit too much really.

However, isn't there a problem in doing that, as going through the
transition process means you:
1. switch planes to use the plane transitional helpers
2. migrate crtc to mode_set_nofb()
3. migrate page_flip to use a transitional implementation
4. clean up to use the plane state for all properties
5. switch to atomic modeset by adding .atomic_check / .atomic_commit
   and DRIVER_ATOMIC flag
6. migrate away from transitional helpers

as separate stages - which means there's a brief period where you have
the .atomic_commit method populated (and hence
drm_drv_uses_atomic_modeset() returns true) but the transitional
helpers are still being used.

I've found if you do (5) and (6) in one go, it becomes quite a large
set of changes:

 drivers/gpu/drm/armada/armada_crtc.c    | 308 +-------------------------------
 drivers/gpu/drm/armada/armada_crtc.h    |  20 ---
 drivers/gpu/drm/armada/armada_overlay.c | 175 ++++--------------
 drivers/gpu/drm/armada/armada_plane.c   |   4 +-
 4 files changed, 36 insertions(+), 471 deletions(-)

> It's all the same cargo-cult though in the plane->destroy hook. What
> drivers should do instead is:
> - Shut down the hw before cleaning up their modeset objects using
>   something like drm_atomic_helper_shutdown().

Hmm, that's something else I've missed in my conversion... thanks for
pointing it out. ;)

> - Remove the drm_plane_helper_disable().
> 
> But for your patch to add the ctx argument everywhere I think just passing
> NULL is ok too. It's not going to make things worse :-) Since that patch
> needs to touch a bunch of drivers probably best if we get it landed
> through drm-misc-next.

Ok, I've a commit for that - I just need to merge it with the one I
already have adding the ctx argument, grab drm-misc-next and make sure
it applies there... I'll try to get it out in shortly.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plane transitional helper prototypes mismatch their methods

[Daniel Vetter]

On Mon, Jul 2, 2018 at 5:41 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Jul 02, 2018 at 10:47:03AM +0200, Daniel Vetter wrote:
>> On Sun, Jul 01, 2018 at 04:34:02PM +0100, Russell King - ARM Linux wrote:
>> > > drivers/gpu/drm/arm/hdlcd_crtc.c:   drm_plane_helper_disable(plane);
>> > > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c: drm_plane_helper_disable(plane);
>> > > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c: drm_plane_helper_disable(plane);
>> > > drivers/gpu/drm/arc/arcpgu_crtc.c:  drm_plane_helper_disable(plane);
>> > > drivers/gpu/drm/sti/sti_hqvdp.c:    drm_plane_helper_disable(drm_plane);
>> > > drivers/gpu/drm/sti/sti_gdp.c:      drm_plane_helper_disable(drm_plane);
>> > > drivers/gpu/drm/sti/sti_cursor.c:   drm_plane_helper_disable(drm_plane);
>> > > drivers/gpu/drm/vc4/vc4_plane.c:    drm_plane_helper_disable(plane);
>> > > drivers/gpu/drm/zte/zx_plane.c:     drm_plane_helper_disable(plane);
>> > >
>> > > Are these drivers buggy using the transitional helper, which won't do
>> > > anything but change the state of that single plane, leaving the CRTC,
>> > > encoders, bridges, etc all active?
>>
>> Yes that's all buggy usage. I've started sprinkling
>> WARN_ON(drm_drv_uses_adomit_modeset) on such legacy functions, but
>> drm_plane_helper_disable() is used a bit too much really.
>
> However, isn't there a problem in doing that, as going through the
> transition process means you:
> 1. switch planes to use the plane transitional helpers
> 2. migrate crtc to mode_set_nofb()
> 3. migrate page_flip to use a transitional implementation
> 4. clean up to use the plane state for all properties
> 5. switch to atomic modeset by adding .atomic_check / .atomic_commit
>    and DRIVER_ATOMIC flag
> 6. migrate away from transitional helpers
>
> as separate stages - which means there's a brief period where you have
> the .atomic_commit method populated (and hence
> drm_drv_uses_atomic_modeset() returns true) but the transitional
> helpers are still being used.
>
> I've found if you do (5) and (6) in one go, it becomes quite a large
> set of changes:
>
>  drivers/gpu/drm/armada/armada_crtc.c    | 308 +-------------------------------
>  drivers/gpu/drm/armada/armada_crtc.h    |  20 ---
>  drivers/gpu/drm/armada/armada_overlay.c | 175 ++++--------------
>  drivers/gpu/drm/armada/armada_plane.c   |   4 +-
>  4 files changed, 36 insertions(+), 471 deletions(-)

Hm yeah that doesn't work. I'd say that's perhaps cleanup work for
someone bored after armada atomic has landed.

I also think that after armada it probably makes sense to drop the
transitional helpers. Anyone else trying to do a legacy2atomic
conversion can dig them out of the git history and just carry them
around in their driver for the transition. That also avoids the
constant headaches of them breaking accidentally all the time.
-Daniel

>> It's all the same cargo-cult though in the plane->destroy hook. What
>> drivers should do instead is:
>> - Shut down the hw before cleaning up their modeset objects using
>>   something like drm_atomic_helper_shutdown().
>
> Hmm, that's something else I've missed in my conversion... thanks for
> pointing it out. ;)
>
>> - Remove the drm_plane_helper_disable().
>>
>> But for your patch to add the ctx argument everywhere I think just passing
>> NULL is ok too. It's not going to make things worse :-) Since that patch
>> needs to touch a bunch of drivers probably best if we get it landed
>> through drm-misc-next.
>
> Ok, I've a commit for that - I just need to merge it with the one I
> already have adding the ctx argument, grab drm-misc-next and make sure
> it applies there... I'll try to get it out in shortly.
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel