diff mbox series

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

Message ID 20190902125359.18001-2-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show
Series drm/omap: misc improvements | expand

Commit Message

Tomi Valkeinen Sept. 2, 2019, 12:53 p.m. UTC
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>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

Comments

Laurent Pinchart Sept. 3, 2019, 2:14 p.m. UTC | #1
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.
diff mbox series

Patch

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];
-	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.