Re: [PATCH 41/65] OMAPDSS: APPLY: add missing uses of spinlock

[Archit Taneja <a0393947@ti.com>]

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.
>
> 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.
>
> apply.c already contains a spinlock, which has been used to protect
> (badly) the dss_data. This patch adds locks/unlocks of the spinlock to
> the missing places, and the lock should now properly protect dss_data.
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/apply.c |   29 +++++++++++++++++++++++++++++
>   1 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index fb6d3c2..9ad2a36 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -405,6 +405,9 @@ void dss_start_update(struct omap_overlay_manager *mgr)
>   	struct mgr_priv_data *mp = get_mgr_priv(mgr);
>   	struct ovl_priv_data *op;
>   	struct omap_overlay *ovl;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data_lock, flags);
>
>   	mp->do_manual_update = true;
>   	dss_write_regs();
> @@ -418,6 +421,8 @@ void dss_start_update(struct omap_overlay_manager *mgr)
>   	mp->shadow_dirty = false;
>
>   	dispc_mgr_enable(mgr->id, true);
> +
> +	spin_unlock_irqrestore(&data_lock, flags);
>   }
>
>   static void dss_apply_irq_handler(void *data, u32 mask);
> @@ -662,16 +667,28 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr)
>   int dss_mgr_set_info(struct omap_overlay_manager *mgr,
>   		struct omap_overlay_manager_info *info)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data_lock, flags);
> +
>   	mgr->info = *info;
>   	mgr->info_dirty = true;
>
> +	spin_unlock_irqrestore(&data_lock, flags);
> +
>   	return 0;
>   }
>
>   void dss_mgr_get_info(struct omap_overlay_manager *mgr,
>   		struct omap_overlay_manager_info *info)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data_lock, flags);
> +
>   	*info = mgr->info;
> +
> +	spin_unlock_irqrestore(&data_lock, flags);
>   }
>
>   int dss_mgr_set_device(struct omap_overlay_manager *mgr,
> @@ -745,16 +762,28 @@ err:
>   int dss_ovl_set_info(struct omap_overlay *ovl,
>   		struct omap_overlay_info *info)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data_lock, flags);
> +
>   	ovl->info = *info;
>   	ovl->info_dirty = true;
>
> +	spin_unlock_irqrestore(&data_lock, flags);
> +
>   	return 0;
>   }
>
>   void dss_ovl_get_info(struct omap_overlay *ovl,
>   		struct omap_overlay_info *info)
>   {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data_lock, flags);
> +
>   	*info = ovl->info;
> +
> +	spin_unlock_irqrestore(&data_lock, flags);
>   }

The get/set info functions for overlays and managers only modify the 
omap_overlay_info or manager_info structs, these aren't really a part of 
'dss_data', they only become a part of dss_data only when we call 
mgr->apply().

So, are we protecting these functions so that 2 users of the same 
overlay don't see incorrect info values?

Archit

>
>   int dss_ovl_set_manager(struct omap_overlay *ovl,