Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:53PM +0300, Tomi Valkeinen wrote:

Commit d49cd15550d9d4495f6187425318c245d58cb63f ("OMAPDSS: DISPC: lock access to DISPC_CONTROL & DISPC_CONFIG") added locking to mgr_fld_write(). This was needed in omapfb times due to lack of good locking, especially in the case of both V4L2 and fbdev layers using the DSS driver.

This is not needed for omapdrm, so we can remove the locking.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

I've followed all code paths, and they end up either in the bridge enable operations or the CRTC atomic flush (disregarding suspend/resume). Those can't race each other, so this should be safe.

---

drivers/gpu/drm/omapdrm/dss/dispc.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 785c5546067a..c6da33e7014f 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -184,9 +184,6 @@ struct dispc_device {

struct regmap *syscon_pol; u32 syscon_pol_offset;

• /* DISPC_CONTROL & DISPC_CONFIG lock*/
• spinlock_t control_lock;

};

enum omap_color_component { @@ -377,16 +374,7 @@ static void mgr_fld_write(struct dispc_device *dispc, enum omap_channel channel, enum mgr_reg_fields regfld, int val) { const struct dispc_reg_field rfld = mgr_desc[channel].reg_desc[regfld];

While at it, should you turn this into a pointer to avoid an unnecessary copy ?

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• const bool need_lock = rfld.reg == DISPC_CONTROL || rfld.reg == DISPC_CONFIG;
• unsigned long flags;
• if (need_lock) {

• spin_lock_irqsave(&dispc->control_lock, flags);
  

• REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
  

• spin_unlock_irqrestore(&dispc->control_lock, flags);
  

• } else {

• REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
  

• }

• REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);

}

static int dispc_get_num_ovls(struct dispc_device *dispc) @@ -4769,8 +4757,6 @@ static int dispc_bind(struct device *dev, struct device *master, void *data) platform_set_drvdata(pdev, dispc); dispc->dss = dss;

• spin_lock_init(&dispc->control_lock);
• /*
  • The OMAP3-based models can't be told apart using the compatible
  • string, use SoC device matching.

Hi Tomi,

Thank you for the path.

On Mon, Sep 02, 2019 at 03:53:54PM +0300, Tomi Valkeinen wrote:

From: Alejandro Hernandez ajhernandez@ti.com

A "HDMI I2C Master Error" is sometimes reported with the current DDC SCL timings. The current settings for a 10us SCL period (100 KHz) causes the error with some displays. This patch increases the SCL signal period from 10us to 10.2us, with the new settings the error is not observed

It would be useful to document what those "some displays" are if you can.

Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Signed-off-by: Alejandro Hernandez ajhernandez@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c index 7400fb99d453..4c588ec7634a 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c @@ -39,8 +39,8 @@ static void hdmi_core_ddc_init(struct hdmi_core_data *core) { void __iomem *base = core->base; const unsigned long long iclk = 266000000; /* DSS L3 ICLK */

• const unsigned int ss_scl_high = 4600; /* ns */
• const unsigned int ss_scl_low = 5400; /* ns */

• const unsigned int ss_scl_high = 4700; /* ns */
• const unsigned int ss_scl_low = 5500; /* ns */ const unsigned int fs_scl_high = 600; /* ns */ const unsigned int fs_scl_low = 1300; /* ns */ const unsigned int sda_hold = 1000; /* ns */

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:55PM +0300, Tomi Valkeinen wrote:

OMAP2 and OMAP3/AM4 have limitations with the scaler:

• OMAP2 can only scale XRGB8888
• OMAP3/AM4 can only scale XRGB8888, RGB565, YUYV and UYVY

The driver doesn't check these limitations, which leads to sync-lost floods.

