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@xxxxxxxxxxxxx> wrote:
Add panel identification entry for the IVO R140NWF5 RH (product ID:
0x057d) panel.
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Nic, green light?
Cheers, (and thanks again!)
Angelo
