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@xxxxxxxxxxxx>
>>>>> Sent: 14 January 2022 13:56
>>>>> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Fabio Estevam
>>>>> <festevam@xxxxxxxxx>
>>>>> Cc: daniel@xxxxxxxx; Laurent.pinchart@xxxxxxxxxxxxxxxx;
>>>>> robert.foss@xxxxxxxxxx; jonas@xxxxxxxxx; jernej.skrabec@xxxxxxxxx;
>>>>> martin.blumenstingl@xxxxxxxxxxxxxx; linux-amlogic@xxxxxxxxxxxxxxxxxxx;
>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>>>>> linux-kernel@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx
>>>>> 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@xxxxxxxxxxxxxx>
>>>>>>>>> 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@xxxxxxxxxxxxxxxx>
> Tested-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> 
> 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
>>>>>>
>>>>
>>>
>>