Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-04-26 Thread Tomi Valkeinen 
On Wed, 2012-04-25 at 18:01 -0500, Ricardo Neri wrote:

  If so, we need to make sure it's not currently called in an atomic
  context, because it would break if the function will sleep in the
  future. And with make sure I just mean that we check the code and keep
  it in mind. Or perhaps adding a comment in the header, that says
  audio_enable may sleep, other audio functions do not sleep or such.
 
 I revisited the ALSA code. Only the audio_start function is atomic. 
 Although ALSA may not be the only user, it makes sense to me to think 
 that they will follow a similar approach in terms of locks.
 
 Hence, based on that and on the reasons you describe (audio_enable 
 potentially taking too long to return), Rephrasing what you stated, a 
 mutex may be used for the enable/disable and config operations. Only 
 start/stop would be protected by a spinlock. This should be described in 
 comments in the header file. Does it make sense to you?

Yes. Well, you don't need to mention the locks, they are internal
implementation details. It should be enough to say that start/stop never
sleeps and the other functions may sleep.

And, this is obvious but just to be sure, note that you should use
spinlocks in all of the functions if possible. We just need to make sure
we can use mutex in the future if we need to.

 Tomi



signature.asc
Description: This is a digitally signed message part

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-04-25 Thread Tomi Valkeinen 
On Tue, 2012-04-24 at 23:48 -0500, Ricardo Neri wrote:
 On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:
  On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
  Implement the DSS device driver audio support interface in the HDMI
  panel driver and generic driver. The implementation relies on the
  IP-specific functions that are defined at DSS probe time.
 
  A HW-safe spinlock is used to protect the audio functions. This is because
 
  What is a HW-safe spinlock?
 Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.
 
 
  the audio functions may be called while holding a lock; in such case,
  the panel's driver mutex is not suitable. Functions should be used
  to set registers and should not wait for any other event.
 
  Are you sure this is the only option? What lock is being held?
 For instance, ALSA calls the start audio function while holding a 
 hardirq-safe readlock. Hence, when reaching the HDMI panel start 
 function, a lock is held and irqs are disabled. Using a mutex, that 
 might sleep, is not correct; nor it is using an hardirq-unsafe spinlock. 
 Otherwise, deadlocks and/or inverse lock ordering may arise. This 
 situation was signaled by lockdep.
 
 IMHO, as the DSS device driver does not know who is going to use it (at 
 least the audio part), it should not assume that no locks are held when 
 its functions are called.
 
  While a spinlock may be ok for now, quite often enabling/disabling things 
  do not
  happen immediately,and it's much easier to do the wait synchronously.
 I don't understand this comment. To me, holding a lock until the 
 enabling function returns is synchronous. Would you please clarify?

I meant that quite often when enabling things on hardware it takes time
until the HW is actually up and running. Perhaps a regulator needs to be
started, or such. And it's usually simpler to wait for the HW to be up
synchronously in the enable function, instead of some kind of
asynchronous mechanism. And if a function waits synchronously, a mutex
is better than spinlock.

And in that sense it's often better to define (define meaning, adding a
comment, or just mentally taking note about it) that the functions in
the API may sleep, so that the driver is free to do what is best for the
case, and it's also future-proof in a way that you can easily add
function calls that sleep to the functions in the future.

But you are also right in your previous comment, it's better to define
functions so that they never sleep, as that gives the callers the
freedom to call the funcs in atomic context.

Perhaps we can have audio_start() that never sleeps, it just enables a
few bits that start the audio. But how about audio_enable()? Is it
possible that on some OMAP version it would need to enable a regulator,
or set a gpio that's in an external i2c controlled mux chip, or such?

If so, we need to make sure it's not currently called in an atomic
context, because it would break if the function will sleep in the
future. And with make sure I just mean that we check the code and keep
it in mind. Or perhaps adding a comment in the header, that says
audio_enable may sleep, other audio functions do not sleep or such.

 Tomi



signature.asc
Description: This is a digitally signed message part

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-04-25 Thread Ricardo Neri 

+alsa-devel list

On 04/25/2012 01:19 AM, Tomi Valkeinen wrote:

On Tue, 2012-04-24 at 23:48 -0500, Ricardo Neri wrote:

On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:

Implement the DSS device driver audio support interface in the HDMI
panel driver and generic driver. The implementation relies on the
IP-specific functions that are defined at DSS probe time.

A HW-safe spinlock is used to protect the audio functions. This is because


What is a HW-safe spinlock?

Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.




the audio functions may be called while holding a lock; in such case,
the panel's driver mutex is not suitable. Functions should be used
to set registers and should not wait for any other event.


Are you sure this is the only option? What lock is being held?

For instance, ALSA calls the start audio function while holding a
hardirq-safe readlock. Hence, when reaching the HDMI panel start
function, a lock is held and irqs are disabled. Using a mutex, that
might sleep, is not correct; nor it is using an hardirq-unsafe spinlock.
Otherwise, deadlocks and/or inverse lock ordering may arise. This
situation was signaled by lockdep.

IMHO, as the DSS device driver does not know who is going to use it (at
least the audio part), it should not assume that no locks are held when
its functions are called.


While a spinlock may be ok for now, quite often enabling/disabling things do not
happen immediately,and it's much easier to do the wait synchronously.

I don't understand this comment. To me, holding a lock until the
enabling function returns is synchronous. Would you please clarify?


I meant that quite often when enabling things on hardware it takes time
until the HW is actually up and running. Perhaps a regulator needs to be
started, or such. And it's usually simpler to wait for the HW to be up
synchronously in the enable function, instead of some kind of
asynchronous mechanism. And if a function waits synchronously, a mutex
is better than spinlock.

And in that sense it's often better to define (define meaning, adding a
comment, or just mentally taking note about it) that the functions in
the API may sleep, so that the driver is free to do what is best for the
case, and it's also future-proof in a way that you can easily add
function calls that sleep to the functions in the future.

But you are also right in your previous comment, it's better to define
functions so that they never sleep, as that gives the callers the
freedom to call the funcs in atomic context.

Perhaps we can have audio_start() that never sleeps, it just enables a
few bits that start the audio. But how about audio_enable()? Is it
possible that on some OMAP version it would need to enable a regulator,
or set a gpio that's in an external i2c controlled mux chip, or such?


I think it might be possible to have such a scenario if, for instance, 
an external chip is used to support DisplayPort on OMAP4/5. As 
DisplayPort can support audio-only use-cases, it would have to enable 
the adapter chip (but HDMI output would have to be enabled to feed the 
chip, though).


If so, we need to make sure it's not currently called in an atomic
context, because it would break if the function will sleep in the
future. And with make sure I just mean that we check the code and keep
it in mind. Or perhaps adding a comment in the header, that says
audio_enable may sleep, other audio functions do not sleep or such.


I revisited the ALSA code. Only the audio_start function is atomic. 
Although ALSA may not be the only user, it makes sense to me to think 
that they will follow a similar approach in terms of locks.


Hence, based on that and on the reasons you describe (audio_enable 
potentially taking too long to return), Rephrasing what you stated, a 
mutex may be used for the enable/disable and config operations. Only 
start/stop would be protected by a spinlock. This should be described in 
comments in the header file. Does it make sense to you?


BR,

Ricardo


  Tomi


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

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-04-24 Thread Ricardo Neri 

On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:

Implement the DSS device driver audio support interface in the HDMI
panel driver and generic driver. The implementation relies on the
IP-specific functions that are defined at DSS probe time.

A HW-safe spinlock is used to protect the audio functions. This is because


What is a HW-safe spinlock?

Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.




the audio functions may be called while holding a lock; in such case,
the panel's driver mutex is not suitable. Functions should be used
to set registers and should not wait for any other event.


Are you sure this is the only option? What lock is being held?
For instance, ALSA calls the start audio function while holding a 
hardirq-safe readlock. Hence, when reaching the HDMI panel start 
function, a lock is held and irqs are disabled. Using a mutex, that 
might sleep, is not correct; nor it is using an hardirq-unsafe spinlock. 
Otherwise, deadlocks and/or inverse lock ordering may arise. This 
situation was signaled by lockdep.


IMHO, as the DSS device driver does not know who is going to use it (at 
least the audio part), it should not assume that no locks are held when 
its functions are called.



While a spinlock may be ok for now, quite often enabling/disabling things do not
happen immediately,and it's much easier to do the wait synchronously.
I don't understand this comment. To me, holding a lock until the 
enabling function returns is synchronous. Would you please clarify?





Signed-off-by: Ricardo Neriricardo.n...@ti.com
---
  drivers/video/omap2/dss/dss.h|7 +++
  drivers/video/omap2/dss/hdmi.c   |   33 +++
  drivers/video/omap2/dss/hdmi_panel.c |   76 ++
  3 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 32ff69f..fca4490 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
  bool omapdss_hdmi_detect(void);
  int hdmi_panel_init(void);
  void hdmi_panel_exit(void);
+#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
+int hdmi_audio_enable(bool enable);
+int hdmi_audio_start(bool start);
+bool hdmi_mode_has_audio(void);
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+   struct snd_cea_861_aud_if *aud_if);
+#endif

  /* RFBI */
  #ifdef CONFIG_OMAP2_DSS_RFBI
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index bd44891..880509d 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)

return 0;
  }
+
+int hdmi_audio_enable(bool enable)
+{
+   DSSDBG(audio_enable\n);
+
+   hdmi.ip_data.ops-audio_enable(hdmi.ip_data, enable);


Shouldn't this, and the others below, return the value from the called
function, instead of always returning 0?


Yes, at the time of writing this patch, the HDMI ops that you refer to 
returned void. In v2, I will submit a patch for the HDMI ops to return a 
result value which will also be returned by the DSS audio interface 
functions just as you point out.



+
+   return 0;
+}
+
+int hdmi_audio_start(bool start)
+{
+   DSSDBG(audio_enable\n);
+
+   hdmi.ip_data.ops-audio_start(hdmi.ip_data, start);
+
+   return 0;
+}
+
+bool hdmi_mode_has_audio(void)
+{
+   if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
+   return true;
+   else
+   return false;
+}
+
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+   struct snd_cea_861_aud_if *aud_if)
+{
+   return hdmi.ip_data.ops-audio_config(hdmi.ip_data, iec, aud_if);
+}
+
  #endif

  /* HDMI HW IP initialisation */
