Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong <narmstrong@baylibre.com>]

Hi,

On 03/03/2022 16:37, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
>> Am 03.03.2022 um 12:45 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>
>> Hi Neil,
>>
>>> Am 03.03.2022 um 12:42 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>>
>>>> So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
>>>> If you have a version with proper commit message I can add it to the beginning of my
>>>> seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
>>>> can build on top.
>>>
>>> You can add it in your v17 patchset with my authorship and my Signed-off-by tag + yours.
>>>
>>> As commit message something like :
>>> ====================
>>> drm/bridge: dw-hdmi: handle unusable or non-configured CSC module
>>>
>>> The dw-hdmi integrates an optional Color Space Conversion feature used
>>> to handle color-space conversions.
>>>
>>> On some platforms, the CSC isn't built-in or non-functional.
>>>
>>> This adds the necessary code to disable the CSC functionality
>>> and limit the bus format negotiation to force using the same
>>> input bus format as the output bus format.
>>> ====================
>>
>> Fine! Will do.
> 
> I was a little too early.
> 
> While preparing the patches I found that I still had the hack to force
> sink_is_hdmi = false in my test branch. Sorry for that.
> 
> Removing this made the panel go black again, even with your latest
> proposal.
> 
> So I looked deeper into your patch and it seems to influence the
> input formats only in dw_hdmi_bridge_atomic_get_input_bus_fmts()?
> 
> While the problem I see is with output formats and we had worked on
> modifying dw_hdmi_bridge_atomic_get_output_bus_fmts().

I just looked and the ingenic drm driver first bridge uses drm_atomic_helper_bridge_propagate_bus_fmt()
which is why this last patch doesn't work, and perhaps would be the main issue here.

Indeed, the core will loop on all the possible output formats for your HDMI monitor :
- MEDIA_BUS_FMT_UYVY8_1X16
- MEDIA_BUS_FMT_YUV8_1X24
- MEDIA_BUS_FMT_RGB888_1X24

For each of these, the dw-hdmi dw_hdmi_bridge_atomic_get_input_bus_fmts() will
return the same format + the possible CSC transformations, for example
for MEDIA_BUS_FMT_UYVY8_1X16 will return as possible inputs:
- MEDIA_BUS_FMT_UYVY8_1X16
- MEDIA_BUS_FMT_YUV8_1X24
- MEDIA_BUS_FMT_RGB888_1X24

The the core will call for each of the those the .atomic_get_input_bus_fmts of
the Ingenic DRM driver, but by using drm_atomic_helper_bridge_propagate_bus_fmt()
it basically sets a pass-through and accepts any format.

This is why MEDIA_BUS_FMT_UYVY8_1X16 is selected, but in this case the ingenic
ingenic_drm_bridge_atomic_check() would fail in the switch.

The Ingenic should implement a proper .atomic_get_input_bus_fmts returning
only the possible supported formats.

Can you check if you hit the default case in ingenic_drm_bridge_atomic_check() ?

Neil

> 
> BR and thanks,
> Nikolaus
>