On 05/09/2012 03:28 AM, Tomi Valkeinen wrote:
On Tue, 2012-05-08 at 18:55 -0500, Ricardo Neri wrote:
Hi Tomi,
Thanks for taking the time to comment.
On 05/07/2012 06:43 AM, Tomi Valkeinen wrote:
Hi,
On Thu, 2012-05-03 at 20:44 -0500, 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.
At the moment, a hardirq-safe spinlock is used to protect the audio
functions. This is because such functions might be called while
holding a lock (this especially true for audio_start/stop). For the
rest of the audio functions, a mutex could be used in the future as
the enablement of resources might take too much time.
The series looks good, except locking. Granted, the locking in omapdss
is a bit bad generally also, but here I think it's a bit more broken.
For example, hdmi_panel.c:hdmi_panel_audio_supported() takes the audio
lock, and then uses variables like dssdev->state, and the hdmi video
mode. However, the video functions do not use audio lock, so effectively
the lock doesn't protect at all.
Yes, it does not protect.
I'm not sure how to fix it, though. I think this shows the shortcomings
of the current locking strategy (or lack of =). What if the audio
Btw, I meant shortcomings in the general DSS locking strategy, not the
locking in this particular patch.
functions that can sleep would take the hdmi panel's mutex, and also the
audio spinlock? That would at least fix some of the cases.
But if the function can sleep, protecting it with the HDMI panel's mutex
should be enough, shouldn't it? Wouldn't it be pointless to also hold
the spinlock?
If the start/stop functions use the spinlock, but not the mutex, then
the sleeping functions should also use the spinlock to prevent touching
the same data at the same time.
Ideally, I think, only one lock, the HDMI panel's mutex, should be
enough to protect the HDMI panel's functions, including the audio
functions. Reusing the HDMI panel's mutex for the audio functions would
prevent the situation you describe regarding
hdmi_panel.c:hdmi_panel_audio_supported()... and the spinlock would not
be required.
The only functions that cause problems with this approach are
audio_start/stop as ALSA calls them while holding a spinlock. The
spinlock could be used for these as they dont read or write the panel's
variables.
Locks always protect a particular piece of data. What is the data in
this case, if not panel's variables? DSS registers?
Yes, in this case, it protects access to HDMI IP registers.
I just implemented an improved locking strategy. I included a new
variable to monitor the audio state, this will be protected by the audio
lock.
This is the new implementation:
http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg68409.html
http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg68411.html
However, holding a spinlock only when start/stopping audio would fail,
for instance, if someone starts/stops audio while enabling or
configuring audio; but that would be an issue in the design of the
component using DSS HDMI audio, wouldn't be? To prevent that, an
I guess it's up to us to decide what is the supposed use-patter of the
functions =). For dss functions in general it's pretty vague and
non-defined. But we could start here with audio functions and define
that the audio functions may not be called from multiple threads at the
same time. That would remove any issues with concurrent calls to audio
functions, presuming the audio side actually conforms to this =).
Under the new strategy, in addition to not allowing the audio functions
to be called from multiple threads, audio functions will fail if the
sequence _CONFIGURED -> _ENABLED -> PLAYING -> DISABLED is not followed.
This is aligned with the behavior that ALSA follows for the audio
codecs. Also, it checks the state of the panel to allow the audio
transitions.
But the video and audio paths are probably always separate, and for
those we need protection. As you said, using the mutex for the may-sleep
audio functions solves the issue for those, leaving start/stop as the
only problem case.
Audio only needs to know if the display is active. Under the improved
strategy, audio_start indirectly checks the state of the panel because
the audio needs to be in AUDIO_ENABLED state to start and this state is
reached only if the panel is active. The mutex is held to check the
state of the panel and the audio lock is held to change the audio state.
Also, the audio transitions to AUDIO_DISABLED if the panel is disabled.
Therefore, I think, this should ensure that the panel is active when
starting audio.
However, even if we could protect start/stop with locks, we still have a
problem (which is general problem related to dss locking): we don't have
any protection between the function calls. So basically this could
happen:
[video thread] setup video& enable
[audio thread] check that all is ok, and configure audio
[video thread] change video config or disable video
[audio thread] start_audio -> fails, because video config no longer
valid for audio
Under the improved locking strategy, at least it does not fail silently.
The video thread will disable audio (while holding the audio lock) and,
when the audio thread gains the lock, audio_start will fail because it
is in an invalid state. The error is reported to the audio user where it
can be handled properly(e.g., it can reconfigure and try again).
But I guess we have to accept that the locking is not perfect, and try
to solve it properly later, as it's a bigger, dss-wide change.
Another piece that is missing is the notify mechanism to let know the
audio user that the panel was disabled or the video configuration changed.
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
