From: Neil Armstrong <[email protected]>
The dw-hdmi integrates an optional Color Space Conversion feature used
to handle color-space conversions.
On some platforms, the CSC isn't built-in or non-functional.
This adds the necessary code to disable the CSC functionality
and limit the bus format negotiation to force using the same
input bus format as the output bus format.
Signed-off-by: Neil Armstrong <[email protected]>
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 100 +++++++++++++++-------
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 1 +
include/drm/bridge/dw_hdmi.h | 1 +
3 files changed, 71 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f50af40e10340..b5a665c5e406e 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -158,6 +158,8 @@ struct dw_hdmi {
struct hdmi_data_info hdmi_data;
const struct dw_hdmi_plat_data *plat_data;
+ bool csc_available; /* indicates if the CSC engine is usable */
+
int vic;
u8 edid[HDMI_EDID_LEN];
@@ -1009,9 +1011,10 @@ static int is_color_space_interpolation(struct dw_hdmi *hdmi)
static bool is_csc_needed(struct dw_hdmi *hdmi)
{
- return is_color_space_conversion(hdmi) ||
- is_color_space_decimation(hdmi) ||
- is_color_space_interpolation(hdmi);
+ return hdmi->csc_available &&
+ (is_color_space_conversion(hdmi) ||
+ is_color_space_decimation(hdmi) ||
+ is_color_space_interpolation(hdmi));
}
static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
@@ -1064,6 +1067,9 @@ static void hdmi_video_csc(struct dw_hdmi *hdmi)
int interpolation = HDMI_CSC_CFG_INTMODE_DISABLE;
int decimation = 0;
+ if (!hdmi->csc_available)
+ return;
+
/* YCC422 interpolation to 444 mode */
if (is_color_space_interpolation(hdmi))
interpolation = HDMI_CSC_CFG_INTMODE_CHROMA_INT_FORMULA1;
@@ -2665,6 +2671,7 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
u32 output_fmt,
unsigned int *num_input_fmts)
{
+ struct dw_hdmi *hdmi = bridge->driver_private;
u32 *input_fmts;
unsigned int i = 0;
@@ -2683,62 +2690,81 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
/* 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available) {
+ 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;
+ if (hdmi->csc_available)
+ 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;
+ if (hdmi->csc_available)
+ input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
break;
/*YUV 4:2:0 */
@@ -2767,15 +2793,24 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
{
struct dw_hdmi *hdmi = bridge->driver_private;
- hdmi->hdmi_data.enc_out_bus_format =
- bridge_state->output_bus_cfg.format;
+ if (!hdmi->csc_available &&
+ bridge_state->output_bus_cfg.format != bridge_state->input_bus_cfg.format) {
+ dev_warn(hdmi->dev, "different input format 0x%04x & output format 0x%04x while CSC isn't usable, fallback to safe format\n",
+ bridge_state->input_bus_cfg.format,
+ bridge_state->output_bus_cfg.format);
+ hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_FIXED;
+ hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_FIXED;
+ } else {
+ hdmi->hdmi_data.enc_out_bus_format =
+ bridge_state->output_bus_cfg.format;
- hdmi->hdmi_data.enc_in_bus_format =
- bridge_state->input_bus_cfg.format;
+ hdmi->hdmi_data.enc_in_bus_format =
+ bridge_state->input_bus_cfg.format;
- dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
- bridge_state->input_bus_cfg.format,
- bridge_state->output_bus_cfg.format);
+ dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
+ bridge_state->input_bus_cfg.format,
+ bridge_state->output_bus_cfg.format);
+ }
return 0;
}
@@ -3481,6 +3516,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
hdmi->cec = platform_device_register_full(&pdevinfo);
}
+ /* Get CSC useability from config0 register and permit override for platforms */
+ hdmi->csc_available = !plat_data->disable_csc || (config0 & HDMI_CONFIG0_CSC);
+
drm_bridge_add(&hdmi->bridge);
return hdmi;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 1999db05bc3b2..279722e4d1898 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -541,6 +541,7 @@ enum {
/* CONFIG0_ID field values */
HDMI_CONFIG0_I2S = 0x10,
+ HDMI_CONFIG0_CSC = 0x04,
HDMI_CONFIG0_CEC = 0x02,
/* CONFIG1_ID field values */
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 2a1f85f9a8a3f..b2f689cbe864c 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -157,6 +157,7 @@ struct dw_hdmi_plat_data {
unsigned long mpixelclock);
unsigned int disable_cec : 1;
+ unsigned int disable_csc : 1;
};
struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
--
2.33.0
Hi,
On 06/04/2022 18:26, H. Nikolaus Schaller wrote:
> From: Neil Armstrong <[email protected]>
>
> The dw-hdmi integrates an optional Color Space Conversion feature used
> to handle color-space conversions.
>
> On some platforms, the CSC isn't built-in or non-functional.
>
> This adds the necessary code to disable the CSC functionality
> and limit the bus format negotiation to force using the same
> input bus format as the output bus format.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 100 +++++++++++++++-------
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 1 +
> include/drm/bridge/dw_hdmi.h | 1 +
> 3 files changed, 71 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index f50af40e10340..b5a665c5e406e 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -158,6 +158,8 @@ struct dw_hdmi {
> struct hdmi_data_info hdmi_data;
> const struct dw_hdmi_plat_data *plat_data;
>
> + bool csc_available; /* indicates if the CSC engine is usable */
> +
> int vic;
>
> u8 edid[HDMI_EDID_LEN];
> @@ -1009,9 +1011,10 @@ static int is_color_space_interpolation(struct dw_hdmi *hdmi)
>
> static bool is_csc_needed(struct dw_hdmi *hdmi)
> {
> - return is_color_space_conversion(hdmi) ||
> - is_color_space_decimation(hdmi) ||
> - is_color_space_interpolation(hdmi);
> + return hdmi->csc_available &&
> + (is_color_space_conversion(hdmi) ||
> + is_color_space_decimation(hdmi) ||
> + is_color_space_interpolation(hdmi));
> }
>
> static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
> @@ -1064,6 +1067,9 @@ static void hdmi_video_csc(struct dw_hdmi *hdmi)
> int interpolation = HDMI_CSC_CFG_INTMODE_DISABLE;
> int decimation = 0;
>
> + if (!hdmi->csc_available)
> + return;
> +
> /* YCC422 interpolation to 444 mode */
> if (is_color_space_interpolation(hdmi))
> interpolation = HDMI_CSC_CFG_INTMODE_CHROMA_INT_FORMULA1;
> @@ -2665,6 +2671,7 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> u32 output_fmt,
> unsigned int *num_input_fmts)
> {
> + struct dw_hdmi *hdmi = bridge->driver_private;
> u32 *input_fmts;
> unsigned int i = 0;
>
> @@ -2683,62 +2690,81 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> /* 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available) {
> + 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;
> + if (hdmi->csc_available)
> + 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;
> + if (hdmi->csc_available)
> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
> break;
>
> /*YUV 4:2:0 */
> @@ -2767,15 +2793,24 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
> {
> struct dw_hdmi *hdmi = bridge->driver_private;
>
> - hdmi->hdmi_data.enc_out_bus_format =
> - bridge_state->output_bus_cfg.format;
> + if (!hdmi->csc_available &&
> + bridge_state->output_bus_cfg.format != bridge_state->input_bus_cfg.format) {
> + dev_warn(hdmi->dev, "different input format 0x%04x & output format 0x%04x while CSC isn't usable, fallback to safe format\n",
> + bridge_state->input_bus_cfg.format,
> + bridge_state->output_bus_cfg.format);
> + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_FIXED;
> + hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_FIXED;
> + } else {
> + hdmi->hdmi_data.enc_out_bus_format =
> + bridge_state->output_bus_cfg.format;
>
> - hdmi->hdmi_data.enc_in_bus_format =
> - bridge_state->input_bus_cfg.format;
> + hdmi->hdmi_data.enc_in_bus_format =
> + bridge_state->input_bus_cfg.format;
>
> - dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
> - bridge_state->input_bus_cfg.format,
> - bridge_state->output_bus_cfg.format);
> + dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
> + bridge_state->input_bus_cfg.format,
> + bridge_state->output_bus_cfg.format);
> + }
>
> return 0;
> }
> @@ -3481,6 +3516,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
> hdmi->cec = platform_device_register_full(&pdevinfo);
> }
>
> + /* Get CSC useability from config0 register and permit override for platforms */
> + hdmi->csc_available = !plat_data->disable_csc || (config0 & HDMI_CONFIG0_CSC);
> +
> drm_bridge_add(&hdmi->bridge);
>
> return hdmi;
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 1999db05bc3b2..279722e4d1898 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -541,6 +541,7 @@ enum {
>
> /* CONFIG0_ID field values */
> HDMI_CONFIG0_I2S = 0x10,
> + HDMI_CONFIG0_CSC = 0x04,
> HDMI_CONFIG0_CEC = 0x02,
>
> /* CONFIG1_ID field values */
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 2a1f85f9a8a3f..b2f689cbe864c 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -157,6 +157,7 @@ struct dw_hdmi_plat_data {
> unsigned long mpixelclock);
>
> unsigned int disable_cec : 1;
> + unsigned int disable_csc : 1;
> };
>
> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
Is this really still needed now you filter correctly the possible
modes in patch 1 ?
Neil
Hi Neil,
> Am 07.04.2022 um 10:28 schrieb Neil Armstrong <[email protected]>:
>
> Hi,
>
> On 06/04/2022 18:26, H. Nikolaus Schaller wrote:
>> From: Neil Armstrong <[email protected]>
>> The dw-hdmi integrates an optional Color Space Conversion feature used
>> to handle color-space conversions.
>> On some platforms, the CSC isn't built-in or non-functional.
>> This adds the necessary code to disable the CSC functionality
>> and limit the bus format negotiation to force using the same
>> input bus format as the output bus format.
>> Signed-off-by: Neil Armstrong <[email protected]>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 100 +++++++++++++++-------
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 1 +
>> include/drm/bridge/dw_hdmi.h | 1 +
>> 3 files changed, 71 insertions(+), 31 deletions(-)
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index f50af40e10340..b5a665c5e406e 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -158,6 +158,8 @@ struct dw_hdmi {
>> struct hdmi_data_info hdmi_data;
>> const struct dw_hdmi_plat_data *plat_data;
>> + bool csc_available; /* indicates if the CSC engine is usable */
>> +
>> int vic;
>> u8 edid[HDMI_EDID_LEN];
>> @@ -1009,9 +1011,10 @@ static int is_color_space_interpolation(struct dw_hdmi *hdmi)
>> static bool is_csc_needed(struct dw_hdmi *hdmi)
>> {
>> - return is_color_space_conversion(hdmi) ||
>> - is_color_space_decimation(hdmi) ||
>> - is_color_space_interpolation(hdmi);
>> + return hdmi->csc_available &&
>> + (is_color_space_conversion(hdmi) ||
>> + is_color_space_decimation(hdmi) ||
>> + is_color_space_interpolation(hdmi));
>> }
>> static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
>> @@ -1064,6 +1067,9 @@ static void hdmi_video_csc(struct dw_hdmi *hdmi)
>> int interpolation = HDMI_CSC_CFG_INTMODE_DISABLE;
>> int decimation = 0;
>> + if (!hdmi->csc_available)
>> + return;
>> +
>> /* YCC422 interpolation to 444 mode */
>> if (is_color_space_interpolation(hdmi))
>> interpolation = HDMI_CSC_CFG_INTMODE_CHROMA_INT_FORMULA1;
>> @@ -2665,6 +2671,7 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>> u32 output_fmt,
>> unsigned int *num_input_fmts)
>> {
>> + struct dw_hdmi *hdmi = bridge->driver_private;
>> u32 *input_fmts;
>> unsigned int i = 0;
>> @@ -2683,62 +2690,81 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>> /* 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available) {
>> + 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;
>> + if (hdmi->csc_available)
>> + 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;
>> + if (hdmi->csc_available)
>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>> break;
>> /*YUV 4:2:0 */
>> @@ -2767,15 +2793,24 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
>> {
>> struct dw_hdmi *hdmi = bridge->driver_private;
>> - hdmi->hdmi_data.enc_out_bus_format =
>> - bridge_state->output_bus_cfg.format;
>> + if (!hdmi->csc_available &&
>> + bridge_state->output_bus_cfg.format != bridge_state->input_bus_cfg.format) {
>> + dev_warn(hdmi->dev, "different input format 0x%04x & output format 0x%04x while CSC isn't usable, fallback to safe format\n",
>> + bridge_state->input_bus_cfg.format,
>> + bridge_state->output_bus_cfg.format);
>> + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_FIXED;
>> + hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_FIXED;
>> + } else {
>> + hdmi->hdmi_data.enc_out_bus_format =
>> + bridge_state->output_bus_cfg.format;
>> - hdmi->hdmi_data.enc_in_bus_format =
>> - bridge_state->input_bus_cfg.format;
>> + hdmi->hdmi_data.enc_in_bus_format =
>> + bridge_state->input_bus_cfg.format;
>> - dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
>> - bridge_state->input_bus_cfg.format,
>> - bridge_state->output_bus_cfg.format);
>> + dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
>> + bridge_state->input_bus_cfg.format,
>> + bridge_state->output_bus_cfg.format);
>> + }
>> return 0;
>> }
>> @@ -3481,6 +3516,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>> hdmi->cec = platform_device_register_full(&pdevinfo);
>> }
>> + /* Get CSC useability from config0 register and permit override for platforms */
>> + hdmi->csc_available = !plat_data->disable_csc || (config0 & HDMI_CONFIG0_CSC);
>> +
>> drm_bridge_add(&hdmi->bridge);
>> return hdmi;
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> index 1999db05bc3b2..279722e4d1898 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> @@ -541,6 +541,7 @@ enum {
>> /* CONFIG0_ID field values */
>> HDMI_CONFIG0_I2S = 0x10,
>> + HDMI_CONFIG0_CSC = 0x04,
>> HDMI_CONFIG0_CEC = 0x02,
>> /* CONFIG1_ID field values */
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index 2a1f85f9a8a3f..b2f689cbe864c 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -157,6 +157,7 @@ struct dw_hdmi_plat_data {
>> unsigned long mpixelclock);
>> unsigned int disable_cec : 1;
>> + unsigned int disable_csc : 1;
>> };
>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>
> Is this really still needed now you filter correctly the possible
> modes in patch 1 ?
I had not tried to remove them because they were needed in [PATCH v16]
but indeed they are no longer needed. Something (which I personally
don't understand) may have blocked it so far, but it is not worth
further analyses.
So we can shrink the series and no need to touch drm/bridge: dw-hdmi:
any more!
I'll now post a new v18.
BR and thanks for review,
Nikolaus