Hi Neil and Jonas,

Thank you for the patch.

On Thu, Feb 06, 2020 at 08:18:24PM +0100, Neil Armstrong wrote:

From: Jonas Karlman jonas@kwiboo.se

Configure the correct mtmdsclock for deep colors to prepare support for 10, 12 & 16bit output.

Signed-off-by: Jonas Karlman jonas@kwiboo.se Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 67fca439bbfb..9e0927d22db6 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1818,9 +1818,26 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,

dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);

Nitpicking a bit, I would change

- vmode->mtmdsclock = vmode->mpixelclock = mode->clock * 1000; + vmode->mpixelclock = mode->clock * 1000;

above, and here add

vmode->mtmdsclock = vmode->mpixelclock;

to keep all mtmdsclock calculation in a single place.

• if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {

• switch (hdmi_bus_fmt_color_depth(
  

• 		hdmi->hdmi_data.enc_out_bus_format)) {
  

• case 16:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 2;
  

Both mpixelclock and mtmdsclock are unsigned int. Is the cast to u64 needed ?

On a separate but related note, what does the 'm' in tmdsclock stand for ? It seems to originate from the 'm' prefix for mpixelclock, which has been there from the start. Unless there's a good reason for the prefix, renaming mtmdsclock to tmds_clock (and handling the other fields in the hdmi_vmode structure similarly) would increase clarity I think.

• 	break;
  

• case 12:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 3 / 2;
  

• 	break;
  

• case 10:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 5 / 4;
  

• 	break;
  

• }
  

• }

• if (hdmi_bus_fmt_is_yuv420(hdmi->hdmi_data.enc_out_bus_format)) vmode->mtmdsclock /= 2;

• dev_dbg(hdmi->dev, "final tmdsclk = %d\n", vmode->mtmdsclock);

s/tmdsclk/tmdsclock/ to match the field name ?

• /* Set up HDMI_FC_INVIDCONF */ inv_val = (hdmi->hdmi_data.hdcp_enable || (dw_hdmi_support_scdc(hdmi) &&

Hi,

On 02/03/2020 10:05, Laurent Pinchart wrote:

Hi Neil and Jonas,

Thank you for the patch.

On Thu, Feb 06, 2020 at 08:18:24PM +0100, Neil Armstrong wrote:

...

From: Jonas Karlman jonas@kwiboo.se

Configure the correct mtmdsclock for deep colors to prepare support for 10, 12 & 16bit output.

Signed-off-by: Jonas Karlman jonas@kwiboo.se Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 67fca439bbfb..9e0927d22db6 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1818,9 +1818,26 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,

dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);

Nitpicking a bit, I would change

• vmode->mtmdsclock = vmode->mpixelclock = mode->clock * 1000;

• vmode->mpixelclock = mode->clock * 1000;

above, and here add

vmode->mtmdsclock = vmode->mpixelclock;

to keep all mtmdsclock calculation in a single place.

...

• if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {

• switch (hdmi_bus_fmt_color_depth(
  

• 		hdmi->hdmi_data.enc_out_bus_format)) {
  

• case 16:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 2;
  

Both mpixelclock and mtmdsclock are unsigned int. Is the cast to u64 needed ?

On a separate but related note, what does the 'm' in tmdsclock stand for ? It seems to originate from the 'm' prefix for mpixelclock, which has been there from the start. Unless there's a good reason for the prefix, renaming mtmdsclock to tmds_clock (and handling the other fields in the hdmi_vmode structure similarly) would increase clarity I think.

...

• 	break;
  

• case 12:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 3 / 2;
  

• 	break;
  

• case 10:
  

• 	vmode->mtmdsclock = (u64)vmode->mpixelclock * 5 / 4;
  

• 	break;
  

• }
  

• }

• if (hdmi_bus_fmt_is_yuv420(hdmi->hdmi_data.enc_out_bus_format)) vmode->mtmdsclock /= 2;

• dev_dbg(hdmi->dev, "final tmdsclk = %d\n", vmode->mtmdsclock);

s/tmdsclk/tmdsclock/ to match the field name ?

...

• /* Set up HDMI_FC_INVIDCONF */ inv_val = (hdmi->hdmi_data.hdcp_enable || (dw_hdmi_support_scdc(hdmi) &&

I fixed the calculus and the cast, but I'll rename the mtmdsclock in a following patch.

is it ok for you ?

Neil

Hi Jernej,

On 17/02/2020 07:38, Jernej Škrabec wrote:

Hi!

Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a):

...

From: Jonas Karlman jonas@kwiboo.se

Add the max_bpc property to the dw-hdmi connector to prepare support for 10, 12 & 16bit output support.

Signed-off-by: Jonas Karlman jonas@kwiboo.se Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 9e0927d22db6..051001f77dd4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) DRM_MODE_CONNECTOR_HDMIA, hdmi->ddc);

• drm_atomic_helper_connector_reset(connector);

Why is this reset needed?

I assume it's to allocate a new connector state to attach a the bpc propery.

But indeed, this helper is never used here, but only as callback to the drm_connector_funcs->reset.

But, amdgpu calls : /* * Some of the properties below require access to state, like bpc. * Allocate some default initial connector state with our reset helper. */ if (aconnector->base.funcs->reset) aconnector->base.funcs->reset(&aconnector->base);

which is the same.

Neil

Best regards, Jernej

...

• drm_connector_attach_max_bpc_property(connector, 8, 16);
• if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) drm_object_attach_property(&connector->base, connector->dev-

mode_config.hdr_output_metadata_property, 0);

On 02/03/2020 10:18, Laurent Pinchart wrote:

Hi Neil and Jonas,

(CC'ing Daniel for a framework question)

Thank you for the patch.

On Fri, Feb 21, 2020 at 09:50:18AM +0100, Neil Armstrong wrote:

...

On 17/02/2020 07:38, Jernej Škrabec wrote:

...

Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a):

...

From: Jonas Karlman jonas@kwiboo.se

Add the max_bpc property to the dw-hdmi connector to prepare support for 10, 12 & 16bit output support.

Signed-off-by: Jonas Karlman jonas@kwiboo.se Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 9e0927d22db6..051001f77dd4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) DRM_MODE_CONNECTOR_HDMIA, hdmi->ddc);

• drm_atomic_helper_connector_reset(connector);

Why is this reset needed?

I assume it's to allocate a new connector state to attach a the bpc propery.

But indeed, this helper is never used here, but only as callback to the drm_connector_funcs->reset.

But, amdgpu calls : /* * Some of the properties below require access to state, like bpc. * Allocate some default initial connector state with our reset helper. */ if (aconnector->base.funcs->reset) aconnector->base.funcs->reset(&aconnector->base);

which is the same.

A comment would be useful:

/* * drm_connector_attach_max_bpc_property() requires the * connector to have a state. */ drm_atomic_helper_connector_reset(connector);

drm_connector_attach_max_bpc_property(connector, 8, 16);

Done

I don't like this much though, it feels like the initial reset performed by drm_mode_config_reset() should set default values for all state members that are related to properties. Daniel, what's the rationale behind the current implementation ?

This is a DRM core issue that shouldn't block this patch though, so

I'll investigate why, but I haven't found out how the intel driver got the connector state initialized since they don't use the atomic helpers....

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

...

...

...

• drm_connector_attach_max_bpc_property(connector, 8, 16);
• if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) drm_object_attach_property(&connector->base, connector->dev-

mode_config.hdr_output_metadata_property, 0);

On Thu, 6 Feb 2020 20:18:26 +0100 Neil Armstrong narmstrong@baylibre.com wrote:

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

Reviewed-by: Boris Brezillon boris.brezillon@collabora.com

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 051001f77dd4..fec4a4bcd1fe 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2494,6 +2494,9 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) }

static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {

• .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
• .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach, .enable = dw_hdmi_bridge_enable,

On 02/03/2020 10:56, Laurent Pinchart wrote:

Hi Neil,

Thank you for the patch.

On Thu, Feb 06, 2020 at 08:18:26PM +0100, Neil Armstrong wrote:

...

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

How about adding a commit message ?

Done, thanks

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

...

---

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 051001f77dd4..fec4a4bcd1fe 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2494,6 +2494,9 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) }

static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {

• .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
• .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach, .enable = dw_hdmi_bridge_enable,

On Thu, 6 Feb 2020 20:18:27 +0100 Neil Armstrong narmstrong@baylibre.com wrote:

Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate

^ hooks?

the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

+/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4

It seems to only be 3 in practice (based on the code) unless I missed something.

+static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,

• 			struct drm_bridge_state *bridge_state,
  

• 			struct drm_crtc_state *crtc_state,
  

• 			struct drm_connector_state *conn_state,
  

• 			u32 output_fmt,
  

• 			unsigned int *num_input_fmts)
  

+{

• u32 *input_fmts;
• int i = 0;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• switch (output_fmt) {
• /* 8bit */
• case MEDIA_BUS_FMT_RGB888_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• break;
  

• case MEDIA_BUS_FMT_YUV8_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY8_1X16:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• /* 10bit */
• case MEDIA_BUS_FMT_RGB101010_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• break;
  

• case MEDIA_BUS_FMT_YUV10_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY10_1X20:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• /* 12bit */
• case MEDIA_BUS_FMT_RGB121212_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_YUV12_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY12_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• /* 16bit */
• case MEDIA_BUS_FMT_RGB161616_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• break;
  

• case MEDIA_BUS_FMT_YUV16_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• break;
  

• /* 420 */
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
• case MEDIA_BUS_FMT_UYYVYY16_0_5X48:

• input_fmts[i++] = output_fmt;
  

• break;
  

• }
• *num_input_fmts = i;
• if (*num_input_fmts == 0) {

• kfree(input_fmts);
  

• input_fmts = NULL;
  

• }
• return input_fmts;

+}

+static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,

• 		       struct drm_bridge_state *bridge_state,
  

• 		       struct drm_crtc_state *crtc_state,
  

• 		       struct drm_connector_state *conn_state)
  

+{

• struct dw_hdmi *hdmi = bridge->driver_private;
• dev_dbg(hdmi->dev, "selected output format %x\n",

• 	bridge_state->output_bus_cfg.format);
  

Nit: not aligned on the open parens.

• hdmi->hdmi_data.enc_out_bus_format =

• 	bridge_state->output_bus_cfg.format;
  

• dev_dbg(hdmi->dev, "selected input format %x\n",

• 	bridge_state->input_bus_cfg.format);
  

• hdmi->hdmi_data.enc_in_bus_format =

• 	bridge_state->input_bus_cfg.format;
  

• return 0;

+}

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) { struct dw_hdmi *hdmi = bridge->driver_private; @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach,

• .atomic_check = dw_hdmi_bridge_atomic_check,
• .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts,
• .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, .enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set,

@@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev,

hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;

Nit: not sure this has to be part of that patch.

Looks good otherwise.

Reviewed-by: Boris Brezillon boris.brezillon@collabora.com

#ifdef CONFIG_OF hdmi->bridge.of_node = pdev->dev.of_node; #endif

Hi Neil!

Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a):

Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- 1 file changed, 268 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index fec4a4bcd1fe..15048ad694bc 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2095,11 +2095,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;

• /* TOFIX: Get input format from plat data or fallback to RGB888 */ if (hdmi->plat_data->input_bus_format) hdmi->hdmi_data.enc_in_bus_format = hdmi->plat_data->input_bus_format;
• else

• else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) hdmi->hdmi_data.enc_in_bus_format =

MEDIA_BUS_FMT_RGB888_1X24;

/* TOFIX: Get input encoding from plat data or fallback to none */ @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) else hdmi->hdmi_data.enc_in_encoding =

V4L2_YCBCR_ENC_DEFAULT;

• /* TOFIX: Default to RGB888 output format */
• hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;

• if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED)

• hdmi->hdmi_data.enc_out_bus_format = 
  

MEDIA_BUS_FMT_RGB888_1X24;

hdmi->hdmi_data.pix_repet_factor = 0; hdmi->hdmi_data.hdcp_enable = 0; @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .atomic_check = dw_hdmi_connector_atomic_check, };

+/*

• • Possible output formats :
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48,
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36,
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30,
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24,
• • • MEDIA_BUS_FMT_YUV16_1X48,
• • • MEDIA_BUS_FMT_RGB161616_1X48,
• • • MEDIA_BUS_FMT_UYVY12_1X24,
• • • MEDIA_BUS_FMT_YUV12_1X36,
• • • MEDIA_BUS_FMT_RGB121212_1X36,
• • • MEDIA_BUS_FMT_UYVY10_1X20,
• • • MEDIA_BUS_FMT_YUV10_1X30,
• • • MEDIA_BUS_FMT_RGB101010_1X30,
• • • MEDIA_BUS_FMT_UYVY8_1X16,
• • • MEDIA_BUS_FMT_YUV8_1X24,
• • • MEDIA_BUS_FMT_RGB888_1X24,
• */

+/* Can return a maximum of 12 possible output formats for a mode/connector */ +#define MAX_OUTPUT_SEL_FORMATS 12

+static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

• 			struct drm_crtc_state 
  

*crtc_state,

• 			struct 
  

drm_connector_state *conn_state,

• 			unsigned int 
  

*num_output_fmts)

+{

• struct drm_connector *conn = conn_state->connector;
• struct drm_display_info *info = &conn->display_info;
• struct drm_display_mode *mode = &crtc_state->mode;
• u8 max_bpc = conn_state->max_requested_bpc;
• bool is_hdmi2_sink = info->hdmi.scdc.supported ||

• 	     (info->color_formats & 
  

DRM_COLOR_FORMAT_YCRCB420);

• u32 *output_fmts;
• int i = 0;
• *num_output_fmts = 0;
• output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts),

• 	      GFP_KERNEL);
  

• if (!output_fmts)

• return NULL;
  

• /*

• * If the current mode enforces 4:2:0, force the output but format
  

• * to 4:2:0 and do not add the YUV422/444/RGB formats
  

• */
  

• if (conn->ycbcr_420_allowed &&

•    (drm_mode_is_420_only(info, mode) ||
  

•     ())) {
  

• /* Order bus formats from 16bit to 8bit if supported */
  

• if (max_bpc >= 16 && info->bpc == 16 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_48))

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY16_0_5X48;

• if (max_bpc >= 12 && info->bpc >= 12 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_36))

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY12_0_5X36;

• if (max_bpc >= 10 && info->bpc >= 10 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_30))

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY10_0_5X30;

• /* Default 8bit fallback */
  

• output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
  

• *num_output_fmts = i;
  

• return output_fmts;
  

Driver shouldn't return just yet for case "is_hdmi2_sink && drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you think?

Best regards, Jernej

• }
• /*

• * Order bus formats from 16bit to 8bit and from YUV422 to RGB
  

• * if supported. In any case the default RGB888 format is added
  

• */
  

• if (max_bpc >= 16 && info->bpc == 16) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• }
• if (max_bpc >= 12 && info->bpc >= 12) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• }
• if (max_bpc >= 10 && info->bpc >= 10) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• }
• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)

• output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)

• output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• /* Default 8bit RGB fallback */
• output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
• *num_output_fmts = i;
• return output_fmts;

+}

+/*

• • Possible input formats :
• • • MEDIA_BUS_FMT_RGB888_1X24
• • • MEDIA_BUS_FMT_YUV8_1X24
• • • MEDIA_BUS_FMT_UYVY8_1X16
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24
• • • MEDIA_BUS_FMT_RGB101010_1X30
• • • MEDIA_BUS_FMT_YUV10_1X30
• • • MEDIA_BUS_FMT_UYVY10_1X20
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30
• • • MEDIA_BUS_FMT_RGB121212_1X36
• • • MEDIA_BUS_FMT_YUV12_1X36
• • • MEDIA_BUS_FMT_UYVY12_1X24
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36
• • • MEDIA_BUS_FMT_RGB161616_1X48
• • • MEDIA_BUS_FMT_YUV16_1X48
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48
• */

+/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4

+static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

• 			struct drm_crtc_state 
  

*crtc_state,

• 			struct 
  

drm_connector_state *conn_state,

• 			u32 output_fmt,
  

• 			unsigned int 
  

*num_input_fmts)

+{

• u32 *input_fmts;
• int i = 0;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• switch (output_fmt) {
• /* 8bit */
• case MEDIA_BUS_FMT_RGB888_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• break;
  

• case MEDIA_BUS_FMT_YUV8_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY8_1X16:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• /* 10bit */
• case MEDIA_BUS_FMT_RGB101010_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• break;
  

• case MEDIA_BUS_FMT_YUV10_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY10_1X20:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• /* 12bit */
• case MEDIA_BUS_FMT_RGB121212_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_YUV12_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY12_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• /* 16bit */
• case MEDIA_BUS_FMT_RGB161616_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• break;
  

• case MEDIA_BUS_FMT_YUV16_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• break;
  

• /* 420 */
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
• case MEDIA_BUS_FMT_UYYVYY16_0_5X48:

• input_fmts[i++] = output_fmt;
  

• break;
  

• }
• *num_input_fmts = i;
• if (*num_input_fmts == 0) {

• kfree(input_fmts);
  

• input_fmts = NULL;
  

• }
• return input_fmts;

+}

+static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,

• 		       struct drm_bridge_state 
  

*bridge_state,

• 		       struct drm_crtc_state 
  

*crtc_state,

• 		       struct drm_connector_state 
  

*conn_state)

+{

• struct dw_hdmi *hdmi = bridge->driver_private;
• dev_dbg(hdmi->dev, "selected output format %x\n",

• 	bridge_state->output_bus_cfg.format);
  

• hdmi->hdmi_data.enc_out_bus_format =

• 	bridge_state->output_bus_cfg.format;
  

• dev_dbg(hdmi->dev, "selected input format %x\n",

• 	bridge_state->input_bus_cfg.format);
  

• hdmi->hdmi_data.enc_in_bus_format =

• 	bridge_state->input_bus_cfg.format;
  

• return 0;

+}

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) { struct dw_hdmi *hdmi = bridge->driver_private; @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach,

• .atomic_check = dw_hdmi_bridge_atomic_check,
• .atomic_get_output_bus_fmts =

dw_hdmi_bridge_atomic_get_output_bus_fmts,

• .atomic_get_input_bus_fmts =

dw_hdmi_bridge_atomic_get_input_bus_fmts,

.enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set, @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev,

hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;

#ifdef CONFIG_OF hdmi->bridge.of_node = pdev->dev.of_node; #endif

Dne sobota, 29. februar 2020 ob 11:09:14 CET je Jonas Karlman napisal(a):

Hi Jernej,

On 2020-02-29 08:42, Jernej Škrabec wrote:

...

Hi Neil!

Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong

napisal(a):

...

...

Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- 1 file changed, 268 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index fec4a4bcd1fe..15048ad694bc 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2095,11 +2095,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0;

hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;

• /* TOFIX: Get input format from plat data or fallback to RGB888 */

  if (hdmi->plat_data->input_bus_format)

  hdmi->hdmi_data.enc_in_bus_format =

  hdmi->plat_data->input_bus_format;
  
  

• else

• else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED)

  hdmi->hdmi_data.enc_in_bus_format =

MEDIA_BUS_FMT_RGB888_1X24;

...

/* TOFIX: Get input encoding from plat data or fallback to none */

@@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) else

hdmi->hdmi_data.enc_in_encoding =

V4L2_YCBCR_ENC_DEFAULT;

...

• /* TOFIX: Default to RGB888 output format */
• hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;

• if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED)

• hdmi->hdmi_data.enc_out_bus_format =
  

MEDIA_BUS_FMT_RGB888_1X24;

...

hdmi->hdmi_data.pix_repet_factor = 0; hdmi->hdmi_data.hdcp_enable = 0;

@@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .atomic_check = dw_hdmi_connector_atomic_check,

};

+/*

• • Possible output formats :
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48,
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36,
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30,
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24,
• • • MEDIA_BUS_FMT_YUV16_1X48,
• • • MEDIA_BUS_FMT_RGB161616_1X48,
• • • MEDIA_BUS_FMT_UYVY12_1X24,
• • • MEDIA_BUS_FMT_YUV12_1X36,
• • • MEDIA_BUS_FMT_RGB121212_1X36,
• • • MEDIA_BUS_FMT_UYVY10_1X20,
• • • MEDIA_BUS_FMT_YUV10_1X30,
• • • MEDIA_BUS_FMT_RGB101010_1X30,
• • • MEDIA_BUS_FMT_UYVY8_1X16,
• • • MEDIA_BUS_FMT_YUV8_1X24,
• • • MEDIA_BUS_FMT_RGB888_1X24,
• */

+/* Can return a maximum of 12 possible output formats for a mode/connector */ +#define MAX_OUTPUT_SEL_FORMATS 12

+static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state
  

*crtc_state,

...

• 			struct
  

drm_connector_state *conn_state,

...

• 			unsigned int
  

*num_output_fmts)

...

+{

• struct drm_connector *conn = conn_state->connector;
• struct drm_display_info *info = &conn->display_info;
• struct drm_display_mode *mode = &crtc_state->mode;
• u8 max_bpc = conn_state->max_requested_bpc;
• bool is_hdmi2_sink = info->hdmi.scdc.supported ||

• 	     (info->color_formats &
  

DRM_COLOR_FORMAT_YCRCB420);

...

• u32 *output_fmts;
• int i = 0;
• *num_output_fmts = 0;
• output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS,

sizeof(*output_fmts),

...

...

• 	      GFP_KERNEL);
  

• if (!output_fmts)

• return NULL;
  

• /*

• * If the current mode enforces 4:2:0, force the output but format
  

• * to 4:2:0 and do not add the YUV422/444/RGB formats
  

• */
  

• if (conn->ycbcr_420_allowed &&

•    (drm_mode_is_420_only(info, mode) ||
  

•     ())) {
  

• /* Order bus formats from 16bit to 8bit if supported */
  

• if (max_bpc >= 16 && info->bpc == 16 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_48))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY16_0_5X48;

...

• if (max_bpc >= 12 && info->bpc >= 12 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_36))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY12_0_5X36;

...

• if (max_bpc >= 10 && info->bpc >= 10 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_30))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY10_0_5X30;

...

• /* Default 8bit fallback */
  

• output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
  

• *num_output_fmts = i;
  

• return output_fmts;
  

Driver shouldn't return just yet for case "is_hdmi2_sink && drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you think?

I think we need to have some way for controller driver and userspace to control what hdmi output format gets selected. I know for a fact that some Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no problems with 420 modes. The Samsung TV edid lie and/or the TV is not fully following HDMI specs

Interesting, maybe just some bandwith issues? I guess we should have a blacklist for such cases. I know that at least Allwinner BSP driver for DW HDMI has a blacklist, in this case for 2 monitors which claim to support YCbCr 4:4:4 mode but they not.

From a personal and mediaplayer userspace perspective I would like to prefer 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 anytime else.

I would argue that YCbCr is always prefered: - CEA 861 prefers it for all CEA modes - avoid dealing with quantization range - it's always limited range for < hdmi 2.0 and selectable on some hdmi 2.0 capable sinks

Anyway, there is no universal solution to avoid color space conversion in all cases. For example, due to design of Allwinner Display Engine 2, it can only work in RGB format internally, but it can convert final output to YCbCr right before it's feeded to DW HDMI. On the other hand, you have meson display pipeline which works in YCbCr format internally and relies on DW HDMI CSC unit to convert output to RGB. Fortunately, there are also display pipelines which can work in any colorspace internally, like Allwinner Display Engine 3. Not sure in which category Rockchip display pipeline falls into.

On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi block, the optimal output format selection in such case should put yuv422 last.

Note that DW HDMI has decimation support which converts 444 to 422. This is part of CSC unit.

Side note: CSC unit is optional feature of DW-HDMI and presence is indicated by config register. However, it seems that CSC support is broken in DW HDMI controller on 40nm Allwinner SoCs. If it is enabled, TV loses signal. I have to investigate if only CSC is broken or decimation also don't work.

Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred output format order?

On a side note but related issue, the dw-hdmi format negotiation code should probably also filter modes on tmds rate, something like [1]. It is needed to filter out deep color modes that is not supported by the sink or hdmi spec.

Ah, you mean on TMDS rates supported by the sink. Could this avoid Samsung deep color issues? If so, we don't need blacklist then.

Best regards, Jernej

[1] https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac59879c 489cbef55fcbe

Best regards, Jonas

...

Best regards, Jernej

...

• }
• /*

• * Order bus formats from 16bit to 8bit and from YUV422 to RGB
  

• * if supported. In any case the default RGB888 format is added
  

• */
  

• if (max_bpc >= 16 && info->bpc == 16) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• }
• if (max_bpc >= 12 && info->bpc >= 12) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYVY12_1X24;

