diff mbox

[07/24] drm/i915: Remove useless checks from primary enable/disable

Message ID 1394209951-9963-8-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala March 7, 2014, 4:32 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We won't be calling intel_enable_primary_plane() or
intel_disable_primary_plane() with the primary plane in the
wrong state. So remove the useless DISPLAY_PLANE_ENABLE checks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Daniel Vetter March 7, 2014, 9:29 p.m. UTC | #1 
On Fri, Mar 07, 2014 at 06:32:14PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We won't be calling intel_enable_primary_plane() or
> intel_disable_primary_plane() with the primary plane in the
> wrong state. So remove the useless DISPLAY_PLANE_ENABLE checks.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Convert them to asserts, or are we already covered in this regard?

/me has an uncanny love for self-checks ...

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7938556..2815351 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1896,8 +1896,6 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
>  
>  	reg = DSPCNTR(plane);
>  	val = I915_READ(reg);
> -	if (val & DISPLAY_PLANE_ENABLE)
> -		return;
>  
>  	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
>  	intel_flush_primary_plane(dev_priv, plane);
> @@ -1926,8 +1924,6 @@ static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
>  
>  	reg = DSPCNTR(plane);
>  	val = I915_READ(reg);
> -	if ((val & DISPLAY_PLANE_ENABLE) == 0)
> -		return;
>  
>  	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
>  	intel_flush_primary_plane(dev_priv, plane);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala March 10, 2014, 11:20 a.m. UTC | #2 
On Fri, Mar 07, 2014 at 10:29:26PM +0100, Daniel Vetter wrote:
> On Fri, Mar 07, 2014 at 06:32:14PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We won't be calling intel_enable_primary_plane() or
> > intel_disable_primary_plane() with the primary plane in the
> > wrong state. So remove the useless DISPLAY_PLANE_ENABLE checks.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Convert them to asserts, or are we already covered in this regard?
> 
> /me has an uncanny love for self-checks ...

There's a WARN() for the crtc->primary_enabled flag in there, which
should cover things. Although we could add some sanity check that makes
sure the DISPLAY_PLANE_ENABLE bit matches crtc->primary_enabled.

> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7938556..2815351 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1896,8 +1896,6 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
> >  
> >  	reg = DSPCNTR(plane);
> >  	val = I915_READ(reg);
> > -	if (val & DISPLAY_PLANE_ENABLE)
> > -		return;
> >  
> >  	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> >  	intel_flush_primary_plane(dev_priv, plane);
> > @@ -1926,8 +1924,6 @@ static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
> >  
> >  	reg = DSPCNTR(plane);
> >  	val = I915_READ(reg);
> > -	if ((val & DISPLAY_PLANE_ENABLE) == 0)
> > -		return;
> >  
> >  	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
> >  	intel_flush_primary_plane(dev_priv, plane);
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter March 10, 2014, 11:57 a.m. UTC | #3 
On Mon, Mar 10, 2014 at 12:20 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 07, 2014 at 10:29:26PM +0100, Daniel Vetter wrote:
>> On Fri, Mar 07, 2014 at 06:32:14PM +0200, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We won't be calling intel_enable_primary_plane() or
>> > intel_disable_primary_plane() with the primary plane in the
>> > wrong state. So remove the useless DISPLAY_PLANE_ENABLE checks.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Convert them to asserts, or are we already covered in this regard?
>>
>> /me has an uncanny love for self-checks ...
>
> There's a WARN() for the crtc->primary_enabled flag in there, which
> should cover things. Although we could add some sanity check that makes
> sure the DISPLAY_PLANE_ENABLE bit matches crtc->primary_enabled.

Yeah, that's what I've thought of - we generally double-check hw state
before frobbing it.
-Daniel
Paulo Zanoni April 7, 2014, 8:04 p.m. UTC | #4 
2014-03-10 8:57 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Mar 10, 2014 at 12:20 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Mar 07, 2014 at 10:29:26PM +0100, Daniel Vetter wrote:
>>> On Fri, Mar 07, 2014 at 06:32:14PM +0200, ville.syrjala@linux.intel.com wrote:
>>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> >
>>> > We won't be calling intel_enable_primary_plane() or
>>> > intel_disable_primary_plane() with the primary plane in the
>>> > wrong state. So remove the useless DISPLAY_PLANE_ENABLE checks.
>>> >
>>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Convert them to asserts, or are we already covered in this regard?
>>>
>>> /me has an uncanny love for self-checks ...
>>
>> There's a WARN() for the crtc->primary_enabled flag in there, which
>> should cover things. Although we could add some sanity check that makes
>> sure the DISPLAY_PLANE_ENABLE bit matches crtc->primary_enabled.
>
> Yeah, that's what I've thought of - we generally double-check hw state
> before frobbing it.

I agree with Daniel here, let's convert it to a WARN.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7938556..2815351 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1896,8 +1896,6 @@  static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-	if (val & DISPLAY_PLANE_ENABLE)
-		return;
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);
@@ -1926,8 +1924,6 @@  static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-	if ((val & DISPLAY_PLANE_ENABLE) == 0)
-		return;
 
 	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
 	intel_flush_primary_plane(dev_priv, plane);