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