Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]

On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
> On 28 June 2012 13:18, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-06-27 at 19:35 +0530, jaswinder.singh@linaro.org wrote:
> >> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>
> >> We can easily keep track of latest EDID from the display and hence avoid
> >> expensive EDID re-reads over I2C.
> >> This could also help some cheapo displays that provide EDID reliably only
> >> immediately after asserting HPD and not later.
> >> Even with good displays, there is something in OMAPDSS that apparantly
> >> messes up DDC occasionally while EDID is being read, giving the
> >>   "operation stopped when reading edid" error.
> >
> > Btw, this is in nitpicking area, but what editor do you use? I find it
> > difficult to read text that is not formatted properly =). At least vim
> > formats text nicely with its formating commands.
> >
> Indeed a nitpick :)
> I use vim and, iirc, checkpatch.pl gave 0 warning. Perhaps my poor
> cmoposition. Please do tell how I could I make it more appealing to
> you ?

Like:

---

We can easily keep track of latest EDID from the display and hence avoid
expensive EDID re-reads over I2C. This could also help some cheapo displays
that provide EDID reliably only immediately after asserting HPD and not later.

Even with good displays, there is something in OMAPDSS that apparantly messes
up DDC occasionally while EDID is being read, giving the "operation stopped
when reading edid" error.

---

So basically two things: properly formatted paragraphs and an empty line
between paragraphs. With "properly formatted" I mean that that the text
goes from the beginning of the line to the end, so that the text fills
the whole line. This can be done easily in vim with first painting the
paragraph (or multiple paragraphs), and then then press g and w.

I think those simple rules make the text much easier to read.

> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> >>
> >>       hpd = gpio_get_value(ip_data->hpd_gpio);
> >>
> >> -     if (hpd)
> >> +     if (hpd) {
> >>               r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> >> -     else
> >> +     } else {
> >> +             /* Invalidate EDID Cache */
> >> +             ip_data->edid_len = 0;
> >>               r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> >> +     }
> >
> > There's a problem with this patch, which leaves a wrong EDID in the
> > cache: if you first have the cable connected and hdmi is enabled, you
> > then turn off the HDMI display device via sysfs, we do not go to
> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
> > the plug-in event, and thus EDID cache is never invalidated.
> >
> If the hdmi cable is not replugged during that period, I don't see why
> would you want the EDID invalidated ?

I wasn't very clear with my comment.

When the display device is disabled, we're not listening to the hpd
interrupt anymore. So when it's disabled, the cable can be replugged and
the monitor changed, and the driver won't know about it. And so it'll
return the old EDID for the new monitor.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]