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

On 28 June 2012 17:33, Andy Green [off-list ref] wrote:

On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included:

quoted

On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:

quoted

On 28 June 2012 16:17, Jassi Brar [off-list ref] wrote:

quoted

On 28 June 2012 15:44, Tomi Valkeinen [off-list ref] wrote:

quoted

On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:

quoted

quoted

quoted

quoted

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