Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes
From: Neil Armstrong
Date: Thu Mar 03 2022 - 03:35:32 EST
Hi,
On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
Hi Neil,
Am 02.03.2022 um 15:34 schrieb Neil Armstrong <narmstrong@xxxxxxxxxxxx>:
Hi,
(cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)
I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?
I have forced hdmi->sink_is_hdmi = false and replaced
/* Default 8bit RGB fallback */
- output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+ output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
(MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
So this indicates that YUV conversion is not working properly. Maybe missing some special
setup.
What I have to test if it works on a different monitor. Not that this specific panel
(a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV capabilities
but does not handle them...
On the other hand this panel works on RasPi and OMAP5 (where I admit I do not know in
which mode).
Pretty sure they don't support YUV HDMI output.
If you can try on a certified HDMI devices like a TV, it would here figuring out where comes the issue.
If your CSC is broken, we'll need to disable it on your platform.
Indeed.
So it seems as if we need a mechanism to overwrite dw_hdmi_bridge_atomic_get_output_bus_fmts()
in our ingenic-dw-hdmi platform specialization [1] to always return MEDIA_BUS_FMT_RGB888_1X24.
Or alternatively set sink_is_hdmi = false there (unfortunately there is no direct access to
struct dw_hdmi in a specialization drivers).
Is this already possible or how can it be done?
It's not handled yet, but we may add the logic to handle the lack of CSC config bit and
add a glue config bit to override this like we already did for CEC.
I wrote an initial support to disable CSC (only compile-tested), could you try on your platform with setting disable_csc = 1 in your dw-hdmi glue code ?
================><=======================================
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 54d8fdad395f..d345166a69aa 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;
@@ -2663,6 +2669,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;
@@ -2681,62 +2688,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 */
@@ -2765,15 +2791,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;
}
@@ -3479,6 +3514,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 1999db05bc3b..279722e4d189 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 2a1f85f9a8a3..b2f689cbe864 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,
=================><==============================================================
Neil
BR and thanks,
Nikolaus
[1]: https://lore.kernel.org/all/24a27226a22adf5f5573f013e5d7d89b0ec73664.1645895582.git.hns@xxxxxxxxxxxxx/