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.syrj...@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