...

...

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• }
• if (max_bpc >= 10 && info->bpc >= 10) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYVY10_1X20;

...

...

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• }
• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)

• output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)

• output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• /* Default 8bit RGB fallback */
• output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
• *num_output_fmts = i;
• return output_fmts;

+}

+/*

• • Possible input formats :
• • • MEDIA_BUS_FMT_RGB888_1X24
• • • MEDIA_BUS_FMT_YUV8_1X24
• • • MEDIA_BUS_FMT_UYVY8_1X16
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24
• • • MEDIA_BUS_FMT_RGB101010_1X30
• • • MEDIA_BUS_FMT_YUV10_1X30
• • • MEDIA_BUS_FMT_UYVY10_1X20
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30
• • • MEDIA_BUS_FMT_RGB121212_1X36
• • • MEDIA_BUS_FMT_YUV12_1X36
• • • MEDIA_BUS_FMT_UYVY12_1X24
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36
• • • MEDIA_BUS_FMT_RGB161616_1X48
• • • MEDIA_BUS_FMT_YUV16_1X48
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48
• */

+/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4

+static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state
  

*crtc_state,

...

• 			struct
  

drm_connector_state *conn_state,

...

• 			u32 output_fmt,
  

• 			unsigned int
  

*num_input_fmts)

...

+{

• u32 *input_fmts;
• int i = 0;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• switch (output_fmt) {
• /* 8bit */
• case MEDIA_BUS_FMT_RGB888_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• break;
  

• case MEDIA_BUS_FMT_YUV8_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY8_1X16:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• /* 10bit */
• case MEDIA_BUS_FMT_RGB101010_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• break;
  

• case MEDIA_BUS_FMT_YUV10_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY10_1X20:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• /* 12bit */
• case MEDIA_BUS_FMT_RGB121212_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_YUV12_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY12_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• /* 16bit */
• case MEDIA_BUS_FMT_RGB161616_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• break;
  

• case MEDIA_BUS_FMT_YUV16_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• break;
  

• /* 420 */
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
• case MEDIA_BUS_FMT_UYYVYY16_0_5X48:

• input_fmts[i++] = output_fmt;
  

• break;
  

• }
• *num_input_fmts = i;
• if (*num_input_fmts == 0) {

• kfree(input_fmts);
  

• input_fmts = NULL;
  

• }
• return input_fmts;

+}

+static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,

• 		       struct drm_bridge_state
  

*bridge_state,

...

• 		       struct drm_crtc_state
  

*crtc_state,

...

• 		       struct drm_connector_state
  

*conn_state)

...

+{

• struct dw_hdmi *hdmi = bridge->driver_private;
• dev_dbg(hdmi->dev, "selected output format %x\n",

• 	bridge_state->output_bus_cfg.format);
  

• hdmi->hdmi_data.enc_out_bus_format =

• 	bridge_state->output_bus_cfg.format;
  

• dev_dbg(hdmi->dev, "selected input format %x\n",

• 	bridge_state->input_bus_cfg.format);
  

• hdmi->hdmi_data.enc_in_bus_format =

• 	bridge_state->input_bus_cfg.format;
  

• return 0;

+}

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) {

struct dw_hdmi *hdmi = bridge->driver_private;

@@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset,

.attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach,

• .atomic_check = dw_hdmi_bridge_atomic_check,
• .atomic_get_output_bus_fmts =

dw_hdmi_bridge_atomic_get_output_bus_fmts,

...

• .atomic_get_input_bus_fmts =

dw_hdmi_bridge_atomic_get_input_bus_fmts,

...

.enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set,

@@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev,

hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;

#ifdef CONFIG_OF

hdmi->bridge.of_node = pdev->dev.of_node;

#endif

Dne sobota, 29. februar 2020 ob 13:15:26 CET je Jonas Karlman napisal(a):

On 2020-02-29 12:07, Jernej Škrabec wrote:

...

Dne sobota, 29. februar 2020 ob 11:09:14 CET je Jonas Karlman napisal(a):

...

Hi Jernej,

On 2020-02-29 08:42, Jernej Škrabec wrote:

...

Hi Neil!

Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong

napisal(a):

...

...

...

Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- 1 file changed, 268 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index fec4a4bcd1fe..15048ad694bc 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2095,11 +2095,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0;

hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;

• /* TOFIX: Get input format from plat data or fallback to RGB888 */

  if (hdmi->plat_data->input_bus_format)

  hdmi->hdmi_data.enc_in_bus_format =

  hdmi->plat_data->input_bus_format;
  
  

• else

• else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED)

  hdmi->hdmi_data.enc_in_bus_format =

MEDIA_BUS_FMT_RGB888_1X24;

...

/* TOFIX: Get input encoding from plat data or fallback to none */

@@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) else

hdmi->hdmi_data.enc_in_encoding =

V4L2_YCBCR_ENC_DEFAULT;

...

• /* TOFIX: Default to RGB888 output format */
• hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;

• if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED)

• hdmi->hdmi_data.enc_out_bus_format =
  

MEDIA_BUS_FMT_RGB888_1X24;

...

hdmi->hdmi_data.pix_repet_factor = 0; hdmi->hdmi_data.hdcp_enable = 0;

@@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .atomic_check = dw_hdmi_connector_atomic_check,

};

+/*

• • Possible output formats :
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48,
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36,
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30,
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24,
• • • MEDIA_BUS_FMT_YUV16_1X48,
• • • MEDIA_BUS_FMT_RGB161616_1X48,
• • • MEDIA_BUS_FMT_UYVY12_1X24,
• • • MEDIA_BUS_FMT_YUV12_1X36,
• • • MEDIA_BUS_FMT_RGB121212_1X36,
• • • MEDIA_BUS_FMT_UYVY10_1X20,
• • • MEDIA_BUS_FMT_YUV10_1X30,
• • • MEDIA_BUS_FMT_RGB101010_1X30,
• • • MEDIA_BUS_FMT_UYVY8_1X16,
• • • MEDIA_BUS_FMT_YUV8_1X24,
• • • MEDIA_BUS_FMT_RGB888_1X24,
• */

+/* Can return a maximum of 12 possible output formats for a mode/connector */ +#define MAX_OUTPUT_SEL_FORMATS 12

+static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state
  

*crtc_state,

...

• 			struct
  

drm_connector_state *conn_state,

...

• 			unsigned int
  

*num_output_fmts)

...

+{

• struct drm_connector *conn = conn_state->connector;
• struct drm_display_info *info = &conn->display_info;
• struct drm_display_mode *mode = &crtc_state->mode;
• u8 max_bpc = conn_state->max_requested_bpc;
• bool is_hdmi2_sink = info->hdmi.scdc.supported ||

• 	     (info->color_formats &
  

DRM_COLOR_FORMAT_YCRCB420);

...

• u32 *output_fmts;
• int i = 0;
• *num_output_fmts = 0;
• output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS,

sizeof(*output_fmts),

...

...

...

• 	      GFP_KERNEL);
  

• if (!output_fmts)

• return NULL;
  

• /*

• * If the current mode enforces 4:2:0, force the output but format
  

• * to 4:2:0 and do not add the YUV422/444/RGB formats
  

• */
  

• if (conn->ycbcr_420_allowed &&

•    (drm_mode_is_420_only(info, mode) ||
  

•     ())) {
  

• /* Order bus formats from 16bit to 8bit if supported */
  

• if (max_bpc >= 16 && info->bpc == 16 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_48))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY16_0_5X48;

...

• if (max_bpc >= 12 && info->bpc >= 12 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_36))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY12_0_5X36;

...

• if (max_bpc >= 10 && info->bpc >= 10 &&
  

•     (info->hdmi.y420_dc_modes &
  

DRM_EDID_YCBCR420_DC_30))

...

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYYVYY10_0_5X30;

...

• /* Default 8bit fallback */
  

• output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
  

• *num_output_fmts = i;
  

• return output_fmts;
  

Driver shouldn't return just yet for case "is_hdmi2_sink && drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you think?

I think we need to have some way for controller driver and userspace to control what hdmi output format gets selected. I know for a fact that some Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no problems with 420 modes. The Samsung TV edid lie and/or the TV is not fully following HDMI specs

Interesting, maybe just some bandwith issues? I guess we should have a blacklist for such cases. I know that at least Allwinner BSP driver for DW HDMI has a blacklist, in this case for 2 monitors which claim to support YCbCr 4:4:4 mode but they not.

...

From a personal and mediaplayer userspace perspective I would like to prefer 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 anytime else.

I would argue that YCbCr is always prefered:

• CEA 861 prefers it for all CEA modes
• avoid dealing with quantization range - it's always limited range for <

hdmi 2.0 and selectable on some hdmi 2.0 capable sinks

This is probably true if your sink is a TV, for monitors with HDMI connections I would expect or at least like to have an option to use rgb444 full range mode for any desktop or gaming or similar use-case.

Well, thing is that CEA 861 clearly states that for CEA modes (720p, 1080p, 2160p, etc.) limited range should be also used for RGB color space. With implemented logic to detect if RGB limited range is needed or not and indicating that in AVI frame, I finally get proper (better?) color reproduction with PC monitor and TV at the same time. I'm not sure if there is any way to detect "full range RGB" capable displays. However, we can always implement connector property like Intel did in i915 driver "Broadcast RGB", which can override RGB quantization mode.

...

Anyway, there is no universal solution to avoid color space conversion in all cases. For example, due to design of Allwinner Display Engine 2, it can only work in RGB format internally, but it can convert final output to YCbCr right before it's feeded to DW HDMI. On the other hand, you have meson display pipeline which works in YCbCr format internally and relies on DW HDMI CSC unit to convert output to RGB. Fortunately, there are also display pipelines which can work in any colorspace internally, like Allwinner Display Engine 3. Not sure in which category Rockchip display pipeline falls into.

Agree, and for Rockchip to my knowledge RK3288 VOP (Video Output Processor) can only output 8/10-bit rgb (full range) to DW-HDMI. And the VOP in RK322x/RK3328/RK3399 can output 8/10-bit rgb and 444/420 yuv. I do not know how the VOP handles internal color conversion.

...

...

On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi block, the optimal output format selection in such case should put yuv422 last.

Note that DW HDMI has decimation support which converts 444 to 422. This is part of CSC unit.

This works as it should (after a patch to disable color conversion for 444 to 422),

Yeah, I noticed that too, I plan to send a patch for that soon.

the issue I see is that the format negotiation code will currently favor 444 to 422 decimation over using 444 output. Note: 422 will not be selected over 444 currently in case sink report deep color support due to drm edid parsing code following HDMI 1.3 spec, see [1].

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_edid.c#L4 801-L4806

...

Side note: CSC unit is optional feature of DW-HDMI and presence is indicated by config register. However, it seems that CSC support is broken in DW HDMI controller on 40nm Allwinner SoCs. If it is enabled, TV loses signal. I have to investigate if only CSC is broken or decimation also don't work.>

Scratch above, DW HDMI CSC unit works ok on Allwinner SoCs, I just forgot to enable CSC clock.

...

...

Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred output format order?

On a side note but related issue, the dw-hdmi format negotiation code should probably also filter modes on tmds rate, something like [1]. It is needed to filter out deep color modes that is not supported by the sink or hdmi spec.

Ah, you mean on TMDS rates supported by the sink. Could this avoid Samsung deep color issues? If so, we don't need blacklist then.

I do not think so, the initial issue was reported with intel graphics but looking at the edid and comparing to the manual of supported modes they do not fully match. See manual info at [2] and edid when UHD mode is off at [3] and when on at [4].

Which part doesn't match? I don't see any issue.

Best regards, Jernej

[2] https://gitlab.freedesktop.org/drm/intel/uploads/36630c8a727bf8c66a53fd7470 c88383/MU7000_manual_-_UHD_colour_modes.PNG [3] http://ix.io/1H25 [4] http://ix.io/1H2e

...

Best regards, Jernej

...

[1] https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac5987 9c 489cbef55fcbe

Best regards, Jonas

...

Best regards, Jernej

...

• }
• /*

• * Order bus formats from 16bit to 8bit and from YUV422 to RGB
  

• * if supported. In any case the default RGB888 format is added
  

• */
  

• if (max_bpc >= 16 && info->bpc == 16) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• }
• if (max_bpc >= 12 && info->bpc >= 12) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYVY12_1X24;

...

...

...

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• }
• if (max_bpc >= 10 && info->bpc >= 10) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] =
  

