dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Biju Das]

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Fabio Estevam]

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.

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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

RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Biju Das]

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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

RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Biju Das]

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

Regards,
Biju

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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.

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
> 

RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Biju Das]

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

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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

RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Biju Das]

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 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:
> ========================><=============================

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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 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:
>> ========================><=============================
> 
> 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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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

Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

[Neil Armstrong]

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
>