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

[Neil Armstrong <narmstrong@baylibre.com>]

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