[RFC PATCH v4 23/42] drm/vkms: add 3x4 matrix in color pipeline

[Pekka Paalanen]

On Mon, 26 Feb 2024 16:10:37 -0500
Harry Wentland <harry.wentland at amd.com> wrote:

> We add two 3x4 matrices into the VKMS color pipeline. The reason
> we're adding matrices is so that we can test that application
> of a matrix and its inverse yields an output equal to the input
> image.

You will test also cases where the matrix configuration will not
produce output equal to input, right?

Otherwise one can accidentally pass the matrix test by ignoring all
matrices.

> One complication with the matrix implementation has to do with
> the fact that the matrix entries are in signed-magnitude fixed
> point, whereas the drm_fixed.h implementation uses 2s-complement.
> The latter one is the one that we want for easy addition and
> subtraction, so we convert all entries to 2s-complement.
> 
> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
> ---
>  drivers/gpu/drm/vkms/vkms_colorop.c  | 32 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/vkms/vkms_composer.c | 27 +++++++++++++++++++++++
>  include/drm/drm_colorop.h            |  2 ++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
> index d2db366da6d3..a0e54b2c1f7a 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -35,7 +35,37 @@ const int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pro
>  
>  	prev_op = op;
>  
> -	/* 2nd op: 1d curve */
> +	/* 2nd op: 3x4 matrix */
> +	op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> +	if (!op) {
> +		DRM_ERROR("KMS: Failed to allocate colorop\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = drm_colorop_ctm_3x4_init(dev, op, plane);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_set_next_property(prev_op, op);
> +
> +	prev_op = op;
> +
> +	/* 3rd op: 3x4 matrix */
> +	op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> +	if (!op) {
> +		DRM_ERROR("KMS: Failed to allocate colorop\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = drm_colorop_ctm_3x4_init(dev, op, plane);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_set_next_property(prev_op, op);
> +
> +	prev_op = op;
> +
> +	/* 4th op: 1d curve */
>  	op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
>  	if (!op) {
>  		DRM_ERROR("KMS: Failed to allocate colorop\n");
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index d2101fa55aa3..8bbfce651526 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -164,6 +164,30 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
>  	}
>  }
>  
> +static void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct drm_color_ctm_3x4 *matrix)
> +{
> +	s64 rf, gf, bf;
> +
> +	rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]), drm_int2fixp(pixel->r)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]), drm_int2fixp(pixel->g)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]), drm_int2fixp(pixel->b)) +
> +	     drm_sm2fixp(matrix->matrix[3]);

It would be nice if the driver had its private data for the matrix
colorop state, so it could convert the matrix only once when userspace
sets it.


Thanks,
pq

> +
> +	gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]), drm_int2fixp(pixel->r)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]), drm_int2fixp(pixel->g)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]), drm_int2fixp(pixel->b)) +
> +	     drm_sm2fixp(matrix->matrix[7]);
> +
> +	bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]), drm_int2fixp(pixel->r)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]), drm_int2fixp(pixel->g)) +
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]), drm_int2fixp(pixel->b)) +
> +	     drm_sm2fixp(matrix->matrix[11]);
> +
> +	pixel->r = drm_fixp2int(rf);
> +	pixel->g = drm_fixp2int(gf);
> +	pixel->b = drm_fixp2int(bf);
> +}
> +
>  static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colorop)
>  {
>  	/* TODO is this right? */
> @@ -185,6 +209,9 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>  				DRM_DEBUG_DRIVER("unkown colorop 1D curve type %d\n", colorop_state->curve_1d_type);
>  				break;
>  		}
> +	} else if (colorop->type == DRM_COLOROP_CTM_3X4) {
> +		if (colorop_state->data)
> +			apply_3x4_matrix(pixel, (struct drm_color_ctm_3x4 *) colorop_state->data->data);
>  	}
>  
>  }
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index 4aee29e161d6..8710e550790c 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -224,6 +224,8 @@ int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
>  
>  int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop *colorop,
>  			      struct drm_plane *plane, u64 supported_tfs);
> +int drm_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop *colorop,
> +			     struct drm_plane *plane);
>  
>  struct drm_colorop_state *
>  drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop);

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20240314/c14c95a1/attachment-0001.sig>