MEDIA_BUS_FMT_UYVY10_1X20;

...

...

...

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• }
• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)

• output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)

• output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• /* Default 8bit RGB fallback */
• output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
• *num_output_fmts = i;
• return output_fmts;

+}

+/*

• • Possible input formats :
• • • MEDIA_BUS_FMT_RGB888_1X24
• • • MEDIA_BUS_FMT_YUV8_1X24
• • • MEDIA_BUS_FMT_UYVY8_1X16
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24
• • • MEDIA_BUS_FMT_RGB101010_1X30
• • • MEDIA_BUS_FMT_YUV10_1X30
• • • MEDIA_BUS_FMT_UYVY10_1X20
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30
• • • MEDIA_BUS_FMT_RGB121212_1X36
• • • MEDIA_BUS_FMT_YUV12_1X36
• • • MEDIA_BUS_FMT_UYVY12_1X24
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36
• • • MEDIA_BUS_FMT_RGB161616_1X48
• • • MEDIA_BUS_FMT_YUV16_1X48
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48
• */

+/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4

+static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state
  

*crtc_state,

...

• 			struct
  

drm_connector_state *conn_state,

...

• 			u32 output_fmt,
  

• 			unsigned int
  

*num_input_fmts)

...

+{

• u32 *input_fmts;
• int i = 0;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• switch (output_fmt) {
• /* 8bit */
• case MEDIA_BUS_FMT_RGB888_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• break;
  

• case MEDIA_BUS_FMT_YUV8_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY8_1X16:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• /* 10bit */
• case MEDIA_BUS_FMT_RGB101010_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• break;
  

• case MEDIA_BUS_FMT_YUV10_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY10_1X20:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• /* 12bit */
• case MEDIA_BUS_FMT_RGB121212_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_YUV12_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY12_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• /* 16bit */
• case MEDIA_BUS_FMT_RGB161616_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• break;
  

• case MEDIA_BUS_FMT_YUV16_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• break;
  

• /* 420 */
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
• case MEDIA_BUS_FMT_UYYVYY16_0_5X48:

• input_fmts[i++] = output_fmt;
  

• break;
  

• }
• *num_input_fmts = i;
• if (*num_input_fmts == 0) {

• kfree(input_fmts);
  

• input_fmts = NULL;
  

• }
• return input_fmts;

+}

+static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,

• 		       struct drm_bridge_state
  

*bridge_state,

...

• 		       struct drm_crtc_state
  

*crtc_state,

...

• 		       struct drm_connector_state
  

*conn_state)

...

+{

• struct dw_hdmi *hdmi = bridge->driver_private;
• dev_dbg(hdmi->dev, "selected output format %x\n",

• 	bridge_state->output_bus_cfg.format);
  

• hdmi->hdmi_data.enc_out_bus_format =

• 	bridge_state->output_bus_cfg.format;
  

• dev_dbg(hdmi->dev, "selected input format %x\n",

• 	bridge_state->input_bus_cfg.format);
  

• hdmi->hdmi_data.enc_in_bus_format =

• 	bridge_state->input_bus_cfg.format;
  

• return 0;

+}

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) {

struct dw_hdmi *hdmi = bridge->driver_private;

@@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset,

.attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach,

• .atomic_check = dw_hdmi_bridge_atomic_check,
• .atomic_get_output_bus_fmts =

dw_hdmi_bridge_atomic_get_output_bus_fmts,

...

• .atomic_get_input_bus_fmts =

dw_hdmi_bridge_atomic_get_input_bus_fmts,

...

.enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set,

@@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev,

hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;

#ifdef CONFIG_OF

hdmi->bridge.of_node = pdev->dev.of_node;

#endif

On 29/02/2020 08:42, Jernej Škrabec wrote:

Hi Neil!

Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a):

...

Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- 1 file changed, 268 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index fec4a4bcd1fe..15048ad694bc 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2095,11 +2095,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;

• /* TOFIX: Get input format from plat data or fallback to RGB888 */ if (hdmi->plat_data->input_bus_format) hdmi->hdmi_data.enc_in_bus_format = hdmi->plat_data->input_bus_format;
• else

• else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) hdmi->hdmi_data.enc_in_bus_format =

MEDIA_BUS_FMT_RGB888_1X24;

...

/* TOFIX: Get input encoding from plat data or fallback to none */ @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) else hdmi->hdmi_data.enc_in_encoding =

V4L2_YCBCR_ENC_DEFAULT;

...

• /* TOFIX: Default to RGB888 output format */
• hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;

• if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED)

• hdmi->hdmi_data.enc_out_bus_format = 
  

MEDIA_BUS_FMT_RGB888_1X24;

...

hdmi->hdmi_data.pix_repet_factor = 0; hdmi->hdmi_data.hdcp_enable = 0; @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .atomic_check = dw_hdmi_connector_atomic_check, };

+/*

• • Possible output formats :
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48,
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36,
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30,
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24,
• • • MEDIA_BUS_FMT_YUV16_1X48,
• • • MEDIA_BUS_FMT_RGB161616_1X48,
• • • MEDIA_BUS_FMT_UYVY12_1X24,
• • • MEDIA_BUS_FMT_YUV12_1X36,
• • • MEDIA_BUS_FMT_RGB121212_1X36,
• • • MEDIA_BUS_FMT_UYVY10_1X20,
• • • MEDIA_BUS_FMT_YUV10_1X30,
• • • MEDIA_BUS_FMT_RGB101010_1X30,
• • • MEDIA_BUS_FMT_UYVY8_1X16,
• • • MEDIA_BUS_FMT_YUV8_1X24,
• • • MEDIA_BUS_FMT_RGB888_1X24,
• */

+/* Can return a maximum of 12 possible output formats for a mode/connector */ +#define MAX_OUTPUT_SEL_FORMATS 12

+static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state 
  

*crtc_state,

...

• 			struct 
  

drm_connector_state *conn_state,

...

• 			unsigned int 
  

*num_output_fmts)

...

+{

• struct drm_connector *conn = conn_state->connector;
• struct drm_display_info *info = &conn->display_info;
• struct drm_display_mode *mode = &crtc_state->mode;
• u8 max_bpc = conn_state->max_requested_bpc;
• bool is_hdmi2_sink = info->hdmi.scdc.supported ||

• 	     (info->color_formats & 
  

DRM_COLOR_FORMAT_YCRCB420);

...

• u32 *output_fmts;
• int i = 0;
• *num_output_fmts = 0;
• output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts),

