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

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

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 ?

>> --- 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 ?