diff --git a/drivers/video/omap2/dss/hdmi_panel.c 
b/drivers/video/omap2/dss/hdmi_panel.c
index 533d5dc..dac1ac2 100644
--- a/drivers/video/omap2/dss/hdmi_panel.c
+++ b/drivers/video/omap2/dss/hdmi_panel.c
@@ -31,6 +31,10 @@

  static struct {
struct mutex hdmi_lock;
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+   /* protects calls to HDMI driver audio functionality */
+   spinlock_t hdmi_sp_lock;


What does sp stand for? Spinlock? Perhaps a better name would be
audio_lock, if it's audio specific. And probably no reason to prefix
it with hdmi, as it's inside hdmi struct already.
Yes, sp stands for spinlock. I prefixed the name with hdmi to align 
with the naming of the HDMI mutex. Maybe audio_lock is a better name. I 
could also submit a patch to remove the hdmi prefix in the mutex name.


BR,

Ricardo


  Tomi


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

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-04-23 Thread Tomi Valkeinen 
On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
 Implement the DSS device driver audio support interface in the HDMI
 panel driver and generic driver. The implementation relies on the
 IP-specific functions that are defined at DSS probe time.
 
 A HW-safe spinlock is used to protect the audio functions. This is because

What is a HW-safe spinlock?

 the audio functions may be called while holding a lock; in such case,
 the panel's driver mutex is not suitable. Functions should be used
 to set registers and should not wait for any other event.

Are you sure this is the only option? What lock is being held? While a
spinlock may be ok for now, quite often enabling/disabling things do not
happen immediately, and it's much easier to do the wait synchronously.

 Signed-off-by: Ricardo Neri ricardo.n...@ti.com
 ---
  drivers/video/omap2/dss/dss.h|7 +++
  drivers/video/omap2/dss/hdmi.c   |   33 +++
  drivers/video/omap2/dss/hdmi_panel.c |   76 
 ++
  3 files changed, 116 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
 index 32ff69f..fca4490 100644
 --- a/drivers/video/omap2/dss/dss.h
 +++ b/drivers/video/omap2/dss/dss.h
 @@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
  bool omapdss_hdmi_detect(void);
  int hdmi_panel_init(void);
  void hdmi_panel_exit(void);
 +#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
 +int hdmi_audio_enable(bool enable);
 +int hdmi_audio_start(bool start);
 +bool hdmi_mode_has_audio(void);
 +int hdmi_audio_config(struct snd_aes_iec958 *iec,
 + struct snd_cea_861_aud_if *aud_if);
 +#endif
  
  /* RFBI */
  #ifdef CONFIG_OMAP2_DSS_RFBI
 diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
 index bd44891..880509d 100644
 --- a/drivers/video/omap2/dss/hdmi.c
 +++ b/drivers/video/omap2/dss/hdmi.c
 @@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
  
   return 0;
  }
 +
 +int hdmi_audio_enable(bool enable)
 +{
 + DSSDBG(audio_enable\n);
 +
 + hdmi.ip_data.ops-audio_enable(hdmi.ip_data, enable);

Shouldn't this, and the others below, return the value from the called
function, instead of always returning 0?

 +
 + return 0;
 +}
 +
 +int hdmi_audio_start(bool start)
 +{
 + DSSDBG(audio_enable\n);
 +
 + hdmi.ip_data.ops-audio_start(hdmi.ip_data, start);
 +
 + return 0;
 +}
 +
 +bool hdmi_mode_has_audio(void)
 +{
 + if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
 + return true;
 + else
 + return false;
 +}
 +
 +int hdmi_audio_config(struct snd_aes_iec958 *iec,
 + struct snd_cea_861_aud_if *aud_if)
 +{
 + return hdmi.ip_data.ops-audio_config(hdmi.ip_data, iec, aud_if);
 +}
 +
  #endif
  
  /* HDMI HW IP initialisation */
 diff --git a/drivers/video/omap2/dss/hdmi_panel.c 
 b/drivers/video/omap2/dss/hdmi_panel.c
 index 533d5dc..dac1ac2 100644
 --- a/drivers/video/omap2/dss/hdmi_panel.c
 +++ b/drivers/video/omap2/dss/hdmi_panel.c
 @@ -31,6 +31,10 @@
  
  static struct {
   struct mutex hdmi_lock;
 +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 + /* protects calls to HDMI driver audio functionality */
 + spinlock_t hdmi_sp_lock;

What does sp stand for? Spinlock? Perhaps a better name would be
audio_lock, if it's audio specific. And probably no reason to prefix
it with hdmi, as it's inside hdmi struct already.

 Tomi



signature.asc
Description: This is a digitally signed message part

[PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

2012-03-28 Thread Ricardo Neri 
Implement the DSS device driver audio support interface in the HDMI
panel driver and generic driver. The implementation relies on the
IP-specific functions that are defined at DSS probe time.

A HW-safe spinlock is used to protect the audio functions. This is because
the audio functions may be called while holding a lock; in such case,
the panel's driver mutex is not suitable. Functions should be used
to set registers and should not wait for any other event.

Signed-off-by: Ricardo Neri ricardo.n...@ti.com
---
 drivers/video/omap2/dss/dss.h|7 +++
 drivers/video/omap2/dss/hdmi.c   |   33 +++
 drivers/video/omap2/dss/hdmi_panel.c |   76 ++
 3 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 32ff69f..fca4490 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
 bool omapdss_hdmi_detect(void);
 int hdmi_panel_init(void);
 void hdmi_panel_exit(void);
+#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
+int hdmi_audio_enable(bool enable);
+int hdmi_audio_start(bool start);
+bool hdmi_mode_has_audio(void);
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+   struct snd_cea_861_aud_if *aud_if);
+#endif
 
 /* RFBI */
 #ifdef CONFIG_OMAP2_DSS_RFBI
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index bd44891..880509d 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
 
return 0;
 }
