Re: [Intel-gfx] [PATCH 12/12] drm/i915/fbc: switch to intel_de_* register accessors in display code
On Thu, 24 Nov 2022, Ville Syrjälä wrote: > On Wed, Nov 23, 2022 at 11:18:25PM +0200, Jani Nikula wrote: >> Avoid direct uncore use in display code. Use the new >> intel_de_rewrite_fw(). >> >> Cc: Maarten Lankhorst >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/display/intel_fbc.c | 10 ++ >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c >> b/drivers/gpu/drm/i915/display/intel_fbc.c >> index b5ee5ea0d010..6066ac412e6f 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> @@ -323,10 +323,7 @@ static void i8xx_fbc_nuke(struct intel_fbc *fbc) >> enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; >> struct drm_i915_private *dev_priv = fbc->i915; >> >> -spin_lock_irq(_priv->uncore.lock); >> -intel_de_write_fw(dev_priv, DSPADDR(i9xx_plane), >> - intel_de_read_fw(dev_priv, DSPADDR(i9xx_plane))); >> -spin_unlock_irq(_priv->uncore.lock); >> +intel_de_rewrite_fw(dev_priv, DSPADDR(i9xx_plane)); > > intel_de_rewrite_fw() seems to imply some kind of atomicicity guarantee > here. But that entirely depends on whether the other writers of this > register also protect it with unore.lock. So just a misleading illusion. > > That said, this locking stuff shouldn't even be needed since > commit de5bd083d247 ("drm/i915/fbc: Skip nuke when flip is pending") > commit 7cfd1a18c5f9 ("drm/i915: Remove remaining locks from i9xx plane > udpates") So instead just drop the whole intel_de_rewrite_fw() thing and the locking around the rewrite here? BR, Jani. > >> } >> >> static void i8xx_fbc_program_cfb(struct intel_fbc *fbc) >> @@ -359,10 +356,7 @@ static void i965_fbc_nuke(struct intel_fbc *fbc) >> enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; >> struct drm_i915_private *dev_priv = fbc->i915; >> >> -spin_lock_irq(_priv->uncore.lock); >> -intel_de_write_fw(dev_priv, DSPSURF(i9xx_plane), >> - intel_de_read_fw(dev_priv, DSPSURF(i9xx_plane))); >> -spin_unlock_irq(_priv->uncore.lock); >> +intel_de_rewrite_fw(dev_priv, DSPSURF(i9xx_plane)); >> } >> >> static const struct intel_fbc_funcs i965_fbc_funcs = { >> -- >> 2.34.1 -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 12/12] drm/i915/fbc: switch to intel_de_* register accessors in display code
On Wed, Nov 23, 2022 at 11:18:25PM +0200, Jani Nikula wrote: > Avoid direct uncore use in display code. Use the new > intel_de_rewrite_fw(). > > Cc: Maarten Lankhorst > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 10 ++ > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > b/drivers/gpu/drm/i915/display/intel_fbc.c > index b5ee5ea0d010..6066ac412e6f 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -323,10 +323,7 @@ static void i8xx_fbc_nuke(struct intel_fbc *fbc) > enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; > struct drm_i915_private *dev_priv = fbc->i915; > > - spin_lock_irq(_priv->uncore.lock); > - intel_de_write_fw(dev_priv, DSPADDR(i9xx_plane), > - intel_de_read_fw(dev_priv, DSPADDR(i9xx_plane))); > - spin_unlock_irq(_priv->uncore.lock); > + intel_de_rewrite_fw(dev_priv, DSPADDR(i9xx_plane)); intel_de_rewrite_fw() seems to imply some kind of atomicicity guarantee here. But that entirely depends on whether the other writers of this register also protect it with unore.lock. So just a misleading illusion. That said, this locking stuff shouldn't even be needed since commit de5bd083d247 ("drm/i915/fbc: Skip nuke when flip is pending") commit 7cfd1a18c5f9 ("drm/i915: Remove remaining locks from i9xx plane udpates") > } > > static void i8xx_fbc_program_cfb(struct intel_fbc *fbc) > @@ -359,10 +356,7 @@ static void i965_fbc_nuke(struct intel_fbc *fbc) > enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; > struct drm_i915_private *dev_priv = fbc->i915; > > - spin_lock_irq(_priv->uncore.lock); > - intel_de_write_fw(dev_priv, DSPSURF(i9xx_plane), > - intel_de_read_fw(dev_priv, DSPSURF(i9xx_plane))); > - spin_unlock_irq(_priv->uncore.lock); > + intel_de_rewrite_fw(dev_priv, DSPSURF(i9xx_plane)); > } > > static const struct intel_fbc_funcs i965_fbc_funcs = { > -- > 2.34.1 -- Ville Syrjälä Intel
[Intel-gfx] [PATCH 12/12] drm/i915/fbc: switch to intel_de_* register accessors in display code
Avoid direct uncore use in display code. Use the new intel_de_rewrite_fw(). Cc: Maarten Lankhorst Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/display/intel_fbc.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index b5ee5ea0d010..6066ac412e6f 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -323,10 +323,7 @@ static void i8xx_fbc_nuke(struct intel_fbc *fbc) enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; struct drm_i915_private *dev_priv = fbc->i915; - spin_lock_irq(_priv->uncore.lock); - intel_de_write_fw(dev_priv, DSPADDR(i9xx_plane), - intel_de_read_fw(dev_priv, DSPADDR(i9xx_plane))); - spin_unlock_irq(_priv->uncore.lock); + intel_de_rewrite_fw(dev_priv, DSPADDR(i9xx_plane)); } static void i8xx_fbc_program_cfb(struct intel_fbc *fbc) @@ -359,10 +356,7 @@ static void i965_fbc_nuke(struct intel_fbc *fbc) enum i9xx_plane_id i9xx_plane = fbc_state->plane->i9xx_plane; struct drm_i915_private *dev_priv = fbc->i915; - spin_lock_irq(_priv->uncore.lock); - intel_de_write_fw(dev_priv, DSPSURF(i9xx_plane), - intel_de_read_fw(dev_priv, DSPSURF(i9xx_plane))); - spin_unlock_irq(_priv->uncore.lock); + intel_de_rewrite_fw(dev_priv, DSPSURF(i9xx_plane)); } static const struct intel_fbc_funcs i965_fbc_funcs = { -- 2.34.1