Re: [PATCH v4 6/6] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jyri Sarha <jsarha@ti.com>
Cc: tomi.valkeinen@ti.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 6/6] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix
Date: Fri, 31 Mar 2017 01:44:22 +0300	[thread overview]
Message-ID: <2361710.kGHQgPDWZv@avalon> (raw)
In-Reply-To: <69b5cdde2a82563e29ae8d8b2a5af714418ea633.1490366823.git.jsarha@ti.com>

Hi Jyri,

Thank you for the patch.

On Friday 24 Mar 2017 16:47:56 Jyri Sarha wrote:
> Implement CTM color management property for OMAP CRTC using DSS
> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> exactly match the CTM property documentation. On DSS the CPR matrix is
> applied before gamma table look up. However, it seems stupid to add a
> custom property just for that.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 026f73b..ed3b631 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -312,6 +312,25 @@ void omap_crtc_vblank_irq(struct drm_crtc *crtc)
>  	DBG("%s: apply done", omap_crtc->name);
>  }
> 
> +static s16 omap_crtc_s32_32_to_s2_8(s64 coef)
> +{
> +	return (s16)(coef >> 24);
> +}
> +
> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> +					 struct omap_dss_cpr_coefs *cpr)
> +{
> +	cpr->rr = omap_crtc_s32_32_to_s2_8(ctm->matrix[0]);
> +	cpr->rg = omap_crtc_s32_32_to_s2_8(ctm->matrix[1]);
> +	cpr->rb = omap_crtc_s32_32_to_s2_8(ctm->matrix[2]);
> +	cpr->gr = omap_crtc_s32_32_to_s2_8(ctm->matrix[3]);
> +	cpr->gg = omap_crtc_s32_32_to_s2_8(ctm->matrix[4]);
> +	cpr->gb = omap_crtc_s32_32_to_s2_8(ctm->matrix[5]);
> +	cpr->br = omap_crtc_s32_32_to_s2_8(ctm->matrix[6]);
> +	cpr->bg = omap_crtc_s32_32_to_s2_8(ctm->matrix[7]);
> +	cpr->bb = omap_crtc_s32_32_to_s2_8(ctm->matrix[8]);
> +}
> +
>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>  {
>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> @@ -325,6 +344,15 @@ static void omap_crtc_write_crtc_properties(struct
> drm_crtc *crtc) info.partial_alpha_enabled = false;
>  	info.cpr_enable = false;
> 
> +	if (crtc->state->ctm && !WARN_ON(crtc->state->ctm->length !=
> +					 sizeof(struct drm_color_ctm))) {

Can the length check fail ? The property set path calls 
drm_atomic_replace_property_blob_from_id() which contains

                if (expected_size > 0 && expected_size != new_blob->length) {
                        drm_property_unreference_blob(new_blob);
                        return -EINVAL;
                }

and expected_size is set to sizeof(struct drm_color_ctm).

> +		struct drm_color_ctm *ctm =
> +			(struct drm_color_ctm *) crtc->state->ctm->data;
> +
> +		info.cpr_enable = true;
> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
> +	}
> +
>  	priv->dispc_ops->mgr_setup(omap_crtc->channel, &info);
>  }
> 
> @@ -624,7 +652,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  	if (priv->dispc_ops->mgr_gamma_size(channel)) {
>  		uint gamma_lut_size = 256;
> 
> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
> +		drm_crtc_enable_color_mgmt(crtc, 0, true, gamma_lut_size);
>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
>  	}

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel