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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

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.

> 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 |   15 +++++++++++++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 0738090..9853621 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -758,6 +758,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..b5c3dc4 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);
> +	}

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.

Perhaps the EDID cache should be invalidated always in
hdmi_check_hpd_state. In that case I think there's a chance
(theoretical, perhaps), that we get two hpd interrupts, and for both the
hpd gpio reads 1. I'm not quite sure what that would imply (perhaps we
just missed the hpd gpio = 0 case), but even in that case invalidating
the cache sounds ok.

 Tomi


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