This patch adds a check for the pixel formats when scaling.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/omapdrm/dss/dispc.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index c6da33e7014f..7d87d60edb80 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -114,6 +114,7 @@ struct dispc_features { const unsigned int num_reg_fields; const enum omap_overlay_caps *overlay_caps; const u32 **supported_color_modes;

• const u32 *supported_scaler_color_modes; unsigned int num_mgrs; unsigned int num_ovls; unsigned int buffer_size_unit;

@@ -2498,6 +2499,19 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc, if (width == out_width && height == out_height) return 0;

• if (dispc->feat->supported_scaler_color_modes) {

• const u32 *modes = dispc->feat->supported_scaler_color_modes;
  

• int i;
  

i is never negative and can thus be an unsigned int. Apart from that,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• for (i = 0; modes[i]; ++i) {
  

• 	if (modes[i] == fourcc)
  

• 		break;
  

• }
  

• if (modes[i] == 0)
  

• 	return -EINVAL;
  

• }
• if (plane == OMAP_DSS_WB) { switch (fourcc) { case DRM_FORMAT_NV12:

@@ -4213,6 +4227,12 @@ static const u32 *omap4_dispc_supported_color_modes[] = { DRM_FORMAT_RGBX8888), };

+static const u32 omap3_dispc_supported_scaler_color_modes[] = {

• DRM_FORMAT_XRGB8888, DRM_FORMAT_RGB565, DRM_FORMAT_YUYV,
• DRM_FORMAT_UYVY,
• 0,

+};

static const struct dispc_features omap24xx_dispc_feats = { .sw_start = 5, .fp_start = 15, @@ -4241,6 +4261,7 @@ static const struct dispc_features omap24xx_dispc_feats = { .num_reg_fields = ARRAY_SIZE(omap2_dispc_reg_fields), .overlay_caps = omap2_dispc_overlay_caps, .supported_color_modes = omap2_dispc_supported_color_modes,

• .supported_scaler_color_modes = COLOR_ARRAY(DRM_FORMAT_XRGB8888), .num_mgrs = 2, .num_ovls = 3, .buffer_size_unit = 1,

@@ -4275,6 +4296,7 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = { .num_reg_fields = ARRAY_SIZE(omap3_dispc_reg_fields), .overlay_caps = omap3430_dispc_overlay_caps, .supported_color_modes = omap3_dispc_supported_color_modes,

• .supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes, .num_mgrs = 2, .num_ovls = 3, .buffer_size_unit = 1,

@@ -4309,6 +4331,7 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = { .num_reg_fields = ARRAY_SIZE(omap3_dispc_reg_fields), .overlay_caps = omap3430_dispc_overlay_caps, .supported_color_modes = omap3_dispc_supported_color_modes,

• .supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes, .num_mgrs = 2, .num_ovls = 3, .buffer_size_unit = 1,

@@ -4343,6 +4366,7 @@ static const struct dispc_features omap36xx_dispc_feats = { .num_reg_fields = ARRAY_SIZE(omap3_dispc_reg_fields), .overlay_caps = omap3630_dispc_overlay_caps, .supported_color_modes = omap3_dispc_supported_color_modes,

• .supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes, .num_mgrs = 2, .num_ovls = 3, .buffer_size_unit = 1,

@@ -4377,6 +4401,7 @@ static const struct dispc_features am43xx_dispc_feats = { .num_reg_fields = ARRAY_SIZE(omap3_dispc_reg_fields), .overlay_caps = omap3430_dispc_overlay_caps, .supported_color_modes = omap3_dispc_supported_color_modes,

• .supported_scaler_color_modes = omap3_dispc_supported_scaler_color_modes, .num_mgrs = 1, .num_ovls = 3, .buffer_size_unit = 1,

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@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.

...

Signed-off-by: Jyri Sarha jsarha@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@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).

...

+}

+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); }

On 04/09/2019 14:11, Laurent Pinchart wrote:

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@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 :-)

I'll try to write something and send the next series to wider audience. Let's see what jury says.

...

...

...

Signed-off-by: Jyri Sarha jsarha@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@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 ?

I've at least seen CSC matrices with 2.0 values, so I think we should accept those and use clamping, but maybe we should reject CTMs with values far bigger than what we can represent. Such matrices would hardly work the way they were intended.

...

...

...

+}

+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 ?

Hmmm, jess I should do it like this: if (crtc->state->color_mgmt_changed) { if (crtc->state->ctm) { ...

...

...

...

• } else {

• info.cpr_enable = false;
  

• }

}

This way the whole CPR functionality is turned off, if the there is no CTM in the crtc state.

...

...

...

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); }

On 05/09/2019 00:52, Laurent Pinchart wrote:

...

...

...

...

...

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 ?

Hmmm, jess I should do it like this: if (crtc->state->color_mgmt_changed) { if (crtc->state->ctm) { ...

...

...

...

...

• } else {

• info.cpr_enable = false;
  

• }

}

This way the whole CPR functionality is turned off, if the there is no CTM in the crtc state.

Yes, but you would also need to update .mgr_setup() :-) A new color_mgmt_changed flag would be needed in the info structure too.

I am starting to thing that such an "optimization" may not be worth the added complexity. The arithmetic and writing three registers is not that costly and we do not commit a new crtc state that often.

If we later consider otherwise we can add the optimization as a separate patch.

BR, Jyri

On 05/09/2019 13:05, Laurent Pinchart wrote:

Hi Jyri,

On Thu, Sep 05, 2019 at 01:00:51PM +0300, Jyri Sarha wrote:

...

On 05/09/2019 00:52, Laurent Pinchart wrote:

...

...

...

...

>> 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 ?

Hmmm, jess I should do it like this: if (crtc->state->color_mgmt_changed) { if (crtc->state->ctm) { ...

...

...

>> + } else { >> + info.cpr_enable = false; >> + }

}

This way the whole CPR functionality is turned off, if the there is no CTM in the crtc state.

Yes, but you would also need to update .mgr_setup() :-) A new color_mgmt_changed flag would be needed in the info structure too.

I am starting to thing that such an "optimization" may not be worth the added complexity. The arithmetic and writing three registers is not that costly and we do not commit a new crtc state that often.

We call omap_crtc_write_crtc_properties() in omap_crtc_atomic_flush(), so that's at every page flip...

Still, the mgr_setup() accesses five different registers even if we do not touch CPR settings (another 4 registers). All of those have static values in the mainline omapdrm (there are custom properties to control those in ti-linux).

I would rather keep this patch as it is and implement another one with a cached struct omap_overlay_manager_info, that calls mgr_setup() only if the info values have changed.

With the cached values in place the unneeded conversion arithmetic can also be skipped based on color_mgmt_changed.

BR, Jyri

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:

From: Jyri Sarha jsarha@ti.com

Adds support for COLOR_ENCODING and COLOR_RANGE properties to omap_plane.c and dispc.c. The supported encodings and ranges are:

For COLOR_ENCODING:

• YCbCt BT.601 (default)

Did you mean YCbCr ?

• YCbCt BT.709

For COLOR_RANGE:

• YCbCt limited range
• YCbCt full range (default)

Signed-off-by: Jyri Sarha jsarha@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/dss/dispc.c | 119 ++++++++++++++++++++------ drivers/gpu/drm/omapdrm/dss/omapdss.h | 4 + drivers/gpu/drm/omapdrm/omap_plane.c | 30 +++++++ 3 files changed, 127 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 7d87d60edb80..40ddb28ee7aa 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc, #undef CVAL }

-static void dispc_setup_color_conv_coef(struct dispc_device *dispc) +/* YUV -> RGB, ITU-R BT.601, full range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {

static const is usually preferred over const static, here and below.

• 256, 0, 358, /* ry, rcb, rcr |1.000 0.000 1.402|*/
• 256, -88, -182, /* gy, gcb, gcr |1.000 -0.344 -0.714|*/
• 256, 452, 0, /* by, bcb, bcr |1.000 1.772 0.000|*/
• true, /* full range */

+};

+/* YUV -> RGB, ITU-R BT.601, limited range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298, 0, 409, /* ry, rcb, rcr |1.164 0.000 1.596|*/
• 298, -100, -208, /* gy, gcb, gcr |1.164 -0.392 -0.813|*/
• 298, 516, 0, /* by, bcb, bcr |1.164 2.017 0.000|*/
• false, /* limited range */

+};

+/* YUV -> RGB, ITU-R BT.709, full range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {

• 256, 0, 402, /* ry, rcb, rcr |1.000 0.000 1.570|*/
• 256, -48, -120, /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
• 256, 475, 0, /* by, bcb, bcr |1.000 1.856 0.000|*/
• true, /* full range */

+};

+/* YUV -> RGB, ITU-R BT.709, limited range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {

• 298, 0, 459, /* ry, rcb, rcr |1.164 0.000 1.793|*/
• 298, -55, -136, /* gy, gcb, gcr |1.164 -0.213 -0.533|*/
• 298, 541, 0, /* by, bcb, bcr |1.164 2.112 0.000|*/
• false, /* limited range */

+};

+/* RGB -> YUV, ITU-R BT.601, limited range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {

• 66, 129,  25,		/* yr,   yg,  yb | 0.257  0.504  0.098|*/
  

• -38, -74, 112, /* cbr, cbg, cbb |-0.148 -0.291 0.439|*/
• 112, -94, -18, /* crr, crg, crb | 0.439 -0.368 -0.071|*/
• false, /* limited range */

+};

+/* RGB -> YUV, ITU-R BT.601, full range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {

• 77,  150,  29,		/* yr,   yg,  yb | 0.299  0.587  0.114|*/
  

• -43, -85, 128, /* cbr, cbg, cbb |-0.173 -0.339 0.511|*/
• 128, -107, -21, /* crr, crg, crb | 0.511 -0.428 -0.083|*/
• true, /* full range */

+};

+/* RGB -> YUV, ITU-R BT.709, limited range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {

That should be coefs_rgb2yuv_bt709_lim

• 47,  157,   16,	/* yr,   yg,  yb | 0.1826  0.6142  0.0620|*/
  

• -26, -87, 112, /* cbr, cbg, cbb |-0.1006 -0.3386 0.4392|*/
• 112, -102, -10, /* crr, crg, crb | 0.4392 -0.3989 -0.0403|*/
• false, /* limited range */

+};

Why is coefs_rgb2yuv_bt709_full not supported ? Actually coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.

+static int dispc_ovl_set_csc(struct dispc_device *dispc,

• 	     enum omap_plane_id plane,
  

• 	     enum drm_color_encoding color_encoding,
  

• 	     enum drm_color_range color_range)
  

{

• int i;
• int num_ovl = dispc_get_num_ovls(dispc);
• /* YUV -> RGB, ITU-R BT.601, limited range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298,    0,  409,	/* ry, rcb, rcr */
  

• 298, -100, -208,	/* gy, gcb, gcr */
  

• 298,  516,    0,	/* by, bcb, bcr */
  

• false,			/* limited range */
  

• };

• const struct csc_coef_yuv2rgb *csc;

• /* RGB -> YUV, ITU-R BT.601, limited range */
• const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {

•  66, 129,  25,		/* yr,   yg,  yb */
  

• -38, -74, 112,		/* cbr, cbg, cbb */
  

• 112, -94, -18,		/* crr, crg, crb */
  

• false,			/* limited range */
  

• };

• switch (color_encoding) {
• case DRM_COLOR_YCBCR_BT601:

• if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
  

• 	csc = &coefs_yuv2rgb_bt601_full;
  

• else
  

• 	csc = &coefs_yuv2rgb_bt601_lim;
  

• break;
  

• case DRM_COLOR_YCBCR_BT709:

• if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
  

• 	csc = &coefs_yuv2rgb_bt709_full;
  

• else
  

• 	csc = &coefs_yuv2rgb_bt709_lim;
  

• break;
  

• default:

• DSSERR("Unsupported CSC mode %d for plane %d\n",
  

•        color_encoding, plane);
  

• return -EINVAL;
  

• }

• for (i = 1; i < num_ovl; i++)

• dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
  

• dispc_ovl_write_color_conv_coef(dispc, plane, csc);

• if (dispc->feat->has_writeback)

• dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
  

• return 0;

}

static void dispc_ovl_set_ba0(struct dispc_device *dispc, @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc, u8 pre_mult_alpha, u8 global_alpha, enum omap_dss_rotation_type rotation_type, bool replication, const struct videomode *vm,

• 		  bool mem_to_mem)
  

• 		  bool mem_to_mem,
  

• 		  enum drm_color_encoding color_encoding,
  

• 		  enum drm_color_range color_range)
  

{ bool five_taps = true; bool fieldmode = false; @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc, fieldmode, fourcc, rotation); dispc_ovl_set_output_size(dispc, plane, out_width, out_height); dispc_ovl_set_vid_color_conv(dispc, plane, cconv);

• if (plane != OMAP_DSS_WB)
  

• 	dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
  

  }

  dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,

@@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc, oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height, oi->out_width, oi->out_height, oi->fourcc, oi->rotation, oi->zorder, oi->pre_mult_alpha, oi->global_alpha,

• oi->rotation_type, replication, vm, mem_to_mem);
  

• oi->rotation_type, replication, vm, mem_to_mem,
  

• oi->color_encoding, oi->color_range);
  
  

  return r;

} @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc, wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width, wi->height, wi->fourcc, wi->rotation, zorder, wi->pre_mult_alpha, global_alpha, wi->rotation_type,

• replication, vm, mem_to_mem);
  

• replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
  

• DRM_COLOR_YCBCR_LIMITED_RANGE);
  

  if (r) return r;

@@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc) dispc->feat->has_gamma_table) REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);

• dispc_setup_color_conv_coef(dispc);

• if (dispc->feat->has_writeback)

• dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
  
  

  dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 79f6b195c7cf..1430cab6b877 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -14,6 +14,7 @@ #include <linux/platform_data/omapdss.h> #include <uapi/drm/drm_mode.h> #include <drm/drm_crtc.h> +#include <drm/drm_color_mgmt.h>

Alphabetical order ? While at is you coulde remove uapi/

#define DISPC_IRQ_FRAMEDONE (1 << 0) #define DISPC_IRQ_VSYNC (1 << 1) @@ -243,6 +244,9 @@ struct omap_overlay_info { u8 global_alpha; u8 pre_mult_alpha; u8 zorder;

• enum drm_color_encoding color_encoding;
• enum drm_color_range color_range;

};

struct omap_overlay_manager_info { diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 73ec99819a3d..db8e917260df 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane, info.pre_mult_alpha = 1; else info.pre_mult_alpha = 0;

• info.color_encoding = state->color_encoding;

• info.color_range = state->color_range;

  /* update scanout: */ omap_framebuffer_update_scanout(state->fb, state, &info);

@@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane) */ plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id;

• plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
• plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;

}

static int omap_plane_atomic_set_property(struct drm_plane *plane, @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = { .atomic_get_property = omap_plane_atomic_get_property, };

+static bool omap_plane_supports_yuv(struct drm_plane *plane) +{

• struct omap_drm_private *priv = plane->dev->dev_private;
• struct omap_plane *omap_plane = to_omap_plane(plane);
• const u32 *formats =

• priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
  

• int i;

unsigned int i ?

• for (i = 0; formats[i]; i++)

• if (formats[i] == DRM_FORMAT_YUYV ||
  

•     formats[i] == DRM_FORMAT_UYVY ||
  

•     formats[i] == DRM_FORMAT_NV12)
  

• 	return true;
  

• return false;

+}

static const char *plane_id_to_name[] = { [OMAP_DSS_GFX] = "gfx", [OMAP_DSS_VIDEO1] = "vid1", @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) | BIT(DRM_MODE_BLEND_COVERAGE));

• if (omap_plane_supports_yuv(plane))

• drm_plane_create_color_properties(plane,
  

• 			BIT(DRM_COLOR_YCBCR_BT601) |
  

• 			BIT(DRM_COLOR_YCBCR_BT709),
  

• 			BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
  

• 			BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
  

• 			DRM_COLOR_YCBCR_BT601,
  

• 			DRM_COLOR_YCBCR_FULL_RANGE);
  

• return plane;

error:

Hi Jyri,

On Thu, Sep 05, 2019 at 12:24:37PM +0300, Jyri Sarha wrote:

On 03/09/2019 18:32, Laurent Pinchart wrote:

...

On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:

...

From: Jyri Sarha jsarha@ti.com

Adds support for COLOR_ENCODING and COLOR_RANGE properties to omap_plane.c and dispc.c. The supported encodings and ranges are:

For COLOR_ENCODING:

• YCbCt BT.601 (default)

Did you mean YCbCr ?

...

...

• YCbCt BT.709

For COLOR_RANGE:

• YCbCt limited range
• YCbCt full range (default)

Signed-off-by: Jyri Sarha jsarha@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

---

drivers/gpu/drm/omapdrm/dss/dispc.c | 119 ++++++++++++++++++++------ drivers/gpu/drm/omapdrm/dss/omapdss.h | 4 + drivers/gpu/drm/omapdrm/omap_plane.c | 30 +++++++ 3 files changed, 127 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 7d87d60edb80..40ddb28ee7aa 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc, #undef CVAL }

-static void dispc_setup_color_conv_coef(struct dispc_device *dispc) +/* YUV -> RGB, ITU-R BT.601, full range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {

static const is usually preferred over const static, here and below.

...

• 256, 0, 358, /* ry, rcb, rcr |1.000 0.000 1.402|*/
• 256, -88, -182, /* gy, gcb, gcr |1.000 -0.344 -0.714|*/
• 256, 452, 0, /* by, bcb, bcr |1.000 1.772 0.000|*/
• true, /* full range */

+};

+/* YUV -> RGB, ITU-R BT.601, limited range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298, 0, 409, /* ry, rcb, rcr |1.164 0.000 1.596|*/
• 298, -100, -208, /* gy, gcb, gcr |1.164 -0.392 -0.813|*/
• 298, 516, 0, /* by, bcb, bcr |1.164 2.017 0.000|*/
• false, /* limited range */

+};

+/* YUV -> RGB, ITU-R BT.709, full range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {

• 256, 0, 402, /* ry, rcb, rcr |1.000 0.000 1.570|*/
• 256, -48, -120, /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
• 256, 475, 0, /* by, bcb, bcr |1.000 1.856 0.000|*/
• true, /* full range */

+};

+/* YUV -> RGB, ITU-R BT.709, limited range */ +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {

• 298, 0, 459, /* ry, rcb, rcr |1.164 0.000 1.793|*/
• 298, -55, -136, /* gy, gcb, gcr |1.164 -0.213 -0.533|*/
• 298, 541, 0, /* by, bcb, bcr |1.164 2.112 0.000|*/
• false, /* limited range */

+};

+/* RGB -> YUV, ITU-R BT.601, limited range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {

• 66, 129,  25,		/* yr,   yg,  yb | 0.257  0.504  0.098|*/
  

• -38, -74, 112, /* cbr, cbg, cbb |-0.148 -0.291 0.439|*/
• 112, -94, -18, /* crr, crg, crb | 0.439 -0.368 -0.071|*/
• false, /* limited range */

+};

+/* RGB -> YUV, ITU-R BT.601, full range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {

• 77,  150,  29,		/* yr,   yg,  yb | 0.299  0.587  0.114|*/
  

• -43, -85, 128, /* cbr, cbg, cbb |-0.173 -0.339 0.511|*/
• 128, -107, -21, /* crr, crg, crb | 0.511 -0.428 -0.083|*/
• true, /* full range */

+};

+/* RGB -> YUV, ITU-R BT.709, limited range */ +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {

That should be coefs_rgb2yuv_bt709_lim

...

• 47,  157,   16,	/* yr,   yg,  yb | 0.1826  0.6142  0.0620|*/
  

• -26, -87, 112, /* cbr, cbg, cbb |-0.1006 -0.3386 0.4392|*/
• 112, -102, -10, /* crr, crg, crb | 0.4392 -0.3989 -0.0403|*/
• false, /* limited range */

+};

Why is coefs_rgb2yuv_bt709_full not supported ? Actually coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.

I must have simply forgotten. This is an old patch and I can not remember exactly. But I remember that I wanted to add all CSCs at one time so that I do not need start from the beginning when ever a new conversion is asked. For the moment rgb to yuv conversions are only used for write back and it currently only uses the default BT.601 Full range.

I'll add rgb2yuv_bt709_full. Or should I remove all the unused CSCs? Or maybe put them behind some ifdef so that they do not bloat the kernel?

Commented-out code is frowned upon. I would just drop the unused data.

...

...

+static int dispc_ovl_set_csc(struct dispc_device *dispc,

• 	     enum omap_plane_id plane,
  

• 	     enum drm_color_encoding color_encoding,
  

• 	     enum drm_color_range color_range)
  

{

• int i;
• int num_ovl = dispc_get_num_ovls(dispc);
• /* YUV -> RGB, ITU-R BT.601, limited range */
• const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {

• 298,    0,  409,	/* ry, rcb, rcr */
  

• 298, -100, -208,	/* gy, gcb, gcr */
  

• 298,  516,    0,	/* by, bcb, bcr */
  

• false,			/* limited range */
  

• };

• const struct csc_coef_yuv2rgb *csc;

• /* RGB -> YUV, ITU-R BT.601, limited range */
• const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {

•  66, 129,  25,		/* yr,   yg,  yb */
  

• -38, -74, 112,		/* cbr, cbg, cbb */
  

• 112, -94, -18,		/* crr, crg, crb */
  

• false,			/* limited range */
  

• };

• switch (color_encoding) {
• case DRM_COLOR_YCBCR_BT601:

• if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
  

• 	csc = &coefs_yuv2rgb_bt601_full;
  

• else
  

• 	csc = &coefs_yuv2rgb_bt601_lim;
  

• break;
  

• case DRM_COLOR_YCBCR_BT709:

• if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
  

• 	csc = &coefs_yuv2rgb_bt709_full;
  

• else
  

• 	csc = &coefs_yuv2rgb_bt709_lim;
  

• break;
  

• default:

• DSSERR("Unsupported CSC mode %d for plane %d\n",
  

•        color_encoding, plane);
  

• return -EINVAL;
  

• }

• for (i = 1; i < num_ovl; i++)

• dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
  

• dispc_ovl_write_color_conv_coef(dispc, plane, csc);

• if (dispc->feat->has_writeback)

• dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
  

• return 0;

}

static void dispc_ovl_set_ba0(struct dispc_device *dispc, @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc, u8 pre_mult_alpha, u8 global_alpha, enum omap_dss_rotation_type rotation_type, bool replication, const struct videomode *vm,

• 		  bool mem_to_mem)
  

• 		  bool mem_to_mem,
  

• 		  enum drm_color_encoding color_encoding,
  

• 		  enum drm_color_range color_range)
  

{ bool five_taps = true; bool fieldmode = false; @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc, fieldmode, fourcc, rotation); dispc_ovl_set_output_size(dispc, plane, out_width, out_height); dispc_ovl_set_vid_color_conv(dispc, plane, cconv);

• if (plane != OMAP_DSS_WB)
  

• 	dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
  

  }

  dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,

@@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc, oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height, oi->out_width, oi->out_height, oi->fourcc, oi->rotation, oi->zorder, oi->pre_mult_alpha, oi->global_alpha,

• oi->rotation_type, replication, vm, mem_to_mem);
  

• oi->rotation_type, replication, vm, mem_to_mem,
  

• oi->color_encoding, oi->color_range);
  
  

  return r;

} @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc, wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width, wi->height, wi->fourcc, wi->rotation, zorder, wi->pre_mult_alpha, global_alpha, wi->rotation_type,

• replication, vm, mem_to_mem);
  

• replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
  

• DRM_COLOR_YCBCR_LIMITED_RANGE);
  

  if (r) return r;

@@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc) dispc->feat->has_gamma_table) REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);

• dispc_setup_color_conv_coef(dispc);

• if (dispc->feat->has_writeback)

• dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
  
  

  dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 79f6b195c7cf..1430cab6b877 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -14,6 +14,7 @@ #include <linux/platform_data/omapdss.h> #include <uapi/drm/drm_mode.h> #include <drm/drm_crtc.h> +#include <drm/drm_color_mgmt.h>

Alphabetical order ? While at is you coulde remove uapi/

...

#define DISPC_IRQ_FRAMEDONE (1 << 0) #define DISPC_IRQ_VSYNC (1 << 1) @@ -243,6 +244,9 @@ struct omap_overlay_info { u8 global_alpha; u8 pre_mult_alpha; u8 zorder;

• enum drm_color_encoding color_encoding;
• enum drm_color_range color_range;

};

struct omap_overlay_manager_info { diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 73ec99819a3d..db8e917260df 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane, info.pre_mult_alpha = 1; else info.pre_mult_alpha = 0;

• info.color_encoding = state->color_encoding;

• info.color_range = state->color_range;

  /* update scanout: */ omap_framebuffer_update_scanout(state->fb, state, &info);

@@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane) */ plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id;

• plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
• plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;

}

static int omap_plane_atomic_set_property(struct drm_plane *plane, @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = { .atomic_get_property = omap_plane_atomic_get_property, };

+static bool omap_plane_supports_yuv(struct drm_plane *plane) +{

• struct omap_drm_private *priv = plane->dev->dev_private;
• struct omap_plane *omap_plane = to_omap_plane(plane);
• const u32 *formats =

• priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
  

• int i;

unsigned int i ?

...

• for (i = 0; formats[i]; i++)

• if (formats[i] == DRM_FORMAT_YUYV ||
  

•     formats[i] == DRM_FORMAT_UYVY ||
  

•     formats[i] == DRM_FORMAT_NV12)
  

• 	return true;
  

• return false;

+}

static const char *plane_id_to_name[] = { [OMAP_DSS_GFX] = "gfx", [OMAP_DSS_VIDEO1] = "vid1", @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) | BIT(DRM_MODE_BLEND_COVERAGE));

• if (omap_plane_supports_yuv(plane))

• drm_plane_create_color_properties(plane,
  

• 			BIT(DRM_COLOR_YCBCR_BT601) |
  

• 			BIT(DRM_COLOR_YCBCR_BT709),
  

• 			BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
  

• 			BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
  

• 			DRM_COLOR_YCBCR_BT601,
  

• 			DRM_COLOR_YCBCR_FULL_RANGE);
  

• return plane;

error:

Hi Tomi,

Thank you for the patch.

Missing "Move" in the subject after "dss: " ?

On Mon, Sep 02, 2019 at 03:53:58PM +0300, Tomi Valkeinen wrote:

From: Jyri Sarha jsarha@ti.com

The core.c just for registering the drivers is kind of useless. Let's get rid of it and register the dss drivers in dss.c.

Signed-off-by: Jyri Sarha jsarha@ti.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/gpu/drm/omapdrm/dss/Makefile | 2 +- drivers/gpu/drm/omapdrm/dss/core.c | 55 ---------------------------- drivers/gpu/drm/omapdrm/dss/dss.c | 37 +++++++++++++++++++ 3 files changed, 38 insertions(+), 56 deletions(-) delete mode 100644 drivers/gpu/drm/omapdrm/dss/core.c

diff --git a/drivers/gpu/drm/omapdrm/dss/Makefile b/drivers/gpu/drm/omapdrm/dss/Makefile index 904101c5e79d..5950c3f52c2e 100644 --- a/drivers/gpu/drm/omapdrm/dss/Makefile +++ b/drivers/gpu/drm/omapdrm/dss/Makefile @@ -6,7 +6,7 @@ omapdss-base-y := base.o display.o dss-of.o output.o

obj-$(CONFIG_OMAP2_DSS) += omapdss.o # Core DSS files -omapdss-y := core.o dss.o dispc.o dispc_coefs.o \ +omapdss-y := dss.o dispc.o dispc_coefs.o \ pll.o video-pll.o omapdss-$(CONFIG_OMAP2_DSS_DPI) += dpi.o omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c deleted file mode 100644 index 6ac497b63711..000000000000 --- a/drivers/gpu/drm/omapdrm/dss/core.c +++ /dev/null @@ -1,55 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/*

• • Copyright (C) 2009 Nokia Corporation
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• • Some code and ideas taken from drivers/video/omap/ driver
• • by Imre Deak.
• */

-#define DSS_SUBSYS_NAME "CORE"

-#include <linux/kernel.h> -#include <linux/module.h> -#include <linux/platform_device.h>

-#include "omapdss.h" -#include "dss.h"

-/* INIT */ -static struct platform_driver * const omap_dss_drivers[] = {

• &omap_dsshw_driver,
• &omap_dispchw_driver,

-#ifdef CONFIG_OMAP2_DSS_DSI

• &omap_dsihw_driver,

-#endif -#ifdef CONFIG_OMAP2_DSS_VENC

• &omap_venchw_driver,

-#endif -#ifdef CONFIG_OMAP4_DSS_HDMI

• &omapdss_hdmi4hw_driver,

-#endif -#ifdef CONFIG_OMAP5_DSS_HDMI

• &omapdss_hdmi5hw_driver,

-#endif -};

-static int __init omap_dss_init(void) -{

• return platform_register_drivers(omap_dss_drivers,

• 			 ARRAY_SIZE(omap_dss_drivers));
  

-}

-static void __exit omap_dss_exit(void) -{

• platform_unregister_drivers(omap_dss_drivers,

• 		    ARRAY_SIZE(omap_dss_drivers));
  

-}

-module_init(omap_dss_init); -module_exit(omap_dss_exit);

-MODULE_AUTHOR("Tomi Valkeinen tomi.valkeinen@ti.com"); -MODULE_DESCRIPTION("OMAP2/3 Display Subsystem"); -MODULE_LICENSE("GPL v2");

diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c index e226324adb69..41d495a360d8 100644 --- a/drivers/gpu/drm/omapdrm/dss/dss.c +++ b/drivers/gpu/drm/omapdrm/dss/dss.c @@ -1598,3 +1598,40 @@ struct platform_driver omap_dsshw_driver = { .suppress_bind_attrs = true, }, };

+/* INIT */ +static struct platform_driver * const omap_dss_drivers[] = {

• &omap_dsshw_driver,
• &omap_dispchw_driver,

+#ifdef CONFIG_OMAP2_DSS_DSI

• &omap_dsihw_driver,

+#endif +#ifdef CONFIG_OMAP2_DSS_VENC

• &omap_venchw_driver,

+#endif +#ifdef CONFIG_OMAP4_DSS_HDMI

• &omapdss_hdmi4hw_driver,

+#endif +#ifdef CONFIG_OMAP5_DSS_HDMI

• &omapdss_hdmi5hw_driver,

+#endif +};

+static int __init omap_dss_init(void) +{

• return platform_register_drivers(omap_dss_drivers,

• 			 ARRAY_SIZE(omap_dss_drivers));
  

+}

+static void __exit omap_dss_exit(void) +{

• platform_unregister_drivers(omap_dss_drivers,

• 		    ARRAY_SIZE(omap_dss_drivers));
  

+}

+module_init(omap_dss_init); +module_exit(omap_dss_exit);

+MODULE_AUTHOR("Tomi Valkeinen tomi.valkeinen@ti.com"); +MODULE_DESCRIPTION("OMAP2/3/4/5 Display Subsystem"); +MODULE_LICENSE("GPL v2");