Re: [PATCH 5/7] drm/plane-helper: Export individual helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 11, 2022 at 08:32:19PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.08.22 um 18:41 schrieb Daniel Vetter:
> > On Wed, Jul 20, 2022 at 10:30:56AM +0200, Thomas Zimmermann wrote:
> > > Export the individual plane helpers that make up the plane functions and
> > > align the naming with other helpers. The plane helpers are for non-atomic
> > > modesetting and exporting them will simplify a later conversion of drivers
> > > to atomic modesetting.
> > > 
> > > With struct drm_plane_funcs removed from drm_plane_helper.h, also remove
> > > the include statements. It only needs linux/types.h for uint32_t and a
> > > number of forward declarations.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > 
> > So my commit message was maybe not super motivated, but I intentionally
> > hid these because atomic drivers where using them, which is all bad no
> > good. Now they're more exposed again, which sucks a bit. Exporting the
> > complete function table for legacy helpers (and the one open-coded one in
> > nouveau, not sure we could not move that back to drm_crtc_init) feels much
> > safer against abuse to me.
> > 
> > I'm not entirely clear why we're doing this, because the include untangle
> > could also have been achieved with a struct forward decl, which is what
> > we're usually doing. Can we walk this back please, or am I missing
> > something here?
> 
> I don't think you miss anything. It's just ugly to export the complete funcs
> table. If we roll that change back, let's add a comment that states the
> rational you wrote here.

So I'm way behind on everything, but maybe another option is to just check
in these functions that it's not an atomic modeset driver (i.e.
drm_drv_uses_atomic_modeset()). Or we just hope driver authors are smarter
now and there wont be fallout.

I'm also not clear on why exporting the entire vfunc table is a bad idea?

Either way I'd say up to you.
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 +-
> > >   drivers/gpu/drm/armada/armada_plane.c         |  2 +-
> > >   drivers/gpu/drm/drm_modeset_helper.c          |  8 ++-
> > >   drivers/gpu/drm/drm_plane_helper.c            | 70 ++++++++++++++-----
> > >   drivers/gpu/drm/nouveau/dispnv04/crtc.c       |  8 ++-
> > >   drivers/gpu/drm/qxl/qxl_display.c             |  4 +-
> > >   drivers/gpu/drm/vboxvideo/vbox_mode.c         |  4 +-
> > >   include/drm/drm_plane_helper.h                | 22 ++++--
> > >   8 files changed, 88 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > index 3e83fed540e8..1548f0a1b1c0 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -86,6 +86,7 @@
> > >   #include <drm/drm_vblank.h>
> > >   #include <drm/drm_audio_component.h>
> > >   #include <drm/drm_gem_atomic_helper.h>
> > > +#include <drm/drm_plane_helper.h>
> > >   #include "ivsrcid/dcn/irqsrcs_dcn_1_0.h"
> > > @@ -7824,7 +7825,7 @@ static void dm_drm_plane_destroy_state(struct drm_plane *plane,
> > >   static const struct drm_plane_funcs dm_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	.reset = dm_drm_plane_reset,
> > >   	.atomic_duplicate_state = dm_drm_plane_duplicate_state,
> > >   	.atomic_destroy_state = dm_drm_plane_destroy_state,
> > > diff --git a/drivers/gpu/drm/armada/armada_plane.c b/drivers/gpu/drm/armada/armada_plane.c
> > > index 959d7f0a5108..cc47c032dbc1 100644
> > > --- a/drivers/gpu/drm/armada/armada_plane.c
> > > +++ b/drivers/gpu/drm/armada/armada_plane.c
> > > @@ -288,7 +288,7 @@ struct drm_plane_state *armada_plane_duplicate_state(struct drm_plane *plane)
> > >   static const struct drm_plane_funcs armada_primary_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	.reset		= armada_plane_reset,
> > >   	.atomic_duplicate_state = armada_plane_duplicate_state,
> > >   	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> > > index 0f08319453b2..bd609a978848 100644
> > > --- a/drivers/gpu/drm/drm_modeset_helper.c
> > > +++ b/drivers/gpu/drm/drm_modeset_helper.c
> > > @@ -108,6 +108,12 @@ static const uint32_t safe_modeset_formats[] = {
> > >   	DRM_FORMAT_ARGB8888,
> > >   };
> > > +static const struct drm_plane_funcs primary_plane_funcs = {
> > > +	.update_plane = drm_plane_helper_update_primary,
> > > +	.disable_plane = drm_plane_helper_disable_primary,
> > > +	.destroy = drm_plane_helper_destroy,
> > > +};
> > > +
> > >   static struct drm_plane *create_primary_plane(struct drm_device *dev)
> > >   {
> > >   	struct drm_plane *primary;
> > > @@ -127,7 +133,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
> > >   	/* possible_crtc's will be filled in later by crtc_init */
> > >   	ret = drm_universal_plane_init(dev, primary, 0,
> > > -				       &drm_primary_helper_funcs,
> > > +				       &primary_plane_funcs,
> > >   				       safe_modeset_formats,
> > >   				       ARRAY_SIZE(safe_modeset_formats),
> > >   				       NULL,
> > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > > index bc08edd69030..c7785967f5bf 100644
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -145,13 +145,36 @@ static int drm_plane_helper_check_update(struct drm_plane *plane,
> > >   	return 0;
> > >   }
> > > -static int drm_primary_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,
> > > -				     struct drm_modeset_acquire_ctx *ctx)
> > > +/**
> > > + * drm_plane_helper_update_primary - Helper for updating primary planes
> > > + * @plane: plane to update
> > > + * @crtc: the plane's new CRTC
> > > + * @fb: the plane's new framebuffer
> > > + * @crtc_x: x coordinate within CRTC
> > > + * @crtc_y: y coordinate within CRTC
> > > + * @crtc_w: width coordinate within CRTC
> > > + * @crtc_h: height coordinate within CRTC
> > > + * @src_x: x coordinate within source
> > > + * @src_y: y coordinate within source
> > > + * @src_w: width coordinate within source
> > > + * @src_h: height coordinate within source
> > > + * @ctx: modeset locking context
> > > + *
> > > + * This helper validates the given parameters and updates the primary plane.
> > > + *
> > > + * This function is only useful for non-atomic modesetting. Don't use
> > > + * it in new drivers.
> > > + *
> > > + * Returns:
> > > + * Zero on success, or an errno code otherwise.
> > > + */
> > > +int drm_plane_helper_update_primary(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)
> > >   {
> > >   	struct drm_mode_set set = {
> > >   		.crtc = crtc,
> > > @@ -218,31 +241,40 @@ static int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *c
> > >   	kfree(connector_list);
> > >   	return ret;
> > >   }
> > > +EXPORT_SYMBOL(drm_plane_helper_update_primary);
> > > -static int drm_primary_helper_disable(struct drm_plane *plane,
> > > -				      struct drm_modeset_acquire_ctx *ctx)
> > > +/**
> > > + * drm_plane_helper_disable_primary - Helper for disabling primary planes
> > > + * @plane: plane to disable
> > > + * @ctx: modeset locking context
> > > + *
> > > + * This helper returns an error when trying to disable the primary
> > > + * plane.
> > > + *
> > > + * This function is only useful for non-atomic modesetting. Don't use
> > > + * it in new drivers.
> > > + *
> > > + * Returns:
> > > + * An errno code.
> > > + */
> > > +int drm_plane_helper_disable_primary(struct drm_plane *plane,
> > > +				     struct drm_modeset_acquire_ctx *ctx)
> > >   {
> > >   	return -EINVAL;
> > >   }
> > > +EXPORT_SYMBOL(drm_plane_helper_disable_primary);
> > >   /**
> > > - * drm_primary_helper_destroy() - Helper for primary plane destruction
> > > + * drm_plane_helper_destroy() - Helper for primary plane destruction
> > >    * @plane: plane to destroy
> > >    *
> > >    * Provides a default plane destroy handler for primary planes.  This handler
> > >    * is called during CRTC destruction.  We disable the primary plane, remove
> > >    * it from the DRM plane list, and deallocate the plane structure.
> > >    */
> > > -void drm_primary_helper_destroy(struct drm_plane *plane)
> > > +void drm_plane_helper_destroy(struct drm_plane *plane)
> > >   {
> > >   	drm_plane_cleanup(plane);
> > >   	kfree(plane);
> > >   }
> > > -EXPORT_SYMBOL(drm_primary_helper_destroy);
> > > -
> > > -const struct drm_plane_funcs drm_primary_helper_funcs = {
> > > -	.update_plane = drm_primary_helper_update,
> > > -	.disable_plane = drm_primary_helper_disable,
> > > -	.destroy = drm_primary_helper_destroy,
> > > -};
> > > -EXPORT_SYMBOL(drm_primary_helper_funcs);
> > > +EXPORT_SYMBOL(drm_plane_helper_destroy);
> > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> > > index f9e962fd94d0..660c4cbc0b3d 100644
> > > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> > > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> > > @@ -1275,6 +1275,12 @@ static const uint32_t modeset_formats[] = {
> > >           DRM_FORMAT_XRGB1555,
> > >   };
> > > +static const struct drm_plane_funcs nv04_primary_plane_funcs = {
> > > +	.update_plane = drm_plane_helper_update_primary,
> > > +	.disable_plane = drm_plane_helper_disable_primary,
> > > +	.destroy = drm_plane_helper_destroy,
> > > +};
> > > +
> > >   static struct drm_plane *
> > >   create_primary_plane(struct drm_device *dev)
> > >   {
> > > @@ -1289,7 +1295,7 @@ create_primary_plane(struct drm_device *dev)
> > >           /* possible_crtc's will be filled in later by crtc_init */
> > >           ret = drm_universal_plane_init(dev, primary, 0,
> > > -                                       &drm_primary_helper_funcs,
> > > +				       &nv04_primary_plane_funcs,
> > >                                          modeset_formats,
> > >                                          ARRAY_SIZE(modeset_formats), NULL,
> > >                                          DRM_PLANE_TYPE_PRIMARY, NULL);
> > > diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> > > index 2e8949863d6b..a152a7c6db21 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > > @@ -902,7 +902,7 @@ static const struct drm_plane_helper_funcs qxl_cursor_helper_funcs = {
> > >   static const struct drm_plane_funcs qxl_cursor_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	.reset		= drm_atomic_helper_plane_reset,
> > >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >   	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > @@ -924,7 +924,7 @@ static const struct drm_plane_helper_funcs primary_helper_funcs = {
> > >   static const struct drm_plane_funcs qxl_primary_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	.reset		= drm_atomic_helper_plane_reset,
> > >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >   	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > > index 604ebfbef314..341edd982cb3 100644
> > > --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > > +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> > > @@ -477,7 +477,7 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
> > >   static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	DRM_GEM_SHADOW_PLANE_FUNCS,
> > >   };
> > > @@ -496,7 +496,7 @@ static const struct drm_plane_helper_funcs vbox_primary_helper_funcs = {
> > >   static const struct drm_plane_funcs vbox_primary_plane_funcs = {
> > >   	.update_plane	= drm_atomic_helper_update_plane,
> > >   	.disable_plane	= drm_atomic_helper_disable_plane,
> > > -	.destroy	= drm_primary_helper_destroy,
> > > +	.destroy	= drm_plane_helper_destroy,
> > >   	.reset		= drm_atomic_helper_plane_reset,
> > >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >   	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> > > index ff85ef41cb33..1781fab24dd6 100644
> > > --- a/include/drm/drm_plane_helper.h
> > > +++ b/include/drm/drm_plane_helper.h
> > > @@ -24,12 +24,22 @@
> > >   #ifndef DRM_PLANE_HELPER_H
> > >   #define DRM_PLANE_HELPER_H
> > > -#include <drm/drm_rect.h>
> > > -#include <drm/drm_crtc.h>
> > > -#include <drm/drm_modeset_helper_vtables.h>
> > > -#include <drm/drm_modeset_helper.h>
> > > +#include <linux/types.h>
> > > -void drm_primary_helper_destroy(struct drm_plane *plane);
> > > -extern const struct drm_plane_funcs drm_primary_helper_funcs;
> > > +struct drm_crtc;
> > > +struct drm_framebuffer;
> > > +struct drm_modeset_acquire_ctx;
> > > +struct drm_plane;
> > > +
> > > +int drm_plane_helper_update_primary(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 drm_plane_helper_disable_primary(struct drm_plane *plane,
> > > +				     struct drm_modeset_acquire_ctx *ctx);
> > > +void drm_plane_helper_destroy(struct drm_plane *plane);
> > >   #endif
> > > -- 
> > > 2.36.1
> > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux