Re: [PATCH RFC 3/5] drm/mediatek: Support post-blend colorops for gamma and ctm

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 22/08/2025 à 20:36, Nícolas F. R. A. Prado a écrit :
> Allow configuring the gamma and ccorr blocks through the post-blend
> color pipeline API instead of the GAMMA_LUT and CTM properties.
> 
> In order to achieve this, initialize the color pipeline property and
> colorops on the CRTC based on the DDP components available in the CRTC
> path. Then introduce a struct mtk_drm_colorop that extends drm_colorop
> and tracks the mtk_ddp_comp that implements it in hardware, and include
> new ddp_comp helper functions for setting gamma and ctm through the new
> API. These helpers will then be called during commit flush for every
> updated colorop if the DRM client supports the post-blend color pipeline
> API.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_crtc.c     | 211 +++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/mediatek/mtk_ddp_comp.h |   2 +
>   2 files changed, 208 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index bc7527542fdc6fb89fc36794cee7d6dc26f7dcce..80ed061de1af31916d814f29f9111973cffd10dd 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -82,6 +82,12 @@ struct mtk_crtc_state {
>   	unsigned int			pending_vrefresh;
>   };
>   
> +struct mtk_drm_colorop {
> +	struct drm_colorop colorop;
> +	struct mtk_ddp_comp *comp;
> +	uint32_t data_id;
> +};
> +

@Alex: This is exactly a case where I think the current 
drm_colorop_pipeline_destroy will break.

>   static inline struct mtk_crtc *to_mtk_crtc(struct drm_crtc *c)
>   {
>   	return container_of(c, struct mtk_crtc, base);
> @@ -92,6 +98,11 @@ static inline struct mtk_crtc_state *to_mtk_crtc_state(struct drm_crtc_state *s)
>   	return container_of(s, struct mtk_crtc_state, base);
>   }
>   
> +static inline struct mtk_drm_colorop *to_mtk_colorop(struct drm_colorop *colorop)
> +{
> +	return container_of(colorop, struct mtk_drm_colorop, colorop);
> +}
> +
>   static void mtk_crtc_finish_page_flip(struct mtk_crtc *mtk_crtc)
>   {
>   	struct drm_crtc *crtc = &mtk_crtc->base;
> @@ -125,6 +136,19 @@ static void mtk_drm_finish_page_flip(struct mtk_crtc *mtk_crtc)
>   	spin_unlock_irqrestore(&mtk_crtc->config_lock, flags);
>   }
>   
> +static void mtk_drm_colorop_pipeline_destroy(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_colorop *colorop, *next;
> +	struct mtk_drm_colorop *mtk_colorop;
> +
> +	list_for_each_entry_safe(colorop, next, &config->colorop_list, head) {
> +		drm_colorop_cleanup(colorop);
> +		mtk_colorop = to_mtk_colorop(colorop);
> +		kfree(mtk_colorop);
> +	}
> +}
> +
>   static void mtk_crtc_destroy(struct drm_crtc *crtc)
>   {
>   	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
> @@ -146,6 +170,8 @@ static void mtk_crtc_destroy(struct drm_crtc *crtc)
>   		mtk_ddp_comp_unregister_vblank_cb(comp);
>   	}
>   
> +	mtk_drm_colorop_pipeline_destroy(crtc->dev);
> +
>   	drm_crtc_cleanup(crtc);
>   }
>   
> @@ -854,20 +880,103 @@ static void mtk_crtc_atomic_begin(struct drm_crtc *crtc,
>   	}
>   }
>   
> +static bool colorop_data_update_flush_status(struct drm_colorop_state *colorop_state)
> +{
> +	struct drm_colorop *colorop = colorop_state->colorop;
> +	struct mtk_drm_colorop *mtk_colorop = to_mtk_colorop(colorop);
> +	struct drm_property_blob *data_blob = colorop_state->data;
> +	uint32_t data_id = colorop_state->bypass ? 0 : data_blob->base.id;
> +	bool needs_flush = mtk_colorop->data_id != data_id;
> +
> +	mtk_colorop->data_id = data_id;
> +
> +	return needs_flush;
> +}
> +
> +static void mtk_crtc_ddp_comp_apply_colorop(struct drm_colorop_state *colorop_state)
> +{
> +	struct drm_colorop *colorop = colorop_state->colorop;
> +	struct mtk_drm_colorop *mtk_colorop = to_mtk_colorop(colorop);
> +	struct drm_property_blob *data_blob = colorop_state->data;
> +	struct mtk_ddp_comp *ddp_comp = mtk_colorop->comp;
> +	struct drm_color_ctm_3x4 *ctm = NULL;
> +	struct drm_color_lut32 *lut = NULL;
> +
> +	switch (colorop->type) {
> +	case DRM_COLOROP_1D_LUT:
> +		if (!colorop_data_update_flush_status(colorop_state))
> +			return;
> +
> +		if (!colorop_state->bypass)
> +			lut = (struct drm_color_lut32 *)data_blob->data;
> +
> +		ddp_comp->funcs->gamma_set_color_pipeline(ddp_comp->dev, lut);
> +		break;
> +	case DRM_COLOROP_CTM_3X4:
> +		if (!colorop_data_update_flush_status(colorop_state))
> +			return;
> +
> +		if (!colorop_state->bypass)
> +			ctm = (struct drm_color_ctm_3x4 *)data_blob->data;
> +
> +		ddp_comp->funcs->ctm_set_color_pipeline(ddp_comp->dev, ctm);
> +		break;
> +	default:
> +		/* If this happens the driver is broken */
> +		drm_WARN(colorop->dev, 1,
> +			 "Trying to atomic flush COLOROP of type unsupported by driver: %d\n",
> +			 colorop->type);
> +		break;
> +	}
> +}
> +
>   static void mtk_crtc_atomic_flush(struct drm_crtc *crtc,
>   				  struct drm_atomic_state *state)
>   {
>   	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
> +	struct drm_colorop_state *new_colorop_state;
> +	struct drm_colorop *colorop;
>   	int i;
>   
> -	if (crtc->state->color_mgmt_changed)
> -		for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> -			mtk_ddp_gamma_set(mtk_crtc->ddp_comp[i], crtc->state);
> -			mtk_ddp_ctm_set(mtk_crtc->ddp_comp[i], crtc->state);
> -		}
> +	if (state->post_blend_color_pipeline) {
> +		for_each_new_colorop_in_state(state, colorop, new_colorop_state, i)
> +			mtk_crtc_ddp_comp_apply_colorop(new_colorop_state);
> +	} else {
> +		if (crtc->state->color_mgmt_changed)
> +			for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +				mtk_ddp_gamma_set(mtk_crtc->ddp_comp[i], crtc->state);
> +				mtk_ddp_ctm_set(mtk_crtc->ddp_comp[i], crtc->state);
> +			}
> +	}
>   	mtk_crtc_update_config(mtk_crtc, !!mtk_crtc->event);
>   }
>   
> +static int mtk_crtc_atomic_check(struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state)
> +{
> +	struct drm_colorop_state *new_colorop_state;
> +	struct drm_colorop *colorop;
> +	int i;
> +
> +	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> +		switch (colorop->type) {
> +		case DRM_COLOROP_1D_LUT:
> +		case DRM_COLOROP_CTM_3X4:
> +			if (!new_colorop_state->bypass && !new_colorop_state->data) {
> +				drm_dbg_atomic(crtc->dev,
> +					       "Missing required DATA property for COLOROP:%d\n",
> +					       colorop->base.id);
> +				return -EINVAL;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct drm_crtc_funcs mtk_crtc_funcs = {
>   	.set_config		= drm_atomic_helper_set_config,
>   	.page_flip		= drm_atomic_helper_page_flip,
> @@ -885,6 +994,7 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
>   	.mode_valid	= mtk_crtc_mode_valid,
>   	.atomic_begin	= mtk_crtc_atomic_begin,
>   	.atomic_flush	= mtk_crtc_atomic_flush,
> +	.atomic_check   = mtk_crtc_atomic_check,
>   	.atomic_enable	= mtk_crtc_atomic_enable,
>   	.atomic_disable	= mtk_crtc_atomic_disable,
>   };
> @@ -987,6 +1097,95 @@ struct device *mtk_crtc_dma_dev_get(struct drm_crtc *crtc)
>   	return mtk_crtc->dma_dev;
>   }
>   
> +#define MAX_COLOR_PIPELINE_OPS 2
> +#define MAX_COLOR_PIPELINES 1
> +
> +static int mtk_colorop_init(struct mtk_crtc *mtk_crtc,
> +			    struct mtk_drm_colorop *mtk_colorop,
> +			    struct mtk_ddp_comp *ddp_comp)
> +{
> +	struct drm_colorop *colorop = &mtk_colorop->colorop;
> +	int ret = 0;
> +
> +	if (ddp_comp->funcs->gamma_set_color_pipeline) {
> +		unsigned int lut_sz = mtk_ddp_gamma_get_lut_size(ddp_comp);
> +
> +		ret = drm_crtc_colorop_curve_1d_lut_init(mtk_crtc->base.dev, colorop,
> +							 &mtk_crtc->base,
> +							 lut_sz,
> +							 DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
> +							 DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +	} else if (ddp_comp->funcs->ctm_set_color_pipeline) {
> +		ret = drm_crtc_colorop_ctm_3x4_init(mtk_crtc->base.dev,
> +						    colorop,
> +						    &mtk_crtc->base,
> +						    DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +	}
> +
> +	mtk_colorop->comp = ddp_comp;
> +
> +	return ret;
> +}
> +
> +static int mtk_crtc_init_post_blend_color_pipeline(struct mtk_crtc *mtk_crtc,
> +						   unsigned int gamma_lut_size)
> +{
> +	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
> +	struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
> +	struct mtk_drm_colorop *mtk_colorop;
> +	unsigned int num_pipelines = 0;
> +	unsigned int op_idx = 0;
> +	int ret;
> +
> +	memset(ops, 0, sizeof(ops));
> +
> +	for (unsigned int i = 0;
> +	     i < mtk_crtc->ddp_comp_nr && op_idx < MAX_COLOR_PIPELINE_OPS;
> +	     i++) {
> +		struct mtk_ddp_comp *ddp_comp = mtk_crtc->ddp_comp[i];
> +
> +		if (!(ddp_comp->funcs->gamma_set_color_pipeline ||
> +		      ddp_comp->funcs->ctm_set_color_pipeline))
> +			continue;
> +
> +		mtk_colorop = kzalloc(sizeof(struct mtk_drm_colorop), GFP_KERNEL);
> +		if (!mtk_colorop) {
> +			ret = -ENOMEM;
> +			goto cleanup;
> +		}
> +
> +		ops[op_idx] = &mtk_colorop->colorop;
> +
> +		ret = mtk_colorop_init(mtk_crtc, mtk_colorop, ddp_comp);
> +		if (ret)
> +			goto cleanup;
> +
> +		if (op_idx > 0)
> +			drm_colorop_set_next_property(ops[op_idx-1], ops[op_idx]);
> +
> +		op_idx++;
> +	}
> +
> +	if (op_idx > 0) {
> +		pipelines[0].type = ops[0]->base.id;
> +		pipelines[0].name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);
> +		num_pipelines = 1;
> +	}
> +
> +	/* Create COLOR_PIPELINE property and attach */
> +	drm_crtc_create_color_pipeline_property(&mtk_crtc->base, pipelines, num_pipelines);
> +
> +	return 0;
> +
> +cleanup:
> +	if (ret == -ENOMEM)
> +		drm_err(mtk_crtc->base.dev, "KMS: Failed to allocate colorop\n");
> +
> +	mtk_drm_colorop_pipeline_destroy(mtk_crtc->base.dev);
> +
> +	return ret;
> +}
> +
>   int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>   		    unsigned int path_len, int priv_data_index,
>   		    const struct mtk_drm_route *conn_routes,
> @@ -1103,6 +1302,8 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>   	if (ret < 0)
>   		return ret;
>   
> +	mtk_crtc_init_post_blend_color_pipeline(mtk_crtc, gamma_lut_size);
> +
>   	if (gamma_lut_size)
>   		drm_mode_crtc_set_gamma_size(&mtk_crtc->base, gamma_lut_size);
>   	drm_crtc_enable_color_mgmt(&mtk_crtc->base, 0, has_ctm, gamma_lut_size);
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> index 7289b3dcf22f22f344016beee0c7c144cf7b93c8..554c3cc8ad7b266b8b8eee74ceb8f7383fe2f8df 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> @@ -75,10 +75,12 @@ struct mtk_ddp_comp_funcs {
>   	unsigned int (*gamma_get_lut_size)(struct device *dev);
>   	void (*gamma_set)(struct device *dev,
>   			  struct drm_crtc_state *state);
> +	void (*gamma_set_color_pipeline)(struct device *dev, struct drm_color_lut32 *lut);
>   	void (*bgclr_in_on)(struct device *dev);
>   	void (*bgclr_in_off)(struct device *dev);
>   	void (*ctm_set)(struct device *dev,
>   			struct drm_crtc_state *state);
> +	void (*ctm_set_color_pipeline)(struct device *dev, struct drm_color_ctm_3x4 *ctm);
>   	struct device * (*dma_dev_get)(struct device *dev);
>   	u32 (*get_blend_modes)(struct device *dev);
>   	const u32 *(*get_formats)(struct device *dev);
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com