On 03/09/2019 18:24, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Mon, Sep 02, 2019 at 03:53:56PM +0300, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@xxxxxx>
>>
>> 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.
>> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>> ---
>> 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).
>> +}
>> +
>> +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)
>> + } 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);
>> }
>>
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
