dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks").
After this patch, the screen becomes greenish(may be it is setting it into YUV format??).
By checking the code, previously it used to call get_input_fmt callback and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
Regards, Biju
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks").
After this patch, the screen becomes greenish(may be it is setting it into YUV format??).
By checking the code, previously it used to call get_input_fmt callback and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
I have tested linux-next 20220112 on a imx6q-sabresd board, which shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks").
After this patch, the screen becomes greenish(may be it is setting it into YUV format??).
By checking the code, previously it used to call get_input_fmt callback and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
I have tested linux-next 20220112 on a imx6q-sabresd board, which shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Hi Neil,
+ renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com
wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
callbacks").
After this patch, the screen becomes greenish(may be it is setting it
into YUV format??).
By checking the code, previously it used to call get_input_fmt callback
and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see
the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
I have tested linux-next 20220112 on a imx6q-sabresd board, which shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com
wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
callbacks").
After this patch, the screen becomes greenish(may be it is setting it
into YUV format??).
By checking the code, previously it used to call get_input_fmt callback
and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see
the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
I have tested linux-next 20220112 on a imx6q-sabresd board, which shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, last_bridge);
- if (last_bridge->funcs->atomic_get_output_bus_fmts) { + /* + * Only negociate with real values if both end of the bridge chain + * support negociation callbacks, otherwise you can end in a situation + * where the selected output format doesn't match with the first bridge + * output format. + */ + if (bridge->funcs->atomic_get_input_bus_fmts && + last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs = last_bridge->funcs;
/* @@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
- if (conn->display_info.num_bus_formats && + /* + * If first bridge doesn't support negociation, use MEDIA_BUS_FMT_FIXED + * as a safe value for the whole bridge chain + */ + if (bridge->funcs->atomic_get_input_bus_fmts && + conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn->display_info.bus_formats[0]; else ==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
Neil
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com
wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
callbacks").
After this patch, the screen becomes greenish(may be it is setting it
into YUV format??).
By checking the code, previously it used to call get_input_fmt callback
and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see
the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
I have tested linux-next 20220112 on a imx6q-sabresd board, which
shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state-
state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridge chain* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the firstbridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs = last_bridge->funcs; /*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-display_info.bus_formats[0];
else==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=0######### [ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16######### [ 3.473644] ########hdmi_video_sample MEDIA_BUS_FMT_UYVY8_1X16#########
Regards, Biju
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com
wrote:
Hi All,
RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) till the commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
callbacks").
After this patch, the screen becomes greenish(may be it is setting it
into YUV format??).
By checking the code, previously it used to call get_input_fmt callback
and set colour as RGB24.
After this commit, it calls get_output_fmt_callbck and returns 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, I see
the outputformat as YUV16 instead of RGB24.
Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
I have tested linux-next 20220112 on a imx6q-sabresd board, which
shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state-
state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridge chain* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the firstbridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs = last_bridge->funcs; /*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-display_info.bus_formats[0];
else==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=0######### [ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16######### [ 3.473644] ########hdmi_video_sample MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
==><=============================== diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..68f79094f648 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2589,45 +2589,44 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, }
/* - * Order bus formats from 16bit to 8bit and from YUV422 to RGB + * Order bus formats from 16bit to 8bit and from RGB to YUV422 * if supported. In any case the default RGB888 format is added */
if (max_bpc >= 16 && info->bpc == 16) { + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; + 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; + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
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 (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; }
if (max_bpc >= 10 && info->bpc >= 10) { - if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) - output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
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_UYVY10_1X20; }
- if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) - output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
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; + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
*num_output_fmts = i;
==><===============================
Neil
Regards, Biju
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote:
Hi Biju,
On Thu, Jan 13, 2022 at 2:45 PM Biju Das biju.das.jz@bp.renesas.com
wrote:
> > Hi All, > > RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) > till the commit > 7cd70656d1285b79("drm/bridge: display-connector: implement bus > fmts
callbacks").
> > After this patch, the screen becomes greenish(may be it is > setting it
into YUV format??).
> > By checking the code, previously it used to call get_input_fmt > callback
and set colour as RGB24.
> > After this commit, it calls get_output_fmt_callbck and returns 3 > outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, > I see
the outputformat as YUV16 instead of RGB24.
> > Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
I have tested linux-next 20220112 on a imx6q-sabresd board, which
shows:
dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with HDCP (DWC HDMI 3D TX PHY)
The colors are shown correctly here.
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state-
state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridgechain
* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the- first
bridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs =last_bridge->funcs;
/*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-display_info.bus_formats[0];
else==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0######### [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24######### [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch. at least it is fixing the colour issue??
Regards, Biju
==><=============================== diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..68f79094f648 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2589,45 +2589,44 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, }
/*
* Order bus formats from 16bit to 8bit and from YUV422 to RGB
* Order bus formats from 16bit to 8bit and from RGB to YUV422
- if supported. In any case the default RGB888 format is added
*/
if (max_bpc >= 16 && info->bpc == 16) {
output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;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;
output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;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 (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;}
if (max_bpc >= 10 && info->bpc >= 10) {
if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;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_UYVY10_1X20;
- if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
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;
if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;*num_output_fmts = i;
==><===============================
Neil
Regards, Biju
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 13/01/2022 21:01, Fabio Estevam wrote: > Hi Biju, > > On Thu, Jan 13, 2022 at 2:45 PM Biju Das > biju.das.jz@bp.renesas.com wrote: >> >> Hi All, >> >> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) >> till the commit >> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >> fmts callbacks"). >> >> After this patch, the screen becomes greenish(may be it is >> setting it into YUV format??). >> >> By checking the code, previously it used to call get_input_fmt >> callback and set colour as RGB24. >> >> After this commit, it calls get_output_fmt_callbck and returns 3 >> outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, >> I see the outputformat as YUV16 instead of RGB24. >> >> Not sure, I am the only one seeing this issue with dw_HDMI driver.
This patch was introduced to maintain the bridge color format negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it seems it behaves incorrectly if the first bridge doesn't implement the negotiation callbacks.
Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
> > I have tested linux-next 20220112 on a imx6q-sabresd board, which
shows:
> > dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with > HDCP (DWC HDMI 3D TX PHY) > > The colors are shown correctly here. >
The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the negotiation fails and use the RGB fallback input & output format.
Anyway thanks for testing
Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state-
state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridgechain
* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the- first
bridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs =last_bridge->funcs;
/*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-display_info.bus_formats[0];
else==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0######### [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24######### [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch. at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in linus master.
Neil
Regards, Biju
==><=============================== diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..68f79094f648 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2589,45 +2589,44 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, }
/*
* Order bus formats from 16bit to 8bit and from YUV422 to RGB
* Order bus formats from 16bit to 8bit and from RGB to YUV422
- if supported. In any case the default RGB888 format is added
*/
if (max_bpc >= 16 && info->bpc == 16) {
output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;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;
output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;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 (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;}
if (max_bpc >= 10 && info->bpc >= 10) {
if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;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_UYVY10_1X20;
- if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
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;
if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;*num_output_fmts = i;
==><===============================
Neil
Regards, Biju
Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote:
Hi Neil,
- renesas-soc
> Subject: Re: dw_hdmi is showing wrong colour after commit > 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts > callbacks") > > Hi, > > On 13/01/2022 21:01, Fabio Estevam wrote: >> Hi Biju, >> >> On Thu, Jan 13, 2022 at 2:45 PM Biju Das >> biju.das.jz@bp.renesas.com > wrote: >>> >>> Hi All, >>> >>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) >>> till the commit >>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>> fmts > callbacks"). >>> >>> After this patch, the screen becomes greenish(may be it is >>> setting it > into YUV format??). >>> >>> By checking the code, previously it used to call get_input_fmt >>> callback > and set colour as RGB24. >>> >>> After this commit, it calls get_output_fmt_callbck and returns 3 >>> outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, >>> I see > the outputformat as YUV16 instead of RGB24. >>> >>> Not sure, I am the only one seeing this issue with dw_HDMI driver. > > This patch was introduced to maintain the bridge color format > negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it > seems it behaves incorrectly if the first bridge doesn't implement > the negotiation callbacks. > > Let me check the code to see how to fix that.
Thanks for the information, I am happy to test the patch/fix.
Cheers, Biju
> >> >> I have tested linux-next 20220112 on a imx6q-sabresd board, which
shows:
>> >> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with >> HDCP (DWC HDMI 3D TX PHY) >> >> The colors are shown correctly here. >> > > The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the > negotiation fails and use the RGB fallback input & output format. > > Anyway thanks for testing > > Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state-
state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridgechain
* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the- first
bridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs =last_bridge->funcs;
/*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-display_info.bus_formats[0];
else==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0######### [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24######### [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch. at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in linus master.
Neil
Regards, Biju
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
- /* If dw-hdmi is the only bridge, avoid negociating with ourselves */ - if (list_is_singular(&bridge->encoder->bridge_chain)) { + /* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */ + if (list_is_singular(&bridge->encoder->bridge_chain) || + list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;
@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
+ /* If dw-hdmi is the first bridge fall-back to safe output format */ + if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) + output_fmt = MEDIA_BUS_FMT_FIXED; + switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED: ========================><=============================
Thanks, Neil
Neil
Regards, Biju
Hi Neil,
Quoting Neil Armstrong (2022-01-17 10:08:38)
Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote: > Hi Neil, > > + renesas-soc > >> Subject: Re: dw_hdmi is showing wrong colour after commit >> 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts >> callbacks") >> >> Hi, >> >> On 13/01/2022 21:01, Fabio Estevam wrote: >>> Hi Biju, >>> >>> On Thu, Jan 13, 2022 at 2:45 PM Biju Das >>> biju.das.jz@bp.renesas.com >> wrote: >>>> >>>> Hi All, >>>> >>>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) >>>> till the commit >>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>>> fmts >> callbacks"). >>>> >>>> After this patch, the screen becomes greenish(may be it is >>>> setting it >> into YUV format??). >>>> >>>> By checking the code, previously it used to call get_input_fmt >>>> callback >> and set colour as RGB24. >>>> >>>> After this commit, it calls get_output_fmt_callbck and returns 3 >>>> outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, >>>> I see >> the outputformat as YUV16 instead of RGB24. >>>> >>>> Not sure, I am the only one seeing this issue with dw_HDMI driver. >> >> This patch was introduced to maintain the bridge color format >> negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it >> seems it behaves incorrectly if the first bridge doesn't implement >> the negotiation callbacks. >> >> Let me check the code to see how to fix that. > > Thanks for the information, I am happy to test the patch/fix. > > Cheers, > Biju > >> >>> >>> I have tested linux-next 20220112 on a imx6q-sabresd board, which shows: >>> >>> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with >>> HDCP (DWC HDMI 3D TX PHY) >>> >>> The colors are shown correctly here. >>> >> >> The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the >> negotiation fails and use the RGB fallback input & output format. >> >> Anyway thanks for testing >> >> Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state- > state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the bridgechain
* support negociation callbacks, otherwise you can end in asituation
* where the selected output format doesn't match with the- first
bridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs =last_bridge->funcs;
/*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation, useMEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-> display_info.bus_formats[0]; else ==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
This fixes the issue for me here on an H3 Salvator-XS.
Could you add...
Bisected-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Tested-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
alongside Biju's Reported-by: tag when posting as a fix please?
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0######### [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24######### [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch. at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in linus master.
Neil
Regards, Biju
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format */if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
Thanks, Neil
Neil
Regards, Biju
On 17/01/2022 11:58, Kieran Bingham wrote:
Hi Neil,
Quoting Neil Armstrong (2022-01-17 10:08:38)
Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
> Subject: Re: dw_hdmi is showing wrong colour after commit > 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts > callbacks") > > On 14/01/2022 09:29, Biju Das wrote: >> Hi Neil, >> >> + renesas-soc >> >>> Subject: Re: dw_hdmi is showing wrong colour after commit >>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts >>> callbacks") >>> >>> Hi, >>> >>> On 13/01/2022 21:01, Fabio Estevam wrote: >>>> Hi Biju, >>>> >>>> On Thu, Jan 13, 2022 at 2:45 PM Biju Das >>>> biju.das.jz@bp.renesas.com >>> wrote: >>>>> >>>>> Hi All, >>>>> >>>>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour) >>>>> till the commit >>>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>>>> fmts >>> callbacks"). >>>>> >>>>> After this patch, the screen becomes greenish(may be it is >>>>> setting it >>> into YUV format??). >>>>> >>>>> By checking the code, previously it used to call get_input_fmt >>>>> callback >>> and set colour as RGB24. >>>>> >>>>> After this commit, it calls get_output_fmt_callbck and returns 3 >>>>> outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback, >>>>> I see >>> the outputformat as YUV16 instead of RGB24. >>>>> >>>>> Not sure, I am the only one seeing this issue with dw_HDMI driver. >>> >>> This patch was introduced to maintain the bridge color format >>> negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it >>> seems it behaves incorrectly if the first bridge doesn't implement >>> the negotiation callbacks. >>> >>> Let me check the code to see how to fix that. >> >> Thanks for the information, I am happy to test the patch/fix. >> >> Cheers, >> Biju >> >>> >>>> >>>> I have tested linux-next 20220112 on a imx6q-sabresd board, which > shows: >>>> >>>> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with >>>> HDCP (DWC HDMI 3D TX PHY) >>>> >>>> The colors are shown correctly here. >>>> >>> >>> The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the >>> negotiation fails and use the RGB fallback input & output format. >>> >>> Anyway thanks for testing >>> >>> Neil > > Can you test : > > ==><=============================== > diff --git a/drivers/gpu/drm/drm_bridge.c > b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 > 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct > drm_bridge *bridge, > last_bridge_state = > drm_atomic_get_new_bridge_state(crtc_state- >> state, > > last_bridge); > > - if (last_bridge->funcs->atomic_get_output_bus_fmts) { > + /* > + * Only negociate with real values if both end of the bridge
chain
> + * support negociation callbacks, otherwise you can end in a > situation > + * where the selected output format doesn't match with the > + first > bridge > + * output format. > + */ > + if (bridge->funcs->atomic_get_input_bus_fmts && > + last_bridge->funcs->atomic_get_output_bus_fmts) { > const struct drm_bridge_funcs *funcs = > last_bridge->funcs; > > /* > @@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct > drm_bridge *bridge, > if (!out_bus_fmts) > return -ENOMEM; > > - if (conn->display_info.num_bus_formats && > + /* > + * If first bridge doesn't support negociation, use > MEDIA_BUS_FMT_FIXED > + * as a safe value for the whole bridge chain > + */ > + if (bridge->funcs->atomic_get_input_bus_fmts && > + conn->display_info.num_bus_formats && > conn->display_info.bus_formats) > out_bus_fmts[0] = conn- >> display_info.bus_formats[0]; > else > ==><=============================== > > This should exclude your situation where the first bridge doesn't > support negociation.
This fixes the issue for me here on an H3 Salvator-XS.
Could you add...
Bisected-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Tested-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
alongside Biju's Reported-by: tag when posting as a fix please?
Which patch did you test ?
Neil
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0######### [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1######### [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24######### [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch. at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in linus master.
Neil
Regards, Biju
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format */if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
Thanks, Neil
Neil
Regards, Biju
Quoting Neil Armstrong (2022-01-17 13:53:47)
On 17/01/2022 11:58, Kieran Bingham wrote:
Hi Neil,
<big snips to clear up contexts>
This fixes the issue for me here on an H3 Salvator-XS.
Could you add...
Bisected-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Tested-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
alongside Biju's Reported-by: tag when posting as a fix please?
Which patch did you test ?
Ah, yes that's not clear is it - sorry! I replied in the wrong place on the thread when I went back to the mail ... see below...
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format */if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
Sorry, I replied in the wrong part of the thread.
Here's the direct diff on my local tree:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..cac9a87949f1 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,10 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
- /* If dw-hdmi is the only bridge, avoid negociating with ourselves */ - if (list_is_singular(&bridge->encoder->bridge_chain)) { + /* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */ + if (list_is_singular(&bridge->encoder->bridge_chain) || + list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { + *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;
@@ -2673,6 +2675,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
+ /* If dw-hdmi is the first bridge fall-back to safe output format */ + if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) + output_fmt = MEDIA_BUS_FMT_FIXED; + switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:
Which I believe matches the above.
-- Kieran
On 17/01/2022 15:05, Kieran Bingham wrote:
Quoting Neil Armstrong (2022-01-17 13:53:47)
On 17/01/2022 11:58, Kieran Bingham wrote:
Hi Neil,
<big snips to clear up contexts>
This fixes the issue for me here on an H3 Salvator-XS.
Could you add...
Bisected-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Tested-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
alongside Biju's Reported-by: tag when posting as a fix please?
Which patch did you test ?
Ah, yes that's not clear is it - sorry! I replied in the wrong place on the thread when I went back to the mail ... see below...
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format */if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
Sorry, I replied in the wrong part of the thread.
Here's the direct diff on my local tree:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..cac9a87949f1 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,10 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) {*num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;@@ -2673,6 +2675,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format */if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:Which I believe matches the above.
Ok thanks of clarifying !
let me post it asap.
Neil
-- Kieran
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
On 14/01/2022 09:29, Biju Das wrote: > Hi Neil, > > + renesas-soc > >> Subject: Re: dw_hdmi is showing wrong colour after commit >> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >> fmts >> callbacks") >> >> Hi, >> >> On 13/01/2022 21:01, Fabio Estevam wrote: >>> Hi Biju, >>> >>> On Thu, Jan 13, 2022 at 2:45 PM Biju Das >>> biju.das.jz@bp.renesas.com >> wrote: >>>> >>>> Hi All, >>>> >>>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working >>>> ok(colour) till the commit >>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>>> fmts >> callbacks"). >>>> >>>> After this patch, the screen becomes greenish(may be it is >>>> setting it >> into YUV format??). >>>> >>>> By checking the code, previously it used to call get_input_fmt >>>> callback >> and set colour as RGB24. >>>> >>>> After this commit, it calls get_output_fmt_callbck and returns >>>> 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt >>>> callback, I see >> the outputformat as YUV16 instead of RGB24. >>>> >>>> Not sure, I am the only one seeing this issue with dw_HDMI
driver.
>> >> This patch was introduced to maintain the bridge color format >> negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it >> seems it behaves incorrectly if the first bridge doesn't >> implement the negotiation callbacks. >> >> Let me check the code to see how to fix that. > > Thanks for the information, I am happy to test the patch/fix. > > Cheers, > Biju > >> >>> >>> I have tested linux-next 20220112 on a imx6q-sabresd board, >>> which shows: >>> >>> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with >>> HDCP (DWC HDMI 3D TX PHY) >>> >>> The colors are shown correctly here. >>> >> >> The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the >> negotiation fails and use the RGB fallback input & output format. >> >> Anyway thanks for testing >> >> Neil
Can you test :
==><=============================== diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state- > state,
last_bridge);
if (last_bridge->funcs->atomic_get_output_bus_fmts) {
/** Only negociate with real values if both end of the- bridge
chain
* support negociation callbacks, otherwise you can end in- a
situation
* where the selected output format doesn't match with the- first
bridge
* output format.*/if (bridge->funcs->atomic_get_input_bus_fmts &&last_bridge->funcs->atomic_get_output_bus_fmts) { const struct drm_bridge_funcs *funcs =last_bridge->funcs;
/*@@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, if (!out_bus_fmts) return -ENOMEM;
if (conn->display_info.num_bus_formats &&
/** If first bridge doesn't support negociation,- use
MEDIA_BUS_FMT_FIXED
* as a safe value for the whole bridge chain*/if (bridge->funcs->atomic_get_input_bus_fmts &&conn->display_info.num_bus_formats && conn->display_info.bus_formats) out_bus_fmts[0] = conn-> display_info.bus_formats[0]; else ==><===============================
This should exclude your situation where the first bridge doesn't support negociation.
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=0#########
[ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24#########
[ 3.506797] ########hdmi_video_sample
MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is
worth to post a patch.
at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in
linus master.
Neil
Regards, Biju
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves*/
if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating withourselves */
if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node,- &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;
@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format*/
if (list_is_first(&bridge->chain_node, &bridge->encoder-bridge_chain))
output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
This patch alone fixes the issue. I have tested with Linux-next. Do we need below code, as it is already taken care in output_bus_fmt callback.
if (list_is_first(&bridge->chain_node, &bridge->encoder-bridge_chain))
output_fmt = MEDIA_BUS_FMT_FIXED;
Cheers, Biju
On 17/01/2022 13:13, Biju Das wrote:
Hi Neil,
Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
Hi,
On 14/01/2022 15:23, Biju Das wrote:
-----Original Message----- From: Neil Armstrong narmstrong@baylibre.com Sent: 14 January 2022 13:56 To: Biju Das biju.das.jz@bp.renesas.com; Fabio Estevam festevam@gmail.com Cc: daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robert.foss@linaro.org; jonas@kwiboo.se; jernej.skrabec@gmail.com; martin.blumenstingl@googlemail.com; linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")
Hi,
On 14/01/2022 12:08, Biju Das wrote:
Hi Neil,
> Subject: Re: dw_hdmi is showing wrong colour after commit > 7cd70656d1285b79("drm/bridge: display-connector: implement bus > fmts > callbacks") > > On 14/01/2022 09:29, Biju Das wrote: >> Hi Neil, >> >> + renesas-soc >> >>> Subject: Re: dw_hdmi is showing wrong colour after commit >>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>> fmts >>> callbacks") >>> >>> Hi, >>> >>> On 13/01/2022 21:01, Fabio Estevam wrote: >>>> Hi Biju, >>>> >>>> On Thu, Jan 13, 2022 at 2:45 PM Biju Das >>>> biju.das.jz@bp.renesas.com >>> wrote: >>>>> >>>>> Hi All, >>>>> >>>>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working >>>>> ok(colour) till the commit >>>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus >>>>> fmts >>> callbacks"). >>>>> >>>>> After this patch, the screen becomes greenish(may be it is >>>>> setting it >>> into YUV format??). >>>>> >>>>> By checking the code, previously it used to call get_input_fmt >>>>> callback >>> and set colour as RGB24. >>>>> >>>>> After this commit, it calls get_output_fmt_callbck and returns >>>>> 3 outputformats(YUV16, YUV24 and RGB24) And get_input_fmt >>>>> callback, I see >>> the outputformat as YUV16 instead of RGB24. >>>>> >>>>> Not sure, I am the only one seeing this issue with dw_HDMI
driver.
>>> >>> This patch was introduced to maintain the bridge color format >>> negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it >>> seems it behaves incorrectly if the first bridge doesn't >>> implement the negotiation callbacks. >>> >>> Let me check the code to see how to fix that. >> >> Thanks for the information, I am happy to test the patch/fix. >> >> Cheers, >> Biju >> >>> >>>> >>>> I have tested linux-next 20220112 on a imx6q-sabresd board, >>>> which > shows: >>>> >>>> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with >>>> HDCP (DWC HDMI 3D TX PHY) >>>> >>>> The colors are shown correctly here. >>>> >>> >>> The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the >>> negotiation fails and use the RGB fallback input & output format. >>> >>> Anyway thanks for testing >>> >>> Neil > > Can you test : > > ==><=============================== > diff --git a/drivers/gpu/drm/drm_bridge.c > b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716 > 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -955,7 +955,14 @@ > drm_atomic_bridge_chain_select_bus_fmts(struct > drm_bridge *bridge, > last_bridge_state = > drm_atomic_get_new_bridge_state(crtc_state- >> state, > > last_bridge); > > - if (last_bridge->funcs->atomic_get_output_bus_fmts) { > + /* > + * Only negociate with real values if both end of the > + bridge
chain
> + * support negociation callbacks, otherwise you can end in > + a > situation > + * where the selected output format doesn't match with the > + first > bridge > + * output format. > + */ > + if (bridge->funcs->atomic_get_input_bus_fmts && > + last_bridge->funcs->atomic_get_output_bus_fmts) { > const struct drm_bridge_funcs *funcs = > last_bridge->funcs; > > /* > @@ -980,7 +987,12 @@ > drm_atomic_bridge_chain_select_bus_fmts(struct > drm_bridge *bridge, > if (!out_bus_fmts) > return -ENOMEM; > > - if (conn->display_info.num_bus_formats && > + /* > + * If first bridge doesn't support negociation, > + use > MEDIA_BUS_FMT_FIXED > + * as a safe value for the whole bridge chain > + */ > + if (bridge->funcs->atomic_get_input_bus_fmts && > + conn->display_info.num_bus_formats && > conn->display_info.bus_formats) > out_bus_fmts[0] = conn- >> display_info.bus_formats[0]; > else > ==><=============================== > > This should exclude your situation where the first bridge doesn't > support negociation.
I have tested this fix with Linux next-20220114. Still I see colour
issue.
It is still negotiating and it is calling get_output_fmt_callbck
[ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=0#########
[ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=2#########
And In get_input_fmt callback, I See the outputformat as YUV16 instead
of RGB24.
[ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16#########
[ 3.473644] ########hdmi_video_sample
MEDIA_BUS_FMT_UYVY8_1X16#########
OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the encoder.
Yep.
Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV.
Anyway please test that:
It works now after reordering.
[ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24=0#########
[ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_YUV8_1X24=1#########
[ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
MEDIA_BUS_FMT_UYVY8_1X16=2#########
[ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
MEDIA_BUS_FMT_RGB888_1X24#########
[ 3.506797] ########hdmi_video_sample
MEDIA_BUS_FMT_RGB888_1X24#########
Is it acceptable solution to the users of dw_hdmi driver? May be it is
worth to post a patch.
at least it is fixing the colour issue??
Yes, it gets back to default behavior before negociation, nevertheless we need to think how to handle your use-case correctly at some point.
I'll post this as a patch ASAP so it gets applied before landing in
linus master.
Neil
Regards, Biju
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><============================= diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 54d8fdad395f..9f2e1cac0ae2 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, if (!output_fmts) return NULL;
/* If dw-hdmi is the only bridge, avoid negociating with ourselves*/
if (list_is_singular(&bridge->encoder->bridge_chain)) {
/* If dw-hdmi is the first or only bridge, avoid negociating withourselves */
if (list_is_singular(&bridge->encoder->bridge_chain) ||list_is_first(&bridge->chain_node,- &bridge->encoder->bridge_chain)) { *num_output_fmts = 1; output_fmts[0] = MEDIA_BUS_FMT_FIXED;
@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, if (!input_fmts) return NULL;
/* If dw-hdmi is the first bridge fall-back to safe output format*/
if (list_is_first(&bridge->chain_node, &bridge->encoder-bridge_chain))
output_fmt = MEDIA_BUS_FMT_FIXED;switch (output_fmt) { /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ case MEDIA_BUS_FMT_FIXED:========================><=============================
This patch alone fixes the issue. I have tested with Linux-next. Do we need below code, as it is already taken care in output_bus_fmt callback.
You're right in your case the first part is enough.
if (list_is_first(&bridge->chain_node, &bridge->encoder-bridge_chain))
output_fmt = MEDIA_BUS_FMT_FIXED;Cheers, Biju
Thanks for testing, Neil
dri-devel@lists.freedesktop.org
-
Biju Das -
Fabio Estevam -
Kieran Bingham -
Neil Armstrong