Re: [PATCH 1/7] drm/omap: drop unneeded locking from mgr_fld_write()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Mon, Sep 02, 2019 at 03:53:53PM +0300, Tomi Valkeinen wrote:
> Commit d49cd15550d9d4495f6187425318c245d58cb63f ("OMAPDSS: DISPC: lock
> access to DISPC_CONTROL & DISPC_CONFIG") added locking to
> mgr_fld_write(). This was needed in omapfb times due to lack of good
> locking, especially in the case of both V4L2 and fbdev layers using the
> DSS driver.
> 
> This is not needed for omapdrm, so we can remove the locking.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I've followed all code paths, and they end up either in the bridge
enable operations or the CRTC atomic flush (disregarding
suspend/resume). Those can't race each other, so this should be safe.

> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 785c5546067a..c6da33e7014f 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -184,9 +184,6 @@ struct dispc_device {
>  
>  	struct regmap *syscon_pol;
>  	u32 syscon_pol_offset;
> -
> -	/* DISPC_CONTROL & DISPC_CONFIG lock*/
> -	spinlock_t control_lock;
>  };
>  
>  enum omap_color_component {
> @@ -377,16 +374,7 @@ static void mgr_fld_write(struct dispc_device *dispc, enum omap_channel channel,
>  			  enum mgr_reg_fields regfld, int val)
>  {
>  	const struct dispc_reg_field rfld = mgr_desc[channel].reg_desc[regfld];

While at it, should you turn this into a pointer to avoid an unnecessary
copy ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -	const bool need_lock = rfld.reg == DISPC_CONTROL || rfld.reg == DISPC_CONFIG;
> -	unsigned long flags;
> -
> -	if (need_lock) {
> -		spin_lock_irqsave(&dispc->control_lock, flags);
> -		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
> -		spin_unlock_irqrestore(&dispc->control_lock, flags);
> -	} else {
> -		REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
> -	}
> +	REG_FLD_MOD(dispc, rfld.reg, val, rfld.high, rfld.low);
>  }
>  
>  static int dispc_get_num_ovls(struct dispc_device *dispc)
> @@ -4769,8 +4757,6 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  	platform_set_drvdata(pdev, dispc);
>  	dispc->dss = dss;
>  
> -	spin_lock_init(&dispc->control_lock);
> -
>  	/*
>  	 * The OMAP3-based models can't be told apart using the compatible
>  	 * string, use SoC device matching.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel