[18/21] drm: Unexport primary plane helpers
diff mbox series

Message ID 20181004202446.22905-19-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • atomic helper cleanup, take 2
Related show

Commit Message

Daniel Vetter Oct. 4, 2018, 8:24 p.m. UTC
Well except the destroy helper, which isn't really a primary helper
but generally useful, if mislabelled.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 85 ++++--------------------------
 include/drm/drm_plane_helper.h     | 10 ----
 2 files changed, 11 insertions(+), 84 deletions(-)

Comments

Sam Ravnborg Oct. 4, 2018, 9:14 p.m. UTC | #1
Hi Daniel.

On Thu, Oct 04, 2018 at 10:24:43PM +0200, Daniel Vetter wrote:
> Well except the destroy helper, which isn't really a primary helper
> but generally useful, if mislabelled.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 85 ++++--------------------------
>  include/drm/drm_plane_helper.h     | 10 ----
>  2 files changed, 11 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 33b3e6892787..0fff72dcd06d 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -42,11 +42,8 @@
>   * primary plane support on top of the normal CRTC configuration interface.
>   * Since the legacy &drm_mode_config_funcs.set_config interface ties the primary
>   * plane together with the CRTC state this does not allow userspace to disable
> - * the primary plane itself.  To avoid too much duplicated code use
> - * drm_plane_helper_check_update() which can be used to enforce the same
> - * restrictions as primary planes had thus. The default primary plane only
> - * expose XRBG8888 and ARGB8888 as valid pixel formats for the attached
> - * framebuffer.
> + * the primary plane itself. The default primary plane only expose XRBG8888 and
> + * ARGB8888 as valid pixel formats for the attached framebuffer.
>   *
>   * Drivers are highly recommended to implement proper support for primary
>   * planes, and newly merged drivers must not rely upon these transitional
> @@ -148,50 +145,13 @@ static int drm_plane_helper_check_update(struct drm_plane *plane,
>  	return 0;
>  }
>  
> -/**
> - * drm_primary_helper_update() - Helper for primary plane update
> - * @plane: plane object to update
> - * @crtc: owning CRTC of owning plane
> - * @fb: framebuffer to flip onto plane
> - * @crtc_x: x offset of primary plane on crtc
> - * @crtc_y: y offset of primary plane on crtc
> - * @crtc_w: width of primary plane rectangle on crtc
> - * @crtc_h: height of primary plane rectangle on crtc
> - * @src_x: x offset of @fb for panning
> - * @src_y: y offset of @fb for panning
> - * @src_w: width of source rectangle in @fb
> - * @src_h: height of source rectangle in @fb
> - * @ctx: lock acquire context, not used here
> - *
> - * Provides a default plane update handler for primary planes.  This is handler
> - * is called in response to a userspace SetPlane operation on the plane with a
> - * non-NULL framebuffer.  We call the driver's modeset handler to update the
> - * framebuffer.
> - *
> - * SetPlane() on a primary plane of a disabled CRTC is not supported, and will
> - * return an error.
> - *
> - * Note that we make some assumptions about hardware limitations that may not be
> - * true for all hardware --
> - *
> - * 1. Primary plane cannot be repositioned.
> - * 2. Primary plane cannot be scaled.
> - * 3. Primary plane must cover the entire CRTC.
> - * 4. Subpixel positioning is not supported.
> - *
> - * Drivers for hardware that don't have these restrictions can provide their
> - * own implementation rather than using this helper.
> - *
> - * RETURNS:
> - * Zero on success, error code on failure
> - */
> -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)
> +static int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,

This looks like a useful comment explaining the purpose
of a function get lost.


> +				     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,
> @@ -258,35 +218,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  	kfree(connector_list);
>  	return ret;
>  }
> -EXPORT_SYMBOL(drm_primary_helper_update);
>  
> -/**
> - * drm_primary_helper_disable() - Helper for primary plane disable
> - * @plane: plane to disable
> - * @ctx: lock acquire context, not used here
> - *
> - * Provides a default plane disable handler for primary planes.  This is handler
> - * is called in response to a userspace SetPlane operation on the plane with a
> - * NULL framebuffer parameter.  It unconditionally fails the disable call with
> - * -EINVAL the only way to disable the primary plane without driver support is
> - * to disable the entire CRTC. Which does not match the plane
> - * &drm_plane_funcs.disable_plane hook.
> - *
> - * Note that some hardware may be able to disable the primary plane without
> - * disabling the whole CRTC.  Drivers for such hardware should provide their
> - * own disable handler that disables just the primary plane (and they'll likely
> - * need to provide their own update handler as well to properly re-enable a
> - * disabled primary plane).
> - *
> - * RETURNS:
> - * Unconditionally returns -EINVAL.
> - */
> -int drm_primary_helper_disable(struct drm_plane *plane,
> -			       struct drm_modeset_acquire_ctx *ctx)
> +static int drm_primary_helper_disable(struct drm_plane *plane,
> +				      struct drm_modeset_acquire_ctx *ctx)
>  {
>  	return -EINVAL;
>  }
Likewise, good comment deleted. But then in this case this is also
nicely commented where disable_plane is called.
The single use of disable_plane could also just check for NULL,
that would be more straightforward and then this functions could be deleted.

	Sam
Daniel Vetter Oct. 5, 2018, 6:18 a.m. UTC | #2
On Thu, Oct 4, 2018 at 11:14 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Daniel.
>
> On Thu, Oct 04, 2018 at 10:24:43PM +0200, Daniel Vetter wrote:
> > Well except the destroy helper, which isn't really a primary helper
> > but generally useful, if mislabelled.
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane_helper.c | 85 ++++--------------------------
> >  include/drm/drm_plane_helper.h     | 10 ----
> >  2 files changed, 11 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > index 33b3e6892787..0fff72dcd06d 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -42,11 +42,8 @@
> >   * primary plane support on top of the normal CRTC configuration interface.
> >   * Since the legacy &drm_mode_config_funcs.set_config interface ties the primary
> >   * plane together with the CRTC state this does not allow userspace to disable
> > - * the primary plane itself.  To avoid too much duplicated code use
> > - * drm_plane_helper_check_update() which can be used to enforce the same
> > - * restrictions as primary planes had thus. The default primary plane only
> > - * expose XRBG8888 and ARGB8888 as valid pixel formats for the attached
> > - * framebuffer.
> > + * the primary plane itself. The default primary plane only expose XRBG8888 and
> > + * ARGB8888 as valid pixel formats for the attached framebuffer.
> >   *
> >   * Drivers are highly recommended to implement proper support for primary
> >   * planes, and newly merged drivers must not rely upon these transitional
> > @@ -148,50 +145,13 @@ static int drm_plane_helper_check_update(struct drm_plane *plane,
> >       return 0;
> >  }
> >
> > -/**
> > - * drm_primary_helper_update() - Helper for primary plane update
> > - * @plane: plane object to update
> > - * @crtc: owning CRTC of owning plane
> > - * @fb: framebuffer to flip onto plane
> > - * @crtc_x: x offset of primary plane on crtc
> > - * @crtc_y: y offset of primary plane on crtc
> > - * @crtc_w: width of primary plane rectangle on crtc
> > - * @crtc_h: height of primary plane rectangle on crtc
> > - * @src_x: x offset of @fb for panning
> > - * @src_y: y offset of @fb for panning
> > - * @src_w: width of source rectangle in @fb
> > - * @src_h: height of source rectangle in @fb
> > - * @ctx: lock acquire context, not used here
> > - *
> > - * Provides a default plane update handler for primary planes.  This is handler
> > - * is called in response to a userspace SetPlane operation on the plane with a
> > - * non-NULL framebuffer.  We call the driver's modeset handler to update the
> > - * framebuffer.
> > - *
> > - * SetPlane() on a primary plane of a disabled CRTC is not supported, and will
> > - * return an error.
> > - *
> > - * Note that we make some assumptions about hardware limitations that may not be
> > - * true for all hardware --
> > - *
> > - * 1. Primary plane cannot be repositioned.
> > - * 2. Primary plane cannot be scaled.
> > - * 3. Primary plane must cover the entire CRTC.
> > - * 4. Subpixel positioning is not supported.
> > - *
> > - * Drivers for hardware that don't have these restrictions can provide their
> > - * own implementation rather than using this helper.
> > - *
> > - * RETURNS:
> > - * Zero on success, error code on failure
> > - */
> > -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)
> > +static int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>
> This looks like a useful comment explaining the purpose
> of a function get lost.

Hm, good point, some of this could be moved into drm_crtc_init().
Maybe with a stern warning that you really don't want this for a new
driver. The current kernel-doc there is rather sparse.

> > +                                  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,
> > @@ -258,35 +218,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> >       kfree(connector_list);
> >       return ret;
> >  }
> > -EXPORT_SYMBOL(drm_primary_helper_update);
> >
> > -/**
> > - * drm_primary_helper_disable() - Helper for primary plane disable
> > - * @plane: plane to disable
> > - * @ctx: lock acquire context, not used here
> > - *
> > - * Provides a default plane disable handler for primary planes.  This is handler
> > - * is called in response to a userspace SetPlane operation on the plane with a
> > - * NULL framebuffer parameter.  It unconditionally fails the disable call with
> > - * -EINVAL the only way to disable the primary plane without driver support is
> > - * to disable the entire CRTC. Which does not match the plane
> > - * &drm_plane_funcs.disable_plane hook.
> > - *
> > - * Note that some hardware may be able to disable the primary plane without
> > - * disabling the whole CRTC.  Drivers for such hardware should provide their
> > - * own disable handler that disables just the primary plane (and they'll likely
> > - * need to provide their own update handler as well to properly re-enable a
> > - * disabled primary plane).
> > - *
> > - * RETURNS:
> > - * Unconditionally returns -EINVAL.
> > - */
> > -int drm_primary_helper_disable(struct drm_plane *plane,
> > -                            struct drm_modeset_acquire_ctx *ctx)
> > +static int drm_primary_helper_disable(struct drm_plane *plane,
> > +                                   struct drm_modeset_acquire_ctx *ctx)
> >  {
> >       return -EINVAL;
> >  }
> Likewise, good comment deleted. But then in this case this is also
> nicely commented where disable_plane is called.
> The single use of disable_plane could also just check for NULL,
> that would be more straightforward and then this functions could be deleted.

Returning -EINVAL unconditionally is really not what any new or modern
driver should ever do. Atomic drivers need to hook up
drm_atomic_helper_plane_disable(), legacy drivers who care about
planes also really want their own implementation here. Imo { return
-EINVAL; } is the worst choice of a default we can pick, since it
would mislead a lot of drivers into doing the wrong thing.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 33b3e6892787..0fff72dcd06d 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -42,11 +42,8 @@ 
  * primary plane support on top of the normal CRTC configuration interface.
  * Since the legacy &drm_mode_config_funcs.set_config interface ties the primary
  * plane together with the CRTC state this does not allow userspace to disable
- * the primary plane itself.  To avoid too much duplicated code use
- * drm_plane_helper_check_update() which can be used to enforce the same
- * restrictions as primary planes had thus. The default primary plane only
- * expose XRBG8888 and ARGB8888 as valid pixel formats for the attached
- * framebuffer.
+ * the primary plane itself. The default primary plane only expose XRBG8888 and
+ * ARGB8888 as valid pixel formats for the attached framebuffer.
  *
  * Drivers are highly recommended to implement proper support for primary
  * planes, and newly merged drivers must not rely upon these transitional
@@ -148,50 +145,13 @@  static int drm_plane_helper_check_update(struct drm_plane *plane,
 	return 0;
 }
 
-/**
- * drm_primary_helper_update() - Helper for primary plane update
- * @plane: plane object to update
- * @crtc: owning CRTC of owning plane
- * @fb: framebuffer to flip onto plane
- * @crtc_x: x offset of primary plane on crtc
- * @crtc_y: y offset of primary plane on crtc
- * @crtc_w: width of primary plane rectangle on crtc
- * @crtc_h: height of primary plane rectangle on crtc
- * @src_x: x offset of @fb for panning
- * @src_y: y offset of @fb for panning
- * @src_w: width of source rectangle in @fb
- * @src_h: height of source rectangle in @fb
- * @ctx: lock acquire context, not used here
- *
- * Provides a default plane update handler for primary planes.  This is handler
- * is called in response to a userspace SetPlane operation on the plane with a
- * non-NULL framebuffer.  We call the driver's modeset handler to update the
- * framebuffer.
- *
- * SetPlane() on a primary plane of a disabled CRTC is not supported, and will
- * return an error.
- *
- * Note that we make some assumptions about hardware limitations that may not be
- * true for all hardware --
- *
- * 1. Primary plane cannot be repositioned.
- * 2. Primary plane cannot be scaled.
- * 3. Primary plane must cover the entire CRTC.
- * 4. Subpixel positioning is not supported.
- *
- * Drivers for hardware that don't have these restrictions can provide their
- * own implementation rather than using this helper.
- *
- * RETURNS:
- * Zero on success, error code on failure
- */
-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)
+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)
 {
 	struct drm_mode_set set = {
 		.crtc = crtc,
@@ -258,35 +218,12 @@  int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	kfree(connector_list);
 	return ret;
 }
-EXPORT_SYMBOL(drm_primary_helper_update);
 
-/**
- * drm_primary_helper_disable() - Helper for primary plane disable
- * @plane: plane to disable
- * @ctx: lock acquire context, not used here
- *
- * Provides a default plane disable handler for primary planes.  This is handler
- * is called in response to a userspace SetPlane operation on the plane with a
- * NULL framebuffer parameter.  It unconditionally fails the disable call with
- * -EINVAL the only way to disable the primary plane without driver support is
- * to disable the entire CRTC. Which does not match the plane
- * &drm_plane_funcs.disable_plane hook.
- *
- * Note that some hardware may be able to disable the primary plane without
- * disabling the whole CRTC.  Drivers for such hardware should provide their
- * own disable handler that disables just the primary plane (and they'll likely
- * need to provide their own update handler as well to properly re-enable a
- * disabled primary plane).
- *
- * RETURNS:
- * Unconditionally returns -EINVAL.
- */
-int drm_primary_helper_disable(struct drm_plane *plane,
-			       struct drm_modeset_acquire_ctx *ctx)
+static int drm_primary_helper_disable(struct drm_plane *plane,
+				      struct drm_modeset_acquire_ctx *ctx)
 {
 	return -EINVAL;
 }
-EXPORT_SYMBOL(drm_primary_helper_disable);
 
 /**
  * drm_primary_helper_destroy() - Helper for primary plane destruction
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 7bff930b8b10..331ebd60b3a3 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -38,16 +38,6 @@ 
  */
 #define DRM_PLANE_HELPER_NO_SCALING (1<<16)
 
-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);
-int drm_primary_helper_disable(struct drm_plane *plane,
-			       struct drm_modeset_acquire_ctx *ctx);
 void drm_primary_helper_destroy(struct drm_plane *plane);
 extern const struct drm_plane_funcs drm_primary_helper_funcs;