| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <OS0PR01MB5922BC31FBCF85F99F17737B86549@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date: Fri, 14 Jan 2022 14:23:52 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Neil Armstrong <narmstrong@...libre.com>,
Fabio Estevam <festevam@...il.com>
CC: "daniel@...ll.ch" <daniel@...ll.ch>,
"Laurent.pinchart@...asonboard.com"
<Laurent.pinchart@...asonboard.com>,
"robert.foss@...aro.org" <robert.foss@...aro.org>,
"jonas@...boo.se" <jonas@...boo.se>,
"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>,
"martin.blumenstingl@...glemail.com"
<martin.blumenstingl@...glemail.com>,
"linux-amlogic@...ts.infradead.org"
<linux-amlogic@...ts.infradead.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-renesas-soc@...r.kernel.org"
<linux-renesas-soc@...r.kernel.org>
Subject: RE: dw_hdmi is showing wrong colour after commit
7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
callbacks")
> -----Original Message-----
> From: Neil Armstrong <narmstrong@...libre.com>
> Sent: 14 January 2022 13:56
> To: Biju Das <biju.das.jz@...renesas.com>; Fabio Estevam
> <festevam@...il.com>
> Cc: daniel@...ll.ch; Laurent.pinchart@...asonboard.com;
> robert.foss@...aro.org; jonas@...boo.se; jernej.skrabec@...il.com;
> martin.blumenstingl@...glemail.com; linux-amlogic@...ts.infradead.org;
> linux-arm-kernel@...ts.infradead.org; dri-devel@...ts.freedesktop.org;
> linux-kernel@...r.kernel.org; linux-renesas-soc@...r.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@...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??
Regards,
Biju
>
> ==><===============================
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 54d8fdad395f..68f79094f648 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2589,45 +2589,44 @@ static u32
> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> }
>
> /*
> - * Order bus formats from 16bit to 8bit and from YUV422 to RGB
> + * Order bus formats from 16bit to 8bit and from RGB to YUV422
> * if supported. In any case the default RGB888 format is added
> */
>
> if (max_bpc >= 16 && info->bpc == 16) {
> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
> +
> if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
> output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
> -
> - output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
> }
>
> if (max_bpc >= 12 && info->bpc >= 12) {
> - if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> - output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>
> if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
> output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>
> - output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
> }
>
> if (max_bpc >= 10 && info->bpc >= 10) {
> - if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> - output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>
> if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
> output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>
> - output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
> }
>
> - if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> - output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>
> if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444)
> output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>
> - /* Default 8bit RGB fallback */
> - output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422)
> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>
> *num_output_fmts = i;
>
> ==><===============================
>
> Neil
>
> >
> > Regards,
> > Biju
> >
Powered by blists - more mailing lists