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

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

On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote:
> On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included:
>
>> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
>>>
>>> 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.
>>
>>
>> Ok, I see. But that's not acceptable. It would require us to keep the
>> TPD12S015 always powered and enabled. Even if you're not interested in
>> using HDMI at all.
>>
>> For this to work like you want we need a bigger restructuring of HDMI
>> and partly omapdss also. Currently we have just one big "enabled" or
>> "disabled" state. We need multiple states. Probably something like:
>>
>> - disabled, everything totally off
>> - low power, hotplug detection enabled
>> - powered on, but no video
>> - video enabled
>>
>> Been long in my mind, but I'm not very familiar with HDMI so I could get
>> my simple prototypes to work when I tried something like this once.
>
>
> That doesn't sound too hard since the difference between the first three
> states at the HDMI chip is just whether the two gpio controlling it are 00,
> 10 or 11.
>
> If Jassi's alright with it we might have a go at implementing this, but can
> you define a bit more about how we logically tell DSS that we want to, eg,
> disable HDMI totally?
>
A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during
probe and disable during remove.
HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only
HDMI_PHY on/off.
The user selecting "Autodetect and Configure" option would then equate
to "(un)loading" of the HDMI driver.
Not to mean a trivial job.