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? > 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 =). 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. 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 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. Tomi
Attachment:
signature.asc
Description: This is a digitally signed message part
