Message-ID: <eead4707-e73f-f648-edc6-8415bbb25b8b@redhat.com>
Date:   Mon, 2 Jan 2023 14:51:42 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Ondřej Jirman <megi@....cz>,
        linux-kernel@...r.kernel.org,
        Kamil Trzciński <ayufan@...fan.eu>,
        Martijn Braam <martijn@...xit.nl>,
        Sam Ravnborg <sam@...nborg.org>,
        Robert Mader <robert.mader@...teo.de>,
        Tom Fitzhenry <tom@...-fitzhenry.me.uk>,
        Peter Robinson <pbrobinson@...il.com>,
        Onuralp Sezer <thunderbirdtr@...oraproject.org>,
        dri-devel@...ts.freedesktop.org,
        Maya Matuszczyk <maccraft123mc@...il.com>,
        Neal Gompa <ngompa13@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller
 driver

Hello Ondřej,

On 1/2/23 11:59, Ondřej Jirman wrote:

[...]

>> Yes, because as you said were debug printks. Feel free to propose adding the
>> debug printks if you consider useful for normal usage and not just for devel
>> purposes.
> 
> I already did, and used them do debug and fix the issues. This submission just
> doesn't include the fixes.
>

I missed the fixes, I think that cherry-picked and squashed from your tree
before you added commit f19ce7bb7d72 ("arm64: dts: rk3399-pinephone-pro:
Use unused GPLL for VOPs DCLK") at least.
 
>>> hooks. Have you tested the driver thoroughly with various DRM apps,
>>> with DPM/suspend/resume, etc.?
>>>
>>
>> I did not. I wasn't expecting suspend and resume to work on the PPP given its
>> support is quite minimal currently.
> 
> System suspend/resume works and is used by distributions. Display blanking is
> also used by normal distros, even if you don't use system suspend/resume.
>

I know but my point was that the PPP mainline support isn't ready to be used
as a daily driver in practice. So I didn't consider susped/resume or display
blank as a requirement to upstream an initial support for the panel driver.

[...]

>>> Also, have you checked the clocks are actually configured correctly by the
>>> rk3399 cru driver? I have a lot of trouble with that, too. clk driver sometimes
>>> selects the fractional clock, but does not give it the necessary >20x difference
>>> between input/output clock rates. You'll only notice if you measure clock rates
>>> directly, by looking at actual refresh rate, by using some testing DRM app.
>>> Clock subsystem sometimes shuffles things around if you switch VOPs and use big
>>> VOP for mipi-dsi display, instead of the default small VOP.
>>>
>>
>> I have not. Just verified that the display was working on my PPP and could start
>> a mutter wayland session. We could fix the clock configuration as follow-up IMO.
> 
> The display output will be broken after you fix the assigned-clocks in DT to
> expected values (use GPLL parent, to make the HW generate the exact pixel clock
> defined in the display mode). So this needs to be dealt with now, not later.
> 
> 
> The driver issues are all known at this time and have fixes available, unlike
> a year ago:
> 

My goal was to have some initial support in mainline even if there could be some
issues. IMO it is better to use upstream as a baseline and attempt to support the
PPP incrementally.

But since you are aware of the issues and know what are the available fixes, I'll
let you continue with the effort and take care of the patches. Hopefully there may
be things that will be helpful, such as the binding schema patch and the collected
tags. I can also take care of pushing the DRM bits to the drm-misc-next tree once
you feel that those are ready to get merged.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Powered by blists - more mailing lists