Hi again,
On 14/01/2022 15:40, Neil Armstrong wrote:
> Hi,
>
> On 14/01/2022 15:23, Biju Das wrote:
>>
>>
>>> -----Original Message-----
>>> From: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>> Sent: 14 January 2022 13:56
>>> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Fabio Estevam
>>> <festevam@xxxxxxxxx>
>>> Cc: daniel@xxxxxxxx; Laurent.pinchart@xxxxxxxxxxxxxxxx;
>>> robert.foss@xxxxxxxxxx; jonas@xxxxxxxxx; jernej.skrabec@xxxxxxxxx;
>>> martin.blumenstingl@xxxxxxxxxxxxxx; linux-amlogic@xxxxxxxxxxxxxxxxxxx;
>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>>> linux-kernel@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx
>>> Subject: Re: dw_hdmi is showing wrong colour after commit
>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
>>> callbacks")
>>>
>>> Hi,
>>>
>>> On 14/01/2022 12:08, Biju Das wrote:
>>>> Hi Neil,
>>>>
>>>>> Subject: Re: dw_hdmi is showing wrong colour after commit
>>>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
>>>>> callbacks")
>>>>>
>>>>> On 14/01/2022 09:29, Biju Das wrote:
>>>>>> Hi Neil,
>>>>>>
>>>>>> + renesas-soc
>>>>>>
>>>>>>> Subject: Re: dw_hdmi is showing wrong colour after commit
>>>>>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts
>>>>>>> callbacks")
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 13/01/2022 21:01, Fabio Estevam wrote:
>>>>>>>> Hi Biju,
>>>>>>>>
>>>>>>>> On Thu, Jan 13, 2022 at 2:45 PM Biju Das
>>>>>>>> <biju.das.jz@xxxxxxxxxxxxxx>
>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> RZ/G2{H, M, N} SoC has dw_hdmi IP and it was working ok(colour)
>>>>>>>>> till the commit
>>>>>>>>> 7cd70656d1285b79("drm/bridge: display-connector: implement bus
>>>>>>>>> fmts
>>>>>>> callbacks").
>>>>>>>>>
>>>>>>>>> After this patch, the screen becomes greenish(may be it is
>>>>>>>>> setting it
>>>>>>> into YUV format??).
>>>>>>>>>
>>>>>>>>> By checking the code, previously it used to call get_input_fmt
>>>>>>>>> callback
>>>>>>> and set colour as RGB24.
>>>>>>>>>
>>>>>>>>> After this commit, it calls get_output_fmt_callbck and returns 3
>>>>>>>>> outputformats(YUV16, YUV24 and RGB24) And get_input_fmt callback,
>>>>>>>>> I see
>>>>>>> the outputformat as YUV16 instead of RGB24.
>>>>>>>>>
>>>>>>>>> Not sure, I am the only one seeing this issue with dw_HDMI driver.
>>>>>>>
>>>>>>> This patch was introduced to maintain the bridge color format
>>>>>>> negotiation after using DRM_BRIDGE_ATTACH_NO_CONNECTOR, but it
>>>>>>> seems it behaves incorrectly if the first bridge doesn't implement
>>>>>>> the negotiation callbacks.
>>>>>>>
>>>>>>> Let me check the code to see how to fix that.
>>>>>>
>>>>>> Thanks for the information, I am happy to test the patch/fix.
>>>>>>
>>>>>> Cheers,
>>>>>> Biju
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> I have tested linux-next 20220112 on a imx6q-sabresd board, which
>>>>> shows:
>>>>>>>>
>>>>>>>> dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.30a with
>>>>>>>> HDCP (DWC HDMI 3D TX PHY)
>>>>>>>>
>>>>>>>> The colors are shown correctly here.
>>>>>>>>
>>>>>>>
>>>>>>> The imx doesn't use DRM_BRIDGE_ATTACH_NO_CONNECTOR so the
>>>>>>> negotiation fails and use the RGB fallback input & output format.
>>>>>>>
>>>>>>> Anyway thanks for testing
>>>>>>>
>>>>>>> Neil
>>>>>
>>>>> Can you test :
>>>>>
>>>>> ==><===============================
>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c
>>>>> b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7019acd37716
>>>>> 100644
>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>> @@ -955,7 +955,14 @@ drm_atomic_bridge_chain_select_bus_fmts(struct
>>>>> drm_bridge *bridge,
>>>>> last_bridge_state =
>>>>> drm_atomic_get_new_bridge_state(crtc_state-
>>>>>> state,
>>>>>
>>>>> last_bridge);
>>>>>
>>>>> - if (last_bridge->funcs->atomic_get_output_bus_fmts) {
>>>>> + /*
>>>>> + * Only negociate with real values if both end of the bridge
>>> chain
>>>>> + * support negociation callbacks, otherwise you can end in a
>>>>> situation
>>>>> + * where the selected output format doesn't match with the
>>>>> + first
>>>>> bridge
>>>>> + * output format.
>>>>> + */
>>>>> + if (bridge->funcs->atomic_get_input_bus_fmts &&
>>>>> + last_bridge->funcs->atomic_get_output_bus_fmts) {
>>>>> const struct drm_bridge_funcs *funcs =
>>>>> last_bridge->funcs;
>>>>>
>>>>> /*
>>>>> @@ -980,7 +987,12 @@ drm_atomic_bridge_chain_select_bus_fmts(struct
>>>>> drm_bridge *bridge,
>>>>> if (!out_bus_fmts)
>>>>> return -ENOMEM;
>>>>>
>>>>> - if (conn->display_info.num_bus_formats &&
>>>>> + /*
>>>>> + * If first bridge doesn't support negociation, use
>>>>> MEDIA_BUS_FMT_FIXED
>>>>> + * as a safe value for the whole bridge chain
>>>>> + */
>>>>> + if (bridge->funcs->atomic_get_input_bus_fmts &&
>>>>> + conn->display_info.num_bus_formats &&
>>>>> conn->display_info.bus_formats)
>>>>> out_bus_fmts[0] = conn-
>>>>>> display_info.bus_formats[0];
>>>>> else
>>>>> ==><===============================
>>>>>
>>>>> This should exclude your situation where the first bridge doesn't
>>>>> support negociation.
>>>>
>>>> I have tested this fix with Linux next-20220114. Still I see colour
>>> issue.
>>>>
>>>> It is still negotiating and it is calling get_output_fmt_callbck
>>>>
>>>> [ 3.460155] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
>>> MEDIA_BUS_FMT_UYVY8_1X16=0#########
>>>> [ 3.460180] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
>>> MEDIA_BUS_FMT_YUV8_1X24=1#########
>>>> [ 3.460202] ########dw_hdmi_bridge_atomic_get_output_bus_fmts
>>> MEDIA_BUS_FMT_RGB888_1X24=2#########
>>>>
>>>> And In get_input_fmt callback, I See the outputformat as YUV16 instead
>>> of RGB24.
>>>>
>>>> [ 3.460319] ########dw_hdmi_bridge_atomic_get_input_bus_fmts
>>> MEDIA_BUS_FMT_UYVY8_1X16#########
>>>> [ 3.473644] ########hdmi_video_sample
>>> MEDIA_BUS_FMT_UYVY8_1X16#########
>>>
>>> OK, looking at rcar-du, the dw-hdmi bridge is directly connected to the
>>> encoder.
>>
>> Yep.
>>
>>>
>>> Let me figure that out, no sure I can find a clean solution except putting
>>> back RGB24 before YUV.
>>>
>>> Anyway please test that:
>>
>> It works now after reordering.
>>
>> [ 3.493302] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_RGB888_1X24=0#########
>> [ 3.493326] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_YUV8_1X24=1#########
>> [ 3.493348] ########dw_hdmi_bridge_atomic_get_output_bus_fmts MEDIA_BUS_FMT_UYVY8_1X16=2#########
>>
>> [ 3.493463] ########dw_hdmi_bridge_atomic_get_input_bus_fmts MEDIA_BUS_FMT_RGB888_1X24#########
>> [ 3.506797] ########hdmi_video_sample MEDIA_BUS_FMT_RGB888_1X24#########
>>
>> Is it acceptable solution to the users of dw_hdmi driver? May be it is worth to post a patch.
>> at least it is fixing the colour issue??
>
> Yes, it gets back to default behavior before negociation, nevertheless we need to think
> how to handle your use-case correctly at some point.
>
> I'll post this as a patch ASAP so it gets applied before landing in linus master.
>
> Neil
>
>>
>> Regards,
>> Biju
>>
>>>
[...]
I'm not happy with this version since it's merely a hack which makes it work.
Can you test the following change instead, it's correctly handles your situation in a generic manner.
========================><=============================
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 54d8fdad395f..9f2e1cac0ae2 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
if (!output_fmts)
return NULL;
- /* If dw-hdmi is the only bridge, avoid negociating with ourselves */
- if (list_is_singular(&bridge->encoder->bridge_chain)) {
+ /* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */
+ if (list_is_singular(&bridge->encoder->bridge_chain) ||
+ list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) {
*num_output_fmts = 1;
output_fmts[0] = MEDIA_BUS_FMT_FIXED;
@@ -2673,6 +2674,10 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
if (!input_fmts)
return NULL;
+ /* If dw-hdmi is the first bridge fall-back to safe output format */
+ if (list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain))
+ output_fmt = MEDIA_BUS_FMT_FIXED;
+
switch (output_fmt) {
/* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */
case MEDIA_BUS_FMT_FIXED:
========================><=============================
Thanks,
Neil
>>>
>>> Neil
>>>
>>>>
>>>> Regards,
>>>> Biju
>>>>
>>
>
