Re: [PATCH 40/65] OMAPDSS: APPLY: add mutex

Tomi Valkeinen
2011-11-23T10:17:10Z

[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]

On Wed, 2011-11-23 at 15:18 +0530, Archit Taneja wrote:
> Hi,
>
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > The functions in apply.c, called mostly via function pointers in overlay
> > and overlay_manager structs, will be divided into two groups. The other
> > group will not sleep and can be called from interrupts, and the other
> > group may sleep.
>
> Small sentence issue above, both groups are called the 'other group'.

Thanks, fixed.

> >
> > The idea is that the non-sleeping functions may only change certain
> > settings in overlays and managers, and those settings may only affect
> > the particular overlay/manager. For example, set the base address of the
> > overlay.
> >
> > The blocking functions, however, will handle more complex configuration
> > changes. For example, when an overlay is enabled and fifo-merge feature
> > is used, we need to do the enable in multiple steps, waiting in between,
> > and the change affects multiple overlays and managers.
> >
> > This patch adds the mutex which is used in the blocking functions to
> > have exclusive access to overlays and overlay managers.
>
> Previously, when we changed the links between 'overlay->managers' and
> 'manager->devices', it wasn't protected by a lock. Why is it needed now?

Previously many places were missing a lock =).

> As an example, suppose we are changing a manager's device to some other
> display. Is this lock preventing someone else to get the older
> 'mgr->device' rather than the new one?

Hmm. We need some lock there, that's for sure, as set/unset manager are
changing the manager's list of overlays. However, it is also protected
by the spinlock, so in that sense mutex is not necessary.

I have to say I'm not sure if mutex is needed at this point. However,
consider the end result when fifo-merge is used:

dss_ovl_enable() will take the mutex, then it does the configuration in
multiple steps, doing multiple spin_lock & spin_unlocks, waiting in
between.

If we had only a spinlock in set/unset_manager, the manager could be
changed while dss_ovl_enable is doing the process of enabling the
overlay.

So I may have added mutexes or spinlocks a bit early in the series to
some places, but I don't see any harm in that. It'd be rather difficult
to try to find the exact spots where a lock becomes a requirement.

Tomi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]