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

[Jassi Brar <jaswinder.singh@linaro.org>]

On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
>
>>> >> --- 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.
>>
> If that could be a problem, then we already have some problem with
> current omapdss.
>
> I think among the first things, while enabling HDMI, should be to see
> if there is really some display connected on the port i.e, HPD
> asserted. Only if ti_hdmi_4_detect() returned true, should we
> proceed otherwise wait for HPQ irq.
>
> Unconditionally invalidating edid really seems like a regression - we
> impose atleast 50ms (edid read) as extra cost on
> hdmi_check_hpd_state(), which kills half the purpose of this patch.
>
Sorry a correction. Reading detect() won't work. I suggest we keep HPD
IRQ enabled for the lifetime of the driver.