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