[PATCH 4/7] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Jyri,

On Wed, Sep 04, 2019 at 10:17:00AM +0300, Jyri Sarha wrote:
> On 03/09/2019 18:24, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha at ti.com>
> >>
> >> 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 after gamma table look up. However, it seems stupid to add a
> >> custom property just for that.
> > 
> > In that case the DRM documentation should be updated to mention that
> > both options are allowed.
> 
> Ok, if that is alright. But if we do that, then I guess all the drivers
> implementing CTM should document the point where it is applied in the
> pipeline.

Whatever solution we end up picking, I think it should at least be
discussed with a broader upstream audience and not just swept under the
omapdrm carpet :-)

> >> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
> >>  1 file changed, 37 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> index 3c5ddbf30e97..d63213dd7d83 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -391,6 +391,32 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
> >>  	}
> >>  }
> >>  
> >> +static s16 omap_crtc_S31_32_to_s2_8(s64 coef)
> >> +{
> >> +	uint64_t sign_bit = 1ULL << 63;
> >> +	uint64_t cbits = (uint64_t) coef;
> > 
> > s/uint64_t/u64/ for both lines as we're dealing with kernel code. And
> > there's no need for a space before coef.
> > 
> >> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1FF);
> >> +
> >> +	if (cbits & sign_bit)
> >> +		ret = -ret;
> >> +
> >> +	return ret;
> > 
> > Can't this be simplified to 
> > 
> > 	s16 ret = (coef >> 24) & 0x1ff;
> > 
> > 	return coef < 0 ? -ret : ret;
> > 
> 
> No. Clamping is different thing. If the original value is greater than
> what we can present with our 2 magnitude bit, we want to use the maximum
> value, not something that we may have in the LSB end of bits. e.g if
> user-space tries to set the value to 2.0 (= 0x200) we rather present it
> as 1.996 (= 0x1FF) than 0.0 (= 0x000).

Of course, my bad.

Perhaps a stupid question, should we reject out of range values at
atomic check time ?

> >> +}
> >> +
> >> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> >> +					 struct omap_dss_cpr_coefs *cpr)
> >> +{
> >> +	cpr->rr = omap_crtc_S31_32_to_s2_8(ctm->matrix[0]);
> >> +	cpr->rg = omap_crtc_S31_32_to_s2_8(ctm->matrix[1]);
> >> +	cpr->rb = omap_crtc_S31_32_to_s2_8(ctm->matrix[2]);
> >> +	cpr->gr = omap_crtc_S31_32_to_s2_8(ctm->matrix[3]);
> >> +	cpr->gg = omap_crtc_S31_32_to_s2_8(ctm->matrix[4]);
> >> +	cpr->gb = omap_crtc_S31_32_to_s2_8(ctm->matrix[5]);
> >> +	cpr->br = omap_crtc_S31_32_to_s2_8(ctm->matrix[6]);
> >> +	cpr->bg = omap_crtc_S31_32_to_s2_8(ctm->matrix[7]);
> >> +	cpr->bb = omap_crtc_S31_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;
> >> @@ -402,7 +428,16 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> >>  	info.default_color = 0x000000;
> >>  	info.trans_enabled = false;
> >>  	info.partial_alpha_enabled = false;
> >> -	info.cpr_enable = false;
> >> +
> >> +	if (crtc->state->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);
> > 
> > As an optimisation it would be nice to only write the coefficients when
> > they actually change. That could be implemented on top of this series.
> 
> E.g. apply this ?
> 
> - if (crtc->state->ctm)
> + if (crtc->state->color_mgmt_changed && crtc->state->ctm)

Something like that, but .mgr_setup() should then be taught not to write
unchanged CTM tables to registers. Do you think it would be worth it ?

> >> +	} else {
> >> +		info.cpr_enable = false;
> >> +	}
> >>  
> >>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
> >>  }
> >> @@ -836,7 +871,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
> >>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
> >>  		unsigned int 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