Message-ID: <194631de-2e3f-6e1f-65f6-76dbef04483e@collabora.com>
Date:   Wed, 20 Jul 2022 09:49:35 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Doug Anderson <dianders@...omium.org>,
        Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
Cc:     kernel@...labora.com, Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Sam Ravnborg <sam@...nborg.org>,
        Thierry Reding <thierry.reding@...il.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] drm/panel-edp: Add panel entry for R140NWF5 RH

Il 20/07/22 00:40, Doug Anderson ha scritto:
> Hi,
> 
> On Tue, Jul 19, 2022 at 1:39 PM Nícolas F. R. A. Prado
> <nfraprado@...labora.com> wrote:
>>
>> Add panel identification entry for the IVO R140NWF5 RH (product ID:
>> 0x057d) panel.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
>>
>> ---
>> The comments on the driver indicate that the T3 timing should be set on
>> hpd_absent, while hpd_reliable would have a shorter time just while the
>> HPD line stabilizes on low after power is supplied.
> 
> Right. Ideally hpd_reliable is 0 unless you've got a badly-designed panel.
> 
> 
>> But can we really assume that the HPD line will be reliable at all
>> before the DDIC is done booting up, at which point the HPD line is
>> brought up? IOW, shouldn't we always delay T3 (by setting hpd_reliable =
>> T3), since only then we're really sure that the DDIC is done setting up
>> and the HPD line is reliable?
> 
> If the panel is hooked up properly, then the HPD pin should be pulled
> low at the start and then should only go high after the panel is ready
> for us to talk to it, right? So it's not like the DDIC has to boot up
> and actively init the state. I would assume that the initial state of
> the "HPD output" from the panel's IC would be one of the following:
> * A floating input.
> * A pulled down input.
> * An output driven low.
> 
> In any of those cases just adding a pull down on the line would be
> enough to ensure that the HPD line is reliable until the panel comes
> around and actively drives the line high.
> 
> Remember, this is eDP and it's not something that's hot-plugged, so
> there's no debouncing involved and in a properly designed system there
> should be no time needed for the signal to stabilize. I would also
> point out that on the oficial eDP docs the eDP timing diagram doesn't
> show the initial state of "HPD" as "unknown". It shows it as low.
> 
> Now, that all being said, I have seen at least one panel that glitched
> itself at bootup. After you powered it on it would blip its HPD line
> high before it had actually finished booting. Then the HPD would go
> low again before finally going high after the panel finished booting.
> This is the reason for "hpd_reliable".
> 
> If you've got a board with a well-designed panel but the hookup
> between the panel and the board is wrong (maybe the board is missing a
> pulldown on the HPD line?) then you can just set the "no-hpd" property
> for your board. That will tell the kernel to just always delay the
> "hpd-absent" delay.
> 

We were concerned exactly about glitchy HPD during DDIC init, as I didn't
want to trust it because the only testing we could do was on just two units...

...but if you're sure that I was too much paranoid about that, that's good,
as it means I will be a bit more "relaxed" on this topic next time :-)

>> I've set the T3 delay to hpd_absent in this series, following what's
>> instructed in the comments, but I'd like to discuss whether we shouldn't
>> be setting T3 on hpd_reliable instead, for all panels, to be on the
>> safer side.
> 
> The way it's specified right now is more flexible, though, isn't it?
> This way if you're on a board where the HPD truly _isn't_ stable then
> you can just set the "no-hpd" and it will automatically use the
> "hpd_absent" delay, right?
> 

For Chromebooks, that's totally doable, thanks to the bootloader seeking for
proper machine compatibles, so yes I agree with that.

> 
>>   drivers/gpu/drm/panel/panel-edp.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
>> index 3626469c4cc2..675d793d925e 100644
>> --- a/drivers/gpu/drm/panel/panel-edp.c
>> +++ b/drivers/gpu/drm/panel/panel-edp.c
>> @@ -1854,6 +1854,12 @@ static const struct panel_delay delay_100_500_e200 = {
>>          .enable = 200,
>>   };
>>
>> +static const struct panel_delay delay_200_500_e200 = {
>> +       .hpd_absent = 200,
>> +       .unprepare = 500,
>> +       .enable = 200,
>> +};
>> +
>>   #define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
>>   { \
>>          .name = _name, \
>> @@ -1882,6 +1888,8 @@ static const struct edp_panel_entry edp_panels[] = {
>>
>>          EDP_PANEL_ENTRY('C', 'M', 'N', 0x114c, &innolux_n116bca_ea1.delay, "N116BCA-EA1"),
>>
>> +       EDP_PANEL_ENTRY('I', 'V', 'O', 0x057d, &delay_200_500_e200, "R140NWF5 RH"),
>> +
> 
> This looks fine to me:
> 
> Reviewed-by: Douglas Anderson <dianders@...omium.org>
> 
> I'm happy to apply this in a day or two assuming you're OK with my
> explanation above.

Thank you for the long mail, your explanation was truly helpful!
(especially for me being paranoid :-P)

So, I agree to go with that one, for which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>

Nic, green light?


Cheers, (and thanks again!)
Angelo

Powered by blists - more mailing lists