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

[Neil Armstrong <narmstrong@baylibre.com>]

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