[PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

Explicitly maintaining HDMI phy power state using a flag is prone to
race and un-necessary when we have a zero-cost alternative of checking
the state before trying to set it.

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

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
 
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
-	bool phy_tx_enabled;
 };
 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 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
 /* PHY_PWR_CMD */
 static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
 {
+	/* Return if already the state */
+	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) == val)
+		return 0;
+
 	/* Command for power control of HDMI PHY */
 	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
 
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 
 	hpd = gpio_get_value(ip_data->hpd_gpio);
 
-	if (hpd == ip_data->phy_tx_enabled) {
-		spin_unlock_irqrestore(&phy_tx_lock, flags);
-		return 0;
-	}
-
 	if (hpd)
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
 	else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		goto err;
 	}
 
-	ip_data->phy_tx_enabled = hpd;
 err:
 	spin_unlock_irqrestore(&phy_tx_lock, flags);
 	return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
 	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
 
 	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
-	ip_data->phy_tx_enabled = false;
 }
 
 static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
-- 
1.7.4.1

[PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

Explicitly maintaining HDMI phy power state using a flag is prone to
race and un-necessary when we have a zero-cost alternative of checking
the state before trying to set it.

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

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
 
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
-	bool phy_tx_enabled;
 };
 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 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
 /* PHY_PWR_CMD */
 static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
 {
+	/* Return if already the state */
+	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) = val)
+		return 0;
+
 	/* Command for power control of HDMI PHY */
 	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
 
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 
 	hpd = gpio_get_value(ip_data->hpd_gpio);
 
-	if (hpd = ip_data->phy_tx_enabled) {
-		spin_unlock_irqrestore(&phy_tx_lock, flags);
-		return 0;
-	}
-
 	if (hpd)
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
 	else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		goto err;
 	}
 
-	ip_data->phy_tx_enabled = hpd;
 err:
 	spin_unlock_irqrestore(&phy_tx_lock, flags);
 	return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
 	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
 
 	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
-	ip_data->phy_tx_enabled = false;
 }
 
 static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
-- 
1.7.4.1

RE: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jingoo Han]

On Saturday 16 June 2012 7:02:00 jaswinder.singh wrote:

> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Explicitly maintaining HDMI phy power state using a flag is prone to
> race and un-necessary when we have a zero-cost alternative of checking
> the state before trying to set it.

CC'ed 'Mythri P K' as the author for OMAP HDMI.

Hi Jassi, long time no see.

This patch looks good.
If there is no problem, checking register is better way.

Best regards,
Jingoo Han.

> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/video/omap2/dss/ti_hdmi.h         |    1 -
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   11 ++++-------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index e734cb4..d174ca1 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -177,7 +177,6 @@ struct hdmi_ip_data {
> 
>  	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>  	int hpd_gpio;
> -	bool phy_tx_enabled;
>  };
>  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 4dae1b2..3fa3d98 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
>  /* PHY_PWR_CMD */
>  static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
>  {
> +	/* Return if already the state */
> +	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) = val)
> +		return 0;
> +
>  	/* Command for power control of HDMI PHY */
>  	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
> 
> @@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> 
>  	hpd = gpio_get_value(ip_data->hpd_gpio);
> 
> -	if (hpd = ip_data->phy_tx_enabled) {
> -		spin_unlock_irqrestore(&phy_tx_lock, flags);
> -		return 0;
> -	}
> -
>  	if (hpd)
>  		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>  	else
> @@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  		goto err;
>  	}
> 
> -	ip_data->phy_tx_enabled = hpd;
>  err:
>  	spin_unlock_irqrestore(&phy_tx_lock, flags);
>  	return r;
> @@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
>  	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
> 
>  	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
> -	ip_data->phy_tx_enabled = false;
>  }
> 
>  static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
> --
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jingoo Han]

On Saturday 16 June 2012 7:02:00 jaswinder.singh wrote:

> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Explicitly maintaining HDMI phy power state using a flag is prone to
> race and un-necessary when we have a zero-cost alternative of checking
> the state before trying to set it.

CC'ed 'Mythri P K' as the author for OMAP HDMI.

Hi Jassi, long time no see.

This patch looks good.
If there is no problem, checking register is better way.

Best regards,
Jingoo Han.

> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/video/omap2/dss/ti_hdmi.h         |    1 -
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   11 ++++-------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index e734cb4..d174ca1 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -177,7 +177,6 @@ struct hdmi_ip_data {
> 
>  	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>  	int hpd_gpio;
> -	bool phy_tx_enabled;
>  };
>  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 4dae1b2..3fa3d98 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
>  /* PHY_PWR_CMD */
>  static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
>  {
> +	/* Return if already the state */
> +	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) == val)
> +		return 0;
> +
>  	/* Command for power control of HDMI PHY */
>  	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
> 
> @@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> 
>  	hpd = gpio_get_value(ip_data->hpd_gpio);
> 
> -	if (hpd == ip_data->phy_tx_enabled) {
> -		spin_unlock_irqrestore(&phy_tx_lock, flags);
> -		return 0;
> -	}
> -
>  	if (hpd)
>  		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>  	else
> @@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  		goto err;
>  	}
> 
> -	ip_data->phy_tx_enabled = hpd;
>  err:
>  	spin_unlock_irqrestore(&phy_tx_lock, flags);
>  	return r;
> @@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
>  	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
> 
>  	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
> -	ip_data->phy_tx_enabled = false;
>  }
> 
>  static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
> --
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Sat, 2012-06-16 at 03:31 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Explicitly maintaining HDMI phy power state using a flag is prone to
> race and un-necessary when we have a zero-cost alternative of checking
> the state before trying to set it.

Why would reading the value from the register be any less racy than
keeping it in memory? And reading from memory is probably much faster
than reading from an HDMI register, so I'm not sure what you mean with
zero-cost.

But I guess it is simpler, so in that sense the patch is ok. But please
revise the description.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Sat, 2012-06-16 at 03:31 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Explicitly maintaining HDMI phy power state using a flag is prone to
> race and un-necessary when we have a zero-cost alternative of checking
> the state before trying to set it.

Why would reading the value from the register be any less racy than
keeping it in memory? And reading from memory is probably much faster
than reading from an HDMI register, so I'm not sure what you mean with
zero-cost.

But I guess it is simpler, so in that sense the patch is ok. But please
revise the description.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> Explicitly maintaining HDMI phy power state using a flag is prone to
>> race and un-necessary when we have a zero-cost alternative of checking
>> the state before trying to set it.
>
> Why would reading the value from the register be any less racy than
> keeping it in memory?
>
Racy in the sense that h/w doesn't always hop states according to what
a "state" variable would expect it to.
Also in this case, phy_tx_enabled modification is unprotected in
ti_hdmi_4xxx_phy_disable().

BTW, coming to think about it, I am not sure what we need the
spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
expensive and is already unprotected elsewhere.

> And reading from memory is probably much faster
> than reading from an HDMI register, so I'm not sure what you mean with
> zero-cost.
>
Zero-cost in terms of space and bother :)

> But I guess it is simpler, so in that sense the patch is ok. But please
> revise the description.
>
OK, will do.

Thanks.

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> Explicitly maintaining HDMI phy power state using a flag is prone to
>> race and un-necessary when we have a zero-cost alternative of checking
>> the state before trying to set it.
>
> Why would reading the value from the register be any less racy than
> keeping it in memory?
>
Racy in the sense that h/w doesn't always hop states according to what
a "state" variable would expect it to.
Also in this case, phy_tx_enabled modification is unprotected in
ti_hdmi_4xxx_phy_disable().

BTW, coming to think about it, I am not sure what we need the
spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
expensive and is already unprotected elsewhere.

> And reading from memory is probably much faster
> than reading from an HDMI register, so I'm not sure what you mean with
> zero-cost.
>
Zero-cost in terms of space and bother :)

> But I guess it is simpler, so in that sense the patch is ok. But please
> revise the description.
>
OK, will do.

Thanks.

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:
> On 18 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> Explicitly maintaining HDMI phy power state using a flag is prone to
> >> race and un-necessary when we have a zero-cost alternative of checking
> >> the state before trying to set it.
> >
> > Why would reading the value from the register be any less racy than
> > keeping it in memory?
> >
> Racy in the sense that h/w doesn't always hop states according to what
> a "state" variable would expect it to.

But I don't think the status register is any better. If I'm not
mistaken, when you set the phy pwr to, say, TXON, the status register
will still show the old value. The status register will be right only
after the HW is actually at TXON state.

In this case the hdmi_set_phy_pwr() function will anyway wait until the
HW has changed states, so both approaches should work just fine.

> Also in this case, phy_tx_enabled modification is unprotected in
> ti_hdmi_4xxx_phy_disable().

So is hdmi_set_phy_pwr(). Note that I'm not saying the current approach
is not racy, but your patch doesn't make it any less racy.

> BTW, coming to think about it, I am not sure what we need the
> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
> expensive and is already unprotected elsewhere.

It's needed when enabling the hdmi output. In phy_enable() the irq is
requested first, and then the phy_enable() runs hdmi_check_hpd_state().
So there's a chance to run hdmi_check_hpd_state() from both
hpd-interrupt and phy_enable() at the same time.

The hdmi_set_phy_pwr() is not called in many places, but I think there's
indeed a problem there. It is called after free_irq(), but I think
(guess) the irq handler could still be running after free_irq. So those
should be protected by the same spinlock too.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:
> On 18 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> Explicitly maintaining HDMI phy power state using a flag is prone to
> >> race and un-necessary when we have a zero-cost alternative of checking
> >> the state before trying to set it.
> >
> > Why would reading the value from the register be any less racy than
> > keeping it in memory?
> >
> Racy in the sense that h/w doesn't always hop states according to what
> a "state" variable would expect it to.

But I don't think the status register is any better. If I'm not
mistaken, when you set the phy pwr to, say, TXON, the status register
will still show the old value. The status register will be right only
after the HW is actually at TXON state.

In this case the hdmi_set_phy_pwr() function will anyway wait until the
HW has changed states, so both approaches should work just fine.

> Also in this case, phy_tx_enabled modification is unprotected in
> ti_hdmi_4xxx_phy_disable().

So is hdmi_set_phy_pwr(). Note that I'm not saying the current approach
is not racy, but your patch doesn't make it any less racy.

> BTW, coming to think about it, I am not sure what we need the
> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
> expensive and is already unprotected elsewhere.

It's needed when enabling the hdmi output. In phy_enable() the irq is
requested first, and then the phy_enable() runs hdmi_check_hpd_state().
So there's a chance to run hdmi_check_hpd_state() from both
hpd-interrupt and phy_enable() at the same time.

The hdmi_set_phy_pwr() is not called in many places, but I think there's
indeed a problem there. It is called after free_irq(), but I think
(guess) the irq handler could still be running after free_irq. So those
should be protected by the same spinlock too.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 16:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:

>> BTW, coming to think about it, I am not sure what we need the
>> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
>> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
>> expensive and is already unprotected elsewhere.
>
> It's needed when enabling the hdmi output. In phy_enable() the irq is
> requested first, and then the phy_enable() runs hdmi_check_hpd_state().
> So there's a chance to run hdmi_check_hpd_state() from both
> hpd-interrupt and phy_enable() at the same time.
>
> The hdmi_set_phy_pwr() is not called in many places, but I think there's
> indeed a problem there. It is called after free_irq(), but I think
> (guess) the irq handler could still be running after free_irq. So those
> should be protected by the same spinlock too.
>
You know TI HDMI better than I do, so I assume your concerns are valid.
So preferably I would move request_threaded_irq() to after
hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
spin_lock_irqsave() in hdmi_check_hpd_state() to some mutex (we don't
want irqs disabled so long as it takes for phy to power on/off).
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 16:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:

>> BTW, coming to think about it, I am not sure what we need the
>> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
>> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
>> expensive and is already unprotected elsewhere.
>
> It's needed when enabling the hdmi output. In phy_enable() the irq is
> requested first, and then the phy_enable() runs hdmi_check_hpd_state().
> So there's a chance to run hdmi_check_hpd_state() from both
> hpd-interrupt and phy_enable() at the same time.
>
> The hdmi_set_phy_pwr() is not called in many places, but I think there's
> indeed a problem there. It is called after free_irq(), but I think
> (guess) the irq handler could still be running after free_irq. So those
> should be protected by the same spinlock too.
>
You know TI HDMI better than I do, so I assume your concerns are valid.
So preferably I would move request_threaded_irq() to after
hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
spin_lock_irqsave() in hdmi_check_hpd_state() to some mutex (we don't
want irqs disabled so long as it takes for phy to power on/off).

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
> On 18 June 2012 16:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:
> 
> >> BTW, coming to think about it, I am not sure what we need the
> >> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
> >> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
> >> expensive and is already unprotected elsewhere.
> >
> > It's needed when enabling the hdmi output. In phy_enable() the irq is
> > requested first, and then the phy_enable() runs hdmi_check_hpd_state().
> > So there's a chance to run hdmi_check_hpd_state() from both
> > hpd-interrupt and phy_enable() at the same time.
> >
> > The hdmi_set_phy_pwr() is not called in many places, but I think there's
> > indeed a problem there. It is called after free_irq(), but I think
> > (guess) the irq handler could still be running after free_irq. So those
> > should be protected by the same spinlock too.
> >
> You know TI HDMI better than I do, so I assume your concerns are valid.
> So preferably I would move request_threaded_irq() to after
> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the

No, you can't move the check. If you move it, the HPD state could change
between the check and the request_irq, and we'd miss it.

> spin_lock_irqsave() in hdmi_check_hpd_state() to some mutex (we don't
> want irqs disabled so long as it takes for phy to power on/off).

Yes, I guess a mutex is better.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
> On 18 June 2012 16:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-18 at 15:42 +0530, Jassi Brar wrote:
> 
> >> BTW, coming to think about it, I am not sure what we need the
> >> spin_lock_irqsave() protection for in hdmi_check_hpd_state() ?  It
> >> can't control HPD gpio state change and hdmi_set_phy_pwr() seems too
> >> expensive and is already unprotected elsewhere.
> >
> > It's needed when enabling the hdmi output. In phy_enable() the irq is
> > requested first, and then the phy_enable() runs hdmi_check_hpd_state().
> > So there's a chance to run hdmi_check_hpd_state() from both
> > hpd-interrupt and phy_enable() at the same time.
> >
> > The hdmi_set_phy_pwr() is not called in many places, but I think there's
> > indeed a problem there. It is called after free_irq(), but I think
> > (guess) the irq handler could still be running after free_irq. So those
> > should be protected by the same spinlock too.
> >
> You know TI HDMI better than I do, so I assume your concerns are valid.
> So preferably I would move request_threaded_irq() to after
> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the

No, you can't move the check. If you move it, the HPD state could change
between the check and the request_irq, and we'd miss it.

> spin_lock_irqsave() in hdmi_check_hpd_state() to some mutex (we don't
> want irqs disabled so long as it takes for phy to power on/off).

Yes, I guess a mutex is better.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:

>> So preferably I would move request_threaded_irq() to after
>> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
>
> No, you can't move the check. If you move it, the HPD state could change
> between the check and the request_irq, and we'd miss it.
>
Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 18:37 +0530, Jassi Brar wrote:
> On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
> 
> >> So preferably I would move request_threaded_irq() to after
> >> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
> >
> > No, you can't move the check. If you move it, the HPD state could change
> > between the check and the request_irq, and we'd miss it.
> >
> Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?

No, if we haven't requested the irq yet. So what could happen:

- initially the cable is unplugged
- ti_hdmi_4xxx_phy_enable() calls hdmi_check_hpd_state(), nothing is
done as cable is unplugged
- cable plugged in
- ti_hdmi_4xxx_phy_enable() calls request_irq. No irq raised, as the
cable's state doesn't change.

We wouldn't know that cable is actually plugged in at that point.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Tomi Valkeinen]

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

On Mon, 2012-06-18 at 18:37 +0530, Jassi Brar wrote:
> On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
> 
> >> So preferably I would move request_threaded_irq() to after
> >> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
> >
> > No, you can't move the check. If you move it, the HPD state could change
> > between the check and the request_irq, and we'd miss it.
> >
> Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?

No, if we haven't requested the irq yet. So what could happen:

- initially the cable is unplugged
- ti_hdmi_4xxx_phy_enable() calls hdmi_check_hpd_state(), nothing is
done as cable is unplugged
- cable plugged in
- ti_hdmi_4xxx_phy_enable() calls request_irq. No irq raised, as the
cable's state doesn't change.

We wouldn't know that cable is actually plugged in at that point.

 Tomi


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

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:

>> So preferably I would move request_threaded_irq() to after
>> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
>
> No, you can't move the check. If you move it, the HPD state could change
> between the check and the request_irq, and we'd miss it.
>
Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 18:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 18:37 +0530, Jassi Brar wrote:
>> On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
>>
>> >> So preferably I would move request_threaded_irq() to after
>> >> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
>> >
>> > No, you can't move the check. If you move it, the HPD state could change
>> > between the check and the request_irq, and we'd miss it.
>> >
>> Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?
>
> No, if we haven't requested the irq yet. So what could happen:
>
> - initially the cable is unplugged
> - ti_hdmi_4xxx_phy_enable() calls hdmi_check_hpd_state(), nothing is
> done as cable is unplugged
> - cable plugged in
> - ti_hdmi_4xxx_phy_enable() calls request_irq. No irq raised, as the
> cable's state doesn't change.
>
> We wouldn't know that cable is actually plugged in at that point.
>
I see, you mean physically (un)plugging the cable could race with phy_enable.

OK, I'll revise the changelog for this patch and submit another patch
converting the spinlock to mutex.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[Jassi Brar]

On 18 June 2012 18:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-18 at 18:37 +0530, Jassi Brar wrote:
>> On 18 June 2012 17:54, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Mon, 2012-06-18 at 17:16 +0530, Jassi Brar wrote:
>>
>> >> So preferably I would move request_threaded_irq() to after
>> >> hdmi_check_hpd_state() in ti_hdmi_4xxx_phy_enable()  and convert the
>> >
>> > No, you can't move the check. If you move it, the HPD state could change
>> > between the check and the request_irq, and we'd miss it.
>> >
>> Wouldn't we then get an irq, and hence another hdmi_check_hpd_state(), for that?
>
> No, if we haven't requested the irq yet. So what could happen:
>
> - initially the cable is unplugged
> - ti_hdmi_4xxx_phy_enable() calls hdmi_check_hpd_state(), nothing is
> done as cable is unplugged
> - cable plugged in
> - ti_hdmi_4xxx_phy_enable() calls request_irq. No irq raised, as the
> cable's state doesn't change.
>
> We wouldn't know that cable is actually plugged in at that point.
>
I see, you mean physically (un)plugging the cable could race with phy_enable.

OK, I'll revise the changelog for this patch and submit another patch
converting the spinlock to mutex.

Thanks.

[PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

It is simpler to read the current status from a register as compared
to maintaining a state variable to hold the information.

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

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
 
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
-	bool phy_tx_enabled;
 };
 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 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
 /* PHY_PWR_CMD */
 static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
 {
+	/* Return if already the state */
+	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) == val)
+		return 0;
+
 	/* Command for power control of HDMI PHY */
 	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
 
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 
 	hpd = gpio_get_value(ip_data->hpd_gpio);
 
-	if (hpd == ip_data->phy_tx_enabled) {
-		spin_unlock_irqrestore(&phy_tx_lock, flags);
-		return 0;
-	}
-
 	if (hpd)
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
 	else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		goto err;
 	}
 
-	ip_data->phy_tx_enabled = hpd;
 err:
 	spin_unlock_irqrestore(&phy_tx_lock, flags);
 	return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
 	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
 
 	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
-	ip_data->phy_tx_enabled = false;
 }
 
 static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
-- 
1.7.4.1

[PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

It is simpler to read the current status from a register as compared
to maintaining a state variable to hold the information.

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

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
 
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
-	bool phy_tx_enabled;
 };
 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 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
 /* PHY_PWR_CMD */
 static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
 {
+	/* Return if already the state */
+	if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) = val)
+		return 0;
+
 	/* Command for power control of HDMI PHY */
 	REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
 
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 
 	hpd = gpio_get_value(ip_data->hpd_gpio);
 
-	if (hpd = ip_data->phy_tx_enabled) {
-		spin_unlock_irqrestore(&phy_tx_lock, flags);
-		return 0;
-	}
-
 	if (hpd)
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
 	else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		goto err;
 	}
 
-	ip_data->phy_tx_enabled = hpd;
 err:
 	spin_unlock_irqrestore(&phy_tx_lock, flags);
 	return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
 	free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
 
 	hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
-	ip_data->phy_tx_enabled = false;
 }
 
 static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
-- 
1.7.4.1