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

[Neil Armstrong <narmstrong@baylibre.com>]

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic