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

[Paul Cercueil]

Hi Neil,

Any feedback on the other patches?

They look fine to me, but I still need an ack to merge them in 
drm-misc-next.

Cheers,
-Paul


Le jeu., mars 3 2022 at 12:42:02 +0100, Neil Armstrong 
<narmstrong@xxxxxxxxxxxx> a écrit :
> On 03/03/2022 11:40, H. Nikolaus Schaller wrote:
> > Hi Neil,
> >
> > > Am 03.03.2022 um 09:35 schrieb Neil Armstrong 
> > > <narmstrong@xxxxxxxxxxxx>:
> > >
> > > Hi,
> > >
> > > On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
> > > > Hi Neil,
> > > > > Am 02.03.2022 um 15:34 schrieb Neil Armstrong 
> > > > > <narmstrong@xxxxxxxxxxxx>:
> > > > >
> > > > > Hi,
> > > > >
> > > > > > (cross-checked: RGB mode still works if I force 
> > > > > > hdmi->sink_is_hdmi = false)
> > > > >
> > > > > I don't understand what's wrong, can you try to make the logic 
> > > > > select MEDIA_BUS_FMT_YUV8_1X24 instead of 
> > > > > DRM_COLOR_FORMAT_YCBCR422 ?
> > > > I have forced hdmi->sink_is_hdmi = false and replaced
> > > >   	/* Default 8bit RGB fallback */
> > > > -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
> > > > +	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
> > > > And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
> > > > (MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
> > > > So this indicates that YUV conversion is not working properly. 
> > > > Maybe missing some special
> > > > setup.
> > > > What I have to test if it works on a different monitor.
> >
> > Same effect on a Xiaomi monitor (user manual just telling HDMI1,4 
> > compatible), an
> > older Acer video projector and a Sharp TV set.
> >
> > The Xiaomi monitor does not say "No signal" but shows a black 
> > screen. The others
> > do not even report any HDMI signals. All work well with 
> > MEDIA_BUS_FMT_RGB888_1X24.
> >
> > This means the transcoding to YUV does not work properly on the 
> > jz4780 SoC setup.
> >
> > So it looks as if we have to disable it (at least unless someone 
> > finds a fix).
> >
> > > > Not that this specific panel
> > > > (a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV 
> > > > capabilities
> > > > but does not handle them...
> > > > On the other hand this panel works on RasPi and OMAP5 (where I 
> > > > admit I do not know in
> > > > which mode).
> > >
> > > Pretty sure they don't support YUV HDMI output.
> > >
> > > If you can try on a certified HDMI devices like a TV, it would here 
> > > figuring out where comes the issue.
> >
> > I am not sure if the Sharp TV is fully certified but would assume...
> >
> > > > > If your CSC is broken, we'll need to disable it on your platform.
> > > > Indeed.
> > > > So it seems as if we need a mechanism to overwrite 
> > > > dw_hdmi_bridge_atomic_get_output_bus_fmts()
> > > > in our ingenic-dw-hdmi platform specialization [1] to always 
> > > > return MEDIA_BUS_FMT_RGB888_1X24.
> > > > Or alternatively set sink_is_hdmi = false there (unfortunately 
> > > > there is no direct access to
> > > > struct dw_hdmi in a specialization drivers).
> > > > Is this already possible or how can it be done?
> > >
> > > It's not handled yet, but we may add the logic to handle the lack 
> > > of CSC config bit and
> > > add a glue config bit to override this like we already did for CEC.
> > >
> > > I wrote an initial support to disable CSC (only compile-tested), 
> > > could you try on your platform with setting disable_csc = 1 in your 
> > > dw-hdmi glue code ?
> >
> > This works!
> >
> > 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.
> ====================
>
> Thanks,
> Neil
>
> > BR and thanks,
> > Nikolaus