Re: [RFC PATCH v3 13/23] drm/plane: Add COLOR PIPELINE property

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8160 bytes --]

On Wed, 8 Nov 2023 11:36:32 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> We're adding a new enum COLOR PIPELINE property. This
> property will have entries for each COLOR PIPELINE by
> referencing the DRM object ID of the first drm_colorop
> of the pipeline. 0 disables the entire COLOR PIPELINE.

I didn't find the call that actually creates that property, where is it?


> Userspace can use this to discover the available color
> pipelines, as well as set the desired one. The color
> pipelines are programmed via properties on the actual
> drm_colorop objects.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic.c              | 46 +++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++
>  drivers/gpu/drm/drm_atomic_uapi.c         | 44 ++++++++++++++++++++++
>  include/drm/drm_atomic_uapi.h             |  2 +
>  include/drm/drm_plane.h                   |  8 ++++
>  5 files changed, 105 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ccf26b034433..cf3cb6d1239f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1463,6 +1463,52 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>  
> +/**
> + * drm_atomic_add_affected_colorops - add colorops for plane
> + * @state: atomic state
> + * @plane: DRM plane
> + *
> + * This function walks the current configuration and adds all colorops
> + * currently used by @plane to the atomic configuration @state. This is useful
> + * when an atomic commit also needs to check all currently enabled colorop on
> + * @plane, e.g. when changing the mode. It's also useful when re-enabling a plane
> + * to avoid special code to force-enable all colorops.
> + *
> + * Since acquiring a colorop state will always also acquire the w/w mutex of the
> + * current plane for that colorop (if there is any) adding all the colorop states for
> + * a plane will not reduce parallelism of atomic updates.
> + *
> + * Returns:
> + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
> + * then the w/w mutex code has detected a deadlock and the entire atomic
> + * sequence must be restarted. All other errors are fatal.
> + */
> +int
> +drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
> +				 struct drm_plane *plane)
> +{
> +	struct drm_colorop *colorop;
> +	struct drm_colorop_state *colorop_state;
> +
> +	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
> +
> +	drm_dbg_atomic(plane->dev,
> +		       "Adding all current colorops for [plane:%d:%s] to %p\n",
> +		       plane->base.id, plane->name, state);
> +
> +	drm_for_each_colorop(colorop, plane->dev) {
> +		if (colorop->plane != plane)
> +			continue;
> +
> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);
> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_add_affected_colorops);
> +
>  /**
>   * drm_atomic_check_only - check whether a given config would work
>   * @state: atomic configuration to check
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..3c5f2c8e33d0 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -267,6 +267,11 @@ void __drm_atomic_helper_plane_state_reset(struct drm_plane_state *plane_state,
>  			plane_state->color_range = val;
>  	}
>  
> +	if (plane->color_pipeline_property) {
> +		/* default is always NULL, i.e., bypass */
> +		plane_state->color_pipeline = NULL;
> +	}
> +
>  	if (plane->zpos_property) {
>  		if (!drm_object_property_get_default_value(&plane->base,
>  							   plane->zpos_property,
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index a8f7a8a6639a..c6629fdaa114 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -256,6 +256,38 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  }
>  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>  
> +
> +/**
> + * drm_atomic_set_colorop_for_plane - set colorop for plane
> + * @plane_state: atomic state object for the plane
> + * @colorop: colorop to use for the plane
> + *
> + * Changing the assigned framebuffer for a plane requires us to grab a reference
> + * to the new fb and drop the reference to the old fb, if there is one. This
> + * function takes care of all these details besides updating the pointer in the
> + * state object itself.

This paragraph does not seem to talk about this function.

> + */
> +void
> +drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> +				 struct drm_colorop *colorop)
> +{
> +	struct drm_plane *plane = plane_state->plane;
> +
> +	if (colorop)
> +		drm_dbg_atomic(plane->dev,
> +			       "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
> +			       colorop->base.id, plane->base.id, plane->name,
> +			       plane_state);
> +	else
> +		drm_dbg_atomic(plane->dev,
> +			       "Set [NOCOLOROP] for [PLANE:%d:%s] state %p\n",
> +			       plane->base.id, plane->name, plane_state);
> +
> +	plane_state->color_pipeline = colorop;
> +}
> +EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
> +
> +
>  /**
>   * drm_atomic_set_crtc_for_connector - set CRTC for connector
>   * @conn_state: atomic state object for the connector
> @@ -581,6 +613,16 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->color_encoding = val;
>  	} else if (property == plane->color_range_property) {
>  		state->color_range = val;
> +	} else if (property == plane->color_pipeline_property) {
> +		/* find DRM colorop object */
> +		struct drm_colorop *colorop = NULL;
> +		colorop = drm_colorop_find(dev, file_priv, val);
> +
> +		if (val && !colorop)
> +			return -EACCES;
> +
> +		/* set it on drm_plane_state */
> +		drm_atomic_set_colorop_for_plane(state, colorop);
>  	} else if (property == config->prop_fb_damage_clips) {
>  		ret = drm_atomic_replace_property_blob_from_id(dev,
>  					&state->fb_damage_clips,
> @@ -647,6 +689,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->color_encoding;
>  	} else if (property == plane->color_range_property) {
>  		*val = state->color_range;
> +	} else if (property == plane->color_pipeline_property) {
> +		*val = (state->color_pipeline) ? state->color_pipeline->base.id : 0;
>  	} else if (property == config->prop_fb_damage_clips) {
>  		*val = (state->fb_damage_clips) ?
>  			state->fb_damage_clips->base.id : 0;
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 70a115d523cd..436315523326 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -50,6 +50,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  			      struct drm_crtc *crtc);
>  void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  				 struct drm_framebuffer *fb);
> +void drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> +				      struct drm_colorop *colorop);
>  int __must_check
>  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  				  struct drm_crtc *crtc);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 57bbd0cd73a9..e65074f266c0 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -745,6 +745,14 @@ struct drm_plane {
>  	 */
>  	struct drm_property *color_range_property;
>  
> +	/**
> +	 * @color_pipeline_property:
> +	 *
> +	 * Optional "COLOR_PIPELINE" enum property for specifying
> +	 * a color pipeline to use on the plane.
> +	 */
> +	struct drm_property *color_pipeline_property;
> +
>  	/**
>  	 * @scaling_filter_property: property to apply a particular filter while
>  	 * scaling.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]