+
+int hdmi_audio_enable(bool enable)
+{
+   DSSDBG(audio_enable\n);
+
+   hdmi.ip_data.ops-audio_enable(hdmi.ip_data, enable);
+
+   return 0;
+}
+
+int hdmi_audio_start(bool start)
+{
+   DSSDBG(audio_enable\n);
+
+   hdmi.ip_data.ops-audio_start(hdmi.ip_data, start);
+
+   return 0;
+}
+
+bool hdmi_mode_has_audio(void)
+{
+   if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
+   return true;
+   else
+   return false;
+}
+
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+   struct snd_cea_861_aud_if *aud_if)
+{
+   return hdmi.ip_data.ops-audio_config(hdmi.ip_data, iec, aud_if);
+}
+
 #endif
 
 /* HDMI HW IP initialisation */
diff --git a/drivers/video/omap2/dss/hdmi_panel.c 
b/drivers/video/omap2/dss/hdmi_panel.c
index 533d5dc..dac1ac2 100644
--- a/drivers/video/omap2/dss/hdmi_panel.c
+++ b/drivers/video/omap2/dss/hdmi_panel.c
@@ -31,6 +31,10 @@
 
 static struct {
struct mutex hdmi_lock;
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+   /* protects calls to HDMI driver audio functionality */
+   spinlock_t hdmi_sp_lock;
+#endif
 } hdmi;
 
 
@@ -222,6 +226,68 @@ err:
return r;
 }
 
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+static int hdmi_panel_audio_enable(struct omap_dss_device *dssdev, bool enable)
+{
+   unsigned long flags;
+   int r;
+
+   spin_lock_irqsave(hdmi.hdmi_sp_lock, flags);
+
+   r = hdmi_audio_enable(enable);
+
+   spin_unlock_irqrestore(hdmi.hdmi_sp_lock, flags);
+   return r;
+}
+
+static int hdmi_panel_audio_start(struct omap_dss_device *dssdev, bool start)
+{
+   unsigned long flags;
+   int r;
+
+   spin_lock_irqsave(hdmi.hdmi_sp_lock, flags);
+
+   r = hdmi_audio_start(start);
+
+   spin_unlock_irqrestore(hdmi.hdmi_sp_lock, flags);
+   return r;
+}
+
+static bool hdmi_panel_audio_detect(struct omap_dss_device *dssdev)
+{
+   unsigned long flags;
+   bool r = false;
+
+   spin_lock_irqsave(hdmi.hdmi_sp_lock, flags);
+
+   if (dssdev-state != OMAP_DSS_DISPLAY_ACTIVE)
+   goto err;
+
+   if (!hdmi_mode_has_audio())
+   goto err;
+
+   r = true;
+err:
+   spin_unlock_irqrestore(hdmi.hdmi_sp_lock, flags);
+   return r;
+}
+
+static int hdmi_panel_audio_config(struct omap_dss_device *dssdev,
+   struct snd_aes_iec958 *iec, struct snd_cea_861_aud_if *aud_if)
+{
+   unsigned long flags;
+   int r;
+
+   spin_lock_irqsave(hdmi.hdmi_sp_lock, flags);
+
+   r = hdmi_audio_config(iec, aud_if);
+
+   spin_unlock_irqrestore(hdmi.hdmi_sp_lock, flags);
+   return r;
+}
+
+#endif
+
 static struct omap_dss_driver hdmi_driver = {
.probe  = hdmi_panel_probe,
.remove = hdmi_panel_remove,
@@ -234,6 +300,12 @@ static struct omap_dss_driver hdmi_driver = {
.check_timings  = hdmi_check_timings,
.read_edid  = hdmi_read_edid,
.detect = hdmi_detect,
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+   .audio_enable   = hdmi_panel_audio_enable,
+   .audio_start= hdmi_panel_audio_start,
+   .audio_detect   = hdmi_panel_audio_detect,
+   .audio_config   = hdmi_panel_audio_config,
+#endif
.driver = {
.name   = hdmi_panel,