* dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") @ 2022-01-13 17:45 Biju Das 2022-01-13 20:01 ` Fabio Estevam 0 siblings, 1 reply; 14+ messages in thread From: Biju Das @ 2022-01-13 17:45 UTC (permalink / raw) To: Neil Armstrong, daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec Cc: martin.blumenstingl, dri-devel, linux-amlogic, linux-arm-kernel, linux-kernel 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. Regards, Biju ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-13 17:45 dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") Biju Das @ 2022-01-13 20:01 ` Fabio Estevam 2022-01-14 8:23 ` Neil Armstrong 0 siblings, 1 reply; 14+ messages in thread From: Fabio Estevam @ 2022-01-13 20:01 UTC (permalink / raw) To: Biju Das Cc: Neil Armstrong, daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel 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. 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-13 20:01 ` Fabio Estevam @ 2022-01-14 8:23 ` Neil Armstrong 2022-01-14 8:29 ` Biju Das 0 siblings, 1 reply; 14+ messages in thread From: Neil Armstrong @ 2022-01-14 8:23 UTC (permalink / raw) To: Fabio Estevam, Biju Das Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel 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. > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 8:23 ` Neil Armstrong @ 2022-01-14 8:29 ` Biju Das 2022-01-14 10:42 ` Neil Armstrong 0 siblings, 1 reply; 14+ messages in thread From: Biju Das @ 2022-01-14 8:29 UTC (permalink / raw) To: Neil Armstrong, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 8:29 ` Biju Das @ 2022-01-14 10:42 ` Neil Armstrong 2022-01-14 11:08 ` Biju Das 0 siblings, 1 reply; 14+ messages in thread From: Neil Armstrong @ 2022-01-14 10:42 UTC (permalink / raw) To: Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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. Neil ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 10:42 ` Neil Armstrong @ 2022-01-14 11:08 ` Biju Das 2022-01-14 13:56 ` Neil Armstrong 0 siblings, 1 reply; 14+ messages in thread From: Biju Das @ 2022-01-14 11:08 UTC (permalink / raw) To: Neil Armstrong, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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######### Regards, Biju ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 11:08 ` Biju Das @ 2022-01-14 13:56 ` Neil Armstrong 2022-01-14 14:23 ` Biju Das 0 siblings, 1 reply; 14+ messages in thread From: Neil Armstrong @ 2022-01-14 13:56 UTC (permalink / raw) To: Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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. Let me figure that out, no sure I can find a clean solution except putting back RGB24 before YUV. Anyway please test that: ==><=============================== 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 > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 13:56 ` Neil Armstrong @ 2022-01-14 14:23 ` Biju Das 2022-01-14 14:40 ` Neil Armstrong 0 siblings, 1 reply; 14+ messages in thread From: Biju Das @ 2022-01-14 14:23 UTC (permalink / raw) To: Neil Armstrong, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc > -----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?? 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 14:23 ` Biju Das @ 2022-01-14 14:40 ` Neil Armstrong 2022-01-17 10:08 ` Neil Armstrong 0 siblings, 1 reply; 14+ messages in thread From: Neil Armstrong @ 2022-01-14 14:40 UTC (permalink / raw) To: Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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 > >> >> ==><=============================== >> 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 >>> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-14 14:40 ` Neil Armstrong @ 2022-01-17 10:08 ` Neil Armstrong 2022-01-17 12:13 ` Biju Das [not found] ` <164241711700.10801.9011781958267060147@Monstersaurus> 0 siblings, 2 replies; 14+ messages in thread From: Neil Armstrong @ 2022-01-17 10:08 UTC (permalink / raw) To: Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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 >>>> >> > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-17 10:08 ` Neil Armstrong @ 2022-01-17 12:13 ` Biju Das 2022-01-17 13:52 ` Neil Armstrong [not found] ` <164241711700.10801.9011781958267060147@Monstersaurus> 1 sibling, 1 reply; 14+ messages in thread From: Biju Das @ 2022-01-17 12:13 UTC (permalink / raw) To: Neil Armstrong, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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@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: > ========================><============================= 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") 2022-01-17 12:13 ` Biju Das @ 2022-01-17 13:52 ` Neil Armstrong 0 siblings, 0 replies; 14+ messages in thread From: Neil Armstrong @ 2022-01-17 13:52 UTC (permalink / raw) To: Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc On 17/01/2022 13:13, Biju Das wrote: > 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@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: >> ========================><============================= > > 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. You're right in your case the first part is enough. > >> + if (list_is_first(&bridge->chain_node, &bridge->encoder- >>> bridge_chain)) >> + output_fmt = MEDIA_BUS_FMT_FIXED; > > Cheers, > Biju > Thanks for testing, Neil ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <164241711700.10801.9011781958267060147@Monstersaurus>]
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") [not found] ` <164241711700.10801.9011781958267060147@Monstersaurus> @ 2022-01-17 13:53 ` Neil Armstrong [not found] ` <164242831905.10801.10615379536917395435@Monstersaurus> 0 siblings, 1 reply; 14+ messages in thread From: Neil Armstrong @ 2022-01-17 13:53 UTC (permalink / raw) To: Kieran Bingham, Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc 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@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. > > This fixes the issue for me here on an H3 Salvator-XS. > > Could you add... > > Bisected-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > 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 >>>>>> >>>> >>> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <164242831905.10801.10615379536917395435@Monstersaurus>]
* Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") [not found] ` <164242831905.10801.10615379536917395435@Monstersaurus> @ 2022-01-17 14:07 ` Neil Armstrong 0 siblings, 0 replies; 14+ messages in thread From: Neil Armstrong @ 2022-01-17 14:07 UTC (permalink / raw) To: Kieran Bingham, Biju Das, Fabio Estevam Cc: daniel, Laurent.pinchart, robert.foss, jonas, jernej.skrabec, martin.blumenstingl, linux-amlogic, linux-arm-kernel, dri-devel, linux-kernel, linux-renesas-soc On 17/01/2022 15:05, Kieran Bingham wrote: > Quoting Neil Armstrong (2022-01-17 13:53:47) >> On 17/01/2022 11:58, Kieran Bingham wrote: >>> Hi Neil, > > <big snips to clear up contexts> > >>> This fixes the issue for me here on an H3 Salvator-XS. >>> >>> Could you add... >>> >>> Bisected-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> >>> alongside Biju's Reported-by: tag when posting as a fix please? >> >> >> Which patch did you test ? > > Ah, yes that's not clear is it - sorry! I replied in the wrong place on > the thread when I went back to the mail ... see below... > >>>> 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: >>>> ========================><============================= > > Sorry, I replied in the wrong part of the thread. > > Here's the direct diff on my local tree: > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 54d8fdad395f..cac9a87949f1 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2551,8 +2551,10 @@ 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 +2675,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: > > Which I believe matches the above. Ok thanks of clarifying ! let me post it asap. Neil > > -- > Kieran > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-01-17 14:07 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-13 17:45 dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks") Biju Das 2022-01-13 20:01 ` Fabio Estevam 2022-01-14 8:23 ` Neil Armstrong 2022-01-14 8:29 ` Biju Das 2022-01-14 10:42 ` Neil Armstrong 2022-01-14 11:08 ` Biju Das 2022-01-14 13:56 ` Neil Armstrong 2022-01-14 14:23 ` Biju Das 2022-01-14 14:40 ` Neil Armstrong 2022-01-17 10:08 ` Neil Armstrong 2022-01-17 12:13 ` Biju Das 2022-01-17 13:52 ` Neil Armstrong [not found] ` <164241711700.10801.9011781958267060147@Monstersaurus> 2022-01-17 13:53 ` Neil Armstrong [not found] ` <164242831905.10801.10615379536917395435@Monstersaurus> 2022-01-17 14:07 ` Neil Armstrong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).