by Neil Armstrong

[permalink] [raw]

Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

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 <[email protected]>
>>>>> Sent: 14 January 2022 13:56
>>>>> To: Biju Das <[email protected]>; Fabio Estevam
>>>>> <[email protected]>
>>>>> Cc: [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected]
>>>>> 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
>>>>>>>>>> <[email protected]>
>>>>>>>>> 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 <[email protected]>
> Tested-by: Kieran Bingham <[email protected]>
>
> 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
>>>>>>
>>>>
>>>
>>

2022-01-18 02:29:15

by Neil Armstrong

[permalink] [raw]

Subject: Re: dw_hdmi is showing wrong colour after commit 7cd70656d1285b79("drm/bridge: display-connector: implement bus fmts callbacks")

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 <[email protected]>
>>> Tested-by: Kieran Bingham <[email protected]>
>>>
>>> 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
>