• 	      GFP_KERNEL);
  

• if (!output_fmts)

• return NULL;
  

• /*

• * If the current mode enforces 4:2:0, force the output but format
  

• * to 4:2:0 and do not add the YUV422/444/RGB formats
  

• */
  

• if (conn->ycbcr_420_allowed &&

•    (drm_mode_is_420_only(info, mode) ||
  

•     ())) {
  

• /* Order bus formats from 16bit to 8bit if supported */
  

• if (max_bpc >= 16 && info->bpc == 16 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_48))

...

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY16_0_5X48;

...

• if (max_bpc >= 12 && info->bpc >= 12 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_36))

...

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY12_0_5X36;

...

• if (max_bpc >= 10 && info->bpc >= 10 &&
  

•     (info->hdmi.y420_dc_modes & 
  

DRM_EDID_YCBCR420_DC_30))

...

• 	output_fmts[i++] = 
  

MEDIA_BUS_FMT_UYYVYY10_0_5X30;

...

• /* Default 8bit fallback */
  

• output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
  

• *num_output_fmts = i;
  

• return output_fmts;
  

Driver shouldn't return just yet for case "is_hdmi2_sink && drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you think?

The code is right.

If drm_mode_is_420_only(info, mode), then the selected mode is 4:2:0 only, so only 4:2:0 bus formats will be allowed in this case.

In this 4:2:0 only mode, the TV only support 4:2:0 4k60 modes and doesn't support SCDC and high tmds frequency pattern & co.

Neil

Best regards, Jernej

...

• }
• /*

• * Order bus formats from 16bit to 8bit and from YUV422 to RGB
  

• * if supported. In any case the default RGB888 format is added
  

• */
  

• if (max_bpc >= 16 && info->bpc == 16) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• }
• if (max_bpc >= 12 && info->bpc >= 12) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• }
• if (max_bpc >= 10 && info->bpc >= 10) {

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
  

• 	output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• }
• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)

• output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)

• output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• /* Default 8bit RGB fallback */
• output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
• *num_output_fmts = i;
• return output_fmts;

+}

+/*

• • Possible input formats :
• • • MEDIA_BUS_FMT_RGB888_1X24
• • • MEDIA_BUS_FMT_YUV8_1X24
• • • MEDIA_BUS_FMT_UYVY8_1X16
• • • MEDIA_BUS_FMT_UYYVYY8_0_5X24
• • • MEDIA_BUS_FMT_RGB101010_1X30
• • • MEDIA_BUS_FMT_YUV10_1X30
• • • MEDIA_BUS_FMT_UYVY10_1X20
• • • MEDIA_BUS_FMT_UYYVYY10_0_5X30
• • • MEDIA_BUS_FMT_RGB121212_1X36
• • • MEDIA_BUS_FMT_YUV12_1X36
• • • MEDIA_BUS_FMT_UYVY12_1X24
• • • MEDIA_BUS_FMT_UYYVYY12_0_5X36
• • • MEDIA_BUS_FMT_RGB161616_1X48
• • • MEDIA_BUS_FMT_YUV16_1X48
• • • MEDIA_BUS_FMT_UYYVYY16_0_5X48
• */

+/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4

+static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct

drm_bridge_state *bridge_state,

...

• 			struct drm_crtc_state 
  

*crtc_state,

...

• 			struct 
  

drm_connector_state *conn_state,

...

• 			u32 output_fmt,
  

• 			unsigned int 
  

*num_input_fmts)

...

+{

• u32 *input_fmts;
• int i = 0;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• switch (output_fmt) {
• /* 8bit */
• case MEDIA_BUS_FMT_RGB888_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• break;
  

• case MEDIA_BUS_FMT_YUV8_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY8_1X16:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
  

• break;
  

• /* 10bit */
• case MEDIA_BUS_FMT_RGB101010_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• break;
  

• case MEDIA_BUS_FMT_YUV10_1X30:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY10_1X20:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
  

• break;
  

• /* 12bit */
• case MEDIA_BUS_FMT_RGB121212_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• break;
  

• case MEDIA_BUS_FMT_YUV12_1X36:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• case MEDIA_BUS_FMT_UYVY12_1X24:

• input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
  

• break;
  

• /* 16bit */
• case MEDIA_BUS_FMT_RGB161616_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• break;
  

• case MEDIA_BUS_FMT_YUV16_1X48:

• input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  

• input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
  

• break;
  

• /* 420 */
• case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
• case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
• case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
• case MEDIA_BUS_FMT_UYYVYY16_0_5X48:

• input_fmts[i++] = output_fmt;
  

• break;
  

• }
• *num_input_fmts = i;
• if (*num_input_fmts == 0) {

• kfree(input_fmts);
  

• input_fmts = NULL;
  

• }
• return input_fmts;

+}

+static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,

• 		       struct drm_bridge_state 
  

*bridge_state,

...

• 		       struct drm_crtc_state 
  

*crtc_state,

...

• 		       struct drm_connector_state 
  

*conn_state)

...

+{

• struct dw_hdmi *hdmi = bridge->driver_private;
• dev_dbg(hdmi->dev, "selected output format %x\n",

• 	bridge_state->output_bus_cfg.format);
  

• hdmi->hdmi_data.enc_out_bus_format =

• 	bridge_state->output_bus_cfg.format;
  

• dev_dbg(hdmi->dev, "selected input format %x\n",

• 	bridge_state->input_bus_cfg.format);
  

• hdmi->hdmi_data.enc_in_bus_format =

• 	bridge_state->input_bus_cfg.format;
  

• return 0;

+}

static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) { struct dw_hdmi *hdmi = bridge->driver_private; @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach,

• .atomic_check = dw_hdmi_bridge_atomic_check,
• .atomic_get_output_bus_fmts =

dw_hdmi_bridge_atomic_get_output_bus_fmts,

...

• .atomic_get_input_bus_fmts =

dw_hdmi_bridge_atomic_get_input_bus_fmts,

...

.enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set, @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev,

hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;

#ifdef CONFIG_OF hdmi->bridge.of_node = pdev->dev.of_node; #endif