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

[Ricardo Neri <ricardo.neri@ti.com>]

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