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 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?
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.
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 audio_configured state variable in the HDMI panel could be used to monitor that audio is started only after it is enabled and configured. In that case, both the spinlock and the mutex would be required, as you propose, because audio_configured would be read inside audio_start/stop.
BR,
Ricardo
Tomi