[PATCH] OMAPDSS: HDMI: Cache EDID

[PATCH] OMAPDSS: HDMI: Cache EDID

[jaswinder.singh]

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.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/video/omap2/dss/hdmi.c            |    1 +
 drivers/video/omap2/dss/ti_hdmi.h         |    2 ++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   23 ++++++++++++++++++++---
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 900e621..0a8c825 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 	hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
 	hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
 	hdmi.ip_data.phy_offset = HDMI_PHY;
+	hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
 	mutex_init(&hdmi.ip_data.lock);
 
 	hdmi_panel_init();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index cc292b8..4735860 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -178,6 +178,8 @@ struct hdmi_ip_data {
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
 	struct mutex lock;
+	u8 edid_cached[256];
+	unsigned edid_len;
 };
 int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
 void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 04acca9..2633ade 100644
--- 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);
+	}
 
 	if (r) {
 		DSSERR("Failed to %s PHY TX power\n",
@@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
 {
 	int r, l;
 
+	if (ip_data->edid_len) {
+		memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
+		return ip_data->edid_len;
+	}
+
 	if (len < 128)
 		return -EINVAL;
 
@@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
 		l += 128;
 	}
 
+	ip_data->edid_len = l;
+	memcpy(ip_data->edid_cached, edid, l);
+
 	return l;
 }
 
 bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
 {
-	return gpio_get_value(ip_data->hpd_gpio);
+	if (gpio_get_value(ip_data->hpd_gpio))
+		return true;
+
+	/* Invalidate EDID Cache */
+	ip_data->edid_len = 0;
+
+	return false;
 }
 
 static void hdmi_core_init(struct hdmi_core_video_config *video_cfg,
-- 
1.7.4.1

Re: [PATCH] OMAPDSS: HDMI: Cache EDID

[Tomi Valkeinen]

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

On Sat, 2012-06-23 at 13:36 +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.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/video/omap2/dss/hdmi.c            |    1 +
>  drivers/video/omap2/dss/ti_hdmi.h         |    2 ++
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   23 ++++++++++++++++++++---
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 900e621..0a8c825 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
>  	hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
>  	hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
>  	hdmi.ip_data.phy_offset = HDMI_PHY;
> +	hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
>  	mutex_init(&hdmi.ip_data.lock);

Your HDMI patches seem to depend on each other. Please post them as a
proper patch series, instead of each one separately.

>  	hdmi_panel_init();
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index cc292b8..4735860 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -178,6 +178,8 @@ struct hdmi_ip_data {
>  	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>  	int hpd_gpio;
>  	struct mutex lock;
> +	u8 edid_cached[256];
> +	unsigned edid_len;
>  };
>  int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
>  void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index 04acca9..2633ade 100644
> --- 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);
> +	}
>  
>  	if (r) {
>  		DSSERR("Failed to %s PHY TX power\n",
> @@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>  {
>  	int r, l;
>  
> +	if (ip_data->edid_len) {
> +		memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
> +		return ip_data->edid_len;
> +	}
> +
>  	if (len < 128)
>  		return -EINVAL;
>  
> @@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>  		l += 128;
>  	}
>  
> +	ip_data->edid_len = l;
> +	memcpy(ip_data->edid_cached, edid, l);
> +
>  	return l;
>  }
>  
>  bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
>  {
> -	return gpio_get_value(ip_data->hpd_gpio);
> +	if (gpio_get_value(ip_data->hpd_gpio))
> +		return true;
> +
> +	/* Invalidate EDID Cache */
> +	ip_data->edid_len = 0;
> +
> +	return false;

Why is this needed? The HPD interrupt should handle this already. And if
the HPD interrupt doesn't work for some reason, this won't work either,
as the user can plug and unplug his HDMI monitors a thousand times
between two detect calls.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Cache EDID

[Jassi Brar]

On 25 June 2012 12:05, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +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.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>> ---
>>  drivers/video/omap2/dss/hdmi.c            |    1 +
>>  drivers/video/omap2/dss/ti_hdmi.h         |    2 ++
>>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   23 ++++++++++++++++++++---
>>  3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
>> index 900e621..0a8c825 100644
>> --- a/drivers/video/omap2/dss/hdmi.c
>> +++ b/drivers/video/omap2/dss/hdmi.c
>> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
>>       hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
>>       hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
>>       hdmi.ip_data.phy_offset = HDMI_PHY;
>> +     hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
>>       mutex_init(&hdmi.ip_data.lock);
>
> Your HDMI patches seem to depend on each other. Please post them as a
> proper patch series, instead of each one separately.
>
They don't depend functionality wise. Any fix can be accepted
regardless of others.
I deliberately avoided a series, because revision of just one could
require resending 3, otherwise
perfectly OK, patches. I just wanted to limit the noise.
I understand, 'git am' might complain but I think that should be trivial to fix.
I am perfectly OK to resend as a patch series, if you want.

>>       hdmi_panel_init();
>> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
>> index cc292b8..4735860 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi.h
>> +++ b/drivers/video/omap2/dss/ti_hdmi.h
>> @@ -178,6 +178,8 @@ struct hdmi_ip_data {
>>       /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>>       int hpd_gpio;
>>       struct mutex lock;
>> +     u8 edid_cached[256];
>> +     unsigned edid_len;
>>  };
>>  int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
>>  void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> index 04acca9..2633ade 100644
>> --- 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);
>> +     }
>>
>>       if (r) {
>>               DSSERR("Failed to %s PHY TX power\n",
>> @@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>>  {
>>       int r, l;
>>
>> +     if (ip_data->edid_len) {
>> +             memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
>> +             return ip_data->edid_len;
>> +     }
>> +
>>       if (len < 128)
>>               return -EINVAL;
>>
>> @@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>>               l += 128;
>>       }
>>
>> +     ip_data->edid_len = l;
>> +     memcpy(ip_data->edid_cached, edid, l);
>> +
>>       return l;
>>  }
>>
>>  bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
>>  {
>> -     return gpio_get_value(ip_data->hpd_gpio);
>> +     if (gpio_get_value(ip_data->hpd_gpio))
>> +             return true;
>> +
>> +     /* Invalidate EDID Cache */
>> +     ip_data->edid_len = 0;
>> +
>> +     return false;
>
> Why is this needed? The HPD interrupt should handle this already. And if
> the HPD interrupt doesn't work for some reason, this won't work either,
> as the user can plug and unplug his HDMI monitors a thousand times
> between two detect calls.
>
I thought it is almost impossible to unplug->plug cycle the HDMI cable
even twice a second, let alone 1000 times against the 10Hz .detect()
poll  :)    Or you mean some relay controlled HDMI switching
mechanism?

 Anyways, that is not the point of this patch. This patch only aims to
avoid un-ncessary EDID reads while doing its best to make sure we
invalidate EDID 'as soon as possible'.

Thanks.

Re: [PATCH] OMAPDSS: HDMI: Cache EDID

[Tomi Valkeinen]

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

On Mon, 2012-06-25 at 13:26 +0530, Jassi Brar wrote:
> On 25 June 2012 12:05, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +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.
> >>
> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >> ---
> >>  drivers/video/omap2/dss/hdmi.c            |    1 +
> >>  drivers/video/omap2/dss/ti_hdmi.h         |    2 ++
> >>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   23 ++++++++++++++++++++---
> >>  3 files changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> >> index 900e621..0a8c825 100644
> >> --- a/drivers/video/omap2/dss/hdmi.c
> >> +++ b/drivers/video/omap2/dss/hdmi.c
> >> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
> >>       hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
> >>       hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
> >>       hdmi.ip_data.phy_offset = HDMI_PHY;
> >> +     hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
> >>       mutex_init(&hdmi.ip_data.lock);
> >
> > Your HDMI patches seem to depend on each other. Please post them as a
> > proper patch series, instead of each one separately.
> >
> They don't depend functionality wise. Any fix can be accepted
> regardless of others.
> I deliberately avoided a series, because revision of just one could
> require resending 3, otherwise
> perfectly OK, patches. I just wanted to limit the noise.

You don't need to send the whole series, you can just send a revised
patch as a reply to the older version of that patch. (see --in-reply-to
of git send-email).

Of course if you end up changing many of the patches, or one patch lots
of times, it is good to send the whole series at some point later when
the patches have stabilized.

> I understand, 'git am' might complain but I think that should be trivial to fix.

I'd rather not spend time doing trivial fixes, or trying to find latest
versions of individual patches that have dependencies. Having the
patches in a series and replying with new versions to the older versions
makes my life much easier. I'll see all of them in my email client in
one bunch, properly sorted.

> I am perfectly OK to resend as a patch series, if you want.

Yes please.

> >>  bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
> >>  {
> >> -     return gpio_get_value(ip_data->hpd_gpio);
> >> +     if (gpio_get_value(ip_data->hpd_gpio))
> >> +             return true;
> >> +
> >> +     /* Invalidate EDID Cache */
> >> +     ip_data->edid_len = 0;
> >> +
> >> +     return false;
> >
> > Why is this needed? The HPD interrupt should handle this already. And if
> > the HPD interrupt doesn't work for some reason, this won't work either,
> > as the user can plug and unplug his HDMI monitors a thousand times
> > between two detect calls.
> >
> I thought it is almost impossible to unplug->plug cycle the HDMI cable
> even twice a second, let alone 1000 times against the 10Hz .detect()
> poll  :)    Or you mean some relay controlled HDMI switching
> mechanism?

omapdss doesn't call the detect function, so it can't presume it's used
in some certain frequency. Also, last time I tested omapdrm, I think
detect was called once in 5 secs or so.

>  Anyways, that is not the point of this patch. This patch only aims to
> avoid un-ncessary EDID reads while doing its best to make sure we
> invalidate EDID 'as soon as possible'.

I'm not sure I understand your point. If the HPD interrupt works
properly, EDID is invalidated there, and the code in detect is not
needed. And if the HPD interrupt doesn't work (although I don't see why
it wouldn't), the code in detect doesn't work. In either case it's not
correct.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Cache EDID

[Jassi Brar]

On 25 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> I am perfectly OK to resend as a patch series, if you want.
>
> Yes please.
>
OK, will do.

>> >>  bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
>> >>  {
>> >> -     return gpio_get_value(ip_data->hpd_gpio);
>> >> +     if (gpio_get_value(ip_data->hpd_gpio))
>> >> +             return true;
>> >> +
>> >> +     /* Invalidate EDID Cache */
>> >> +     ip_data->edid_len = 0;
>> >> +
>> >> +     return false;
>> >
>> > Why is this needed? The HPD interrupt should handle this already. And if
>> > the HPD interrupt doesn't work for some reason, this won't work either,
>> > as the user can plug and unplug his HDMI monitors a thousand times
>> > between two detect calls.
>> >
>> I thought it is almost impossible to unplug->plug cycle the HDMI cable
>> even twice a second, let alone 1000 times against the 10Hz .detect()
>> poll  :)    Or you mean some relay controlled HDMI switching
>> mechanism?
>
> omapdss doesn't call the detect function, so it can't presume it's used
> in some certain frequency. Also, last time I tested omapdrm, I think
> detect was called once in 5 secs or so.
>
It's not omapdss.  It's the DRM stack, via the omapdrm, that polls
every 10 secs (not 5).
Sorry I said 10Hz instead of 1/10Hz.

>>  Anyways, that is not the point of this patch. This patch only aims to
>> avoid un-ncessary EDID reads while doing its best to make sure we
>> invalidate EDID 'as soon as possible'.
>
> I'm not sure I understand your point. If the HPD interrupt works
> properly, EDID is invalidated there, and the code in detect is not
> needed. And if the HPD interrupt doesn't work (although I don't see why
> it wouldn't), the code in detect doesn't work. In either case it's not
> correct.
>
Well, the idea was to tie edid-cache invalidating with de-asserted
HPD, wherever we read HPD. I will drop it.

thnx