diff mbox

drm/i915: Don't try to use SPR_SCALE when we don't have a sprite scaler

Message ID 1350926367-2386-1-git-send-email-damien.lespiau@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Damien Lespiau Oct. 22, 2012, 5:19 p.m. UTC 
From: Damien Lespiau <damien.lespiau@intel.com>

Haswell does not have a scaler in the sprite pipeline anymore, so let's
ensure:
  1/ We bail out of update_plate() when someone is trying to ask to
     display a scaled framebuffer,
  2/ We never write to the nonexistent SPR_SCALE register

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_sprite.c | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Jesse Barnes Oct. 23, 2012, 2:16 p.m. UTC | #1 
On Mon, 22 Oct 2012 18:19:27 +0100
Damien Lespiau <damien.lespiau@gmail.com> wrote:

> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> Haswell does not have a scaler in the sprite pipeline anymore, so let's
> ensure:
>   1/ We bail out of update_plate() when someone is trying to ask to
>      display a scaled framebuffer,
>   2/ We never write to the nonexistent SPR_SCALE register
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0942fb4..97c714d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -221,6 +221,7 @@ struct intel_plane {
>  	struct drm_plane base;
>  	enum pipe pipe;
>  	struct drm_i915_gem_object *obj;
> +	bool can_scale;
>  	int max_downscale;
>  	u32 lut_r[1024], lut_g[1024], lut_b[1024];
>  	void (*update_plane)(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index f2ca943..ccfaa6f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -136,7 +136,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
>  		I915_WRITE(SPRLINOFF(pipe), offset);
>  	}
>  	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> -	I915_WRITE(SPRSCALE(pipe), sprscale);
> +	if (intel_plane->can_scale)
> +		I915_WRITE(SPRSCALE(pipe), sprscale);
>  	I915_WRITE(SPRCTL(pipe), sprctl);
>  	I915_MODIFY_DISPBASE(SPRSURF(pipe), obj->gtt_offset);
>  	POSTING_READ(SPRSURF(pipe));
> @@ -474,6 +475,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		goto out;
>  
>  	/*
> +	 * We may not have a scaler, eg. HSW does not have it any more
> +	 */
> +	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
> +		return -EINVAL;
> +
> +	/*
>  	 * We can take a larger source and scale it down, but
>  	 * only so much...  16x is the max on SNB.
>  	 */
> @@ -666,6 +673,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
>  	switch (INTEL_INFO(dev)->gen) {
>  	case 5:
>  	case 6:
> +		intel_plane->can_scale = true;
>  		intel_plane->max_downscale = 16;
>  		intel_plane->update_plane = ilk_update_plane;
>  		intel_plane->disable_plane = ilk_disable_plane;
> @@ -682,6 +690,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
>  		break;
>  
>  	case 7:
> +		if (IS_HASWELL(dev))
> +			intel_plane->can_scale = false;
> +		else
> +			intel_plane->can_scale = true;
>  		intel_plane->max_downscale = 2;
>  		intel_plane->update_plane = ivb_update_plane;
>  		intel_plane->disable_plane = ivb_disable_plane;

Yeah, looks fine.

Thanks,
Daniel Vetter Oct. 23, 2012, 2:45 p.m. UTC | #2 
On Tue, Oct 23, 2012 at 07:16:03AM -0700, Jesse Barnes wrote:
> On Mon, 22 Oct 2012 18:19:27 +0100
> Damien Lespiau <damien.lespiau@gmail.com> wrote:
> 
> > From: Damien Lespiau <damien.lespiau@intel.com>
> > 
> > Haswell does not have a scaler in the sprite pipeline anymore, so let's
> > ensure:
> >   1/ We bail out of update_plate() when someone is trying to ask to
> >      display a scaled framebuffer,
> >   2/ We never write to the nonexistent SPR_SCALE register
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> >  drivers/gpu/drm/i915/intel_sprite.c | 14 +++++++++++++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0942fb4..97c714d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -221,6 +221,7 @@ struct intel_plane {
> >  	struct drm_plane base;
> >  	enum pipe pipe;
> >  	struct drm_i915_gem_object *obj;
> > +	bool can_scale;
> >  	int max_downscale;
> >  	u32 lut_r[1024], lut_g[1024], lut_b[1024];
> >  	void (*update_plane)(struct drm_plane *plane,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index f2ca943..ccfaa6f 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -136,7 +136,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
> >  		I915_WRITE(SPRLINOFF(pipe), offset);
> >  	}
> >  	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> > -	I915_WRITE(SPRSCALE(pipe), sprscale);
> > +	if (intel_plane->can_scale)
> > +		I915_WRITE(SPRSCALE(pipe), sprscale);
> >  	I915_WRITE(SPRCTL(pipe), sprctl);
> >  	I915_MODIFY_DISPBASE(SPRSURF(pipe), obj->gtt_offset);
> >  	POSTING_READ(SPRSURF(pipe));
> > @@ -474,6 +475,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		goto out;
> >  
> >  	/*
> > +	 * We may not have a scaler, eg. HSW does not have it any more
> > +	 */
> > +	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
> > +		return -EINVAL;
> > +
> > +	/*
> >  	 * We can take a larger source and scale it down, but
> >  	 * only so much...  16x is the max on SNB.
> >  	 */
> > @@ -666,6 +673,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
> >  	switch (INTEL_INFO(dev)->gen) {
> >  	case 5:
> >  	case 6:
> > +		intel_plane->can_scale = true;
> >  		intel_plane->max_downscale = 16;
> >  		intel_plane->update_plane = ilk_update_plane;
> >  		intel_plane->disable_plane = ilk_disable_plane;
> > @@ -682,6 +690,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
> >  		break;
> >  
> >  	case 7:
> > +		if (IS_HASWELL(dev))
> > +			intel_plane->can_scale = false;
> > +		else
> > +			intel_plane->can_scale = true;
> >  		intel_plane->max_downscale = 2;
> >  		intel_plane->update_plane = ivb_update_plane;
> >  		intel_plane->disable_plane = ivb_disable_plane;
> 
> Yeah, looks fine.

Queued for -next, thanks for the patch.
-Daniel
Paulo Zanoni Oct. 23, 2012, 3 p.m. UTC | #3 
Hi

2012/10/23 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Oct 23, 2012 at 07:16:03AM -0700, Jesse Barnes wrote:
>> On Mon, 22 Oct 2012 18:19:27 +0100
>> Damien Lespiau <damien.lespiau@gmail.com> wrote:
>>
>> > From: Damien Lespiau <damien.lespiau@intel.com>
>> >
>> > Haswell does not have a scaler in the sprite pipeline anymore, so let's
>> > ensure:
>> >   1/ We bail out of update_plate() when someone is trying to ask to
>> >      display a scaled framebuffer,
>> >   2/ We never write to the nonexistent SPR_SCALE register
>> >
>> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>> >  drivers/gpu/drm/i915/intel_sprite.c | 14 +++++++++++++-
>> >  2 files changed, 14 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 0942fb4..97c714d 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -221,6 +221,7 @@ struct intel_plane {
>> >     struct drm_plane base;
>> >     enum pipe pipe;
>> >     struct drm_i915_gem_object *obj;
>> > +   bool can_scale;
>> >     int max_downscale;
>> >     u32 lut_r[1024], lut_g[1024], lut_b[1024];
>> >     void (*update_plane)(struct drm_plane *plane,
>> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> > index f2ca943..ccfaa6f 100644
>> > --- a/drivers/gpu/drm/i915/intel_sprite.c
>> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> > @@ -136,7 +136,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
>> >             I915_WRITE(SPRLINOFF(pipe), offset);
>> >     }
>> >     I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
>> > -   I915_WRITE(SPRSCALE(pipe), sprscale);
>> > +   if (intel_plane->can_scale)
>> > +           I915_WRITE(SPRSCALE(pipe), sprscale);
>> >     I915_WRITE(SPRCTL(pipe), sprctl);
>> >     I915_MODIFY_DISPBASE(SPRSURF(pipe), obj->gtt_offset);
>> >     POSTING_READ(SPRSURF(pipe));
>> > @@ -474,6 +475,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>> >             goto out;
>> >
>> >     /*
>> > +    * We may not have a scaler, eg. HSW does not have it any more
>> > +    */
>> > +   if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
>> > +           return -EINVAL;
>> > +
>> > +   /*
>> >      * We can take a larger source and scale it down, but
>> >      * only so much...  16x is the max on SNB.
>> >      */
>> > @@ -666,6 +673,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
>> >     switch (INTEL_INFO(dev)->gen) {
>> >     case 5:
>> >     case 6:
>> > +           intel_plane->can_scale = true;
>> >             intel_plane->max_downscale = 16;
>> >             intel_plane->update_plane = ilk_update_plane;
>> >             intel_plane->disable_plane = ilk_disable_plane;
>> > @@ -682,6 +690,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe)
>> >             break;
>> >
>> >     case 7:
>> > +           if (IS_HASWELL(dev))
>> > +                   intel_plane->can_scale = false;
>> > +           else
>> > +                   intel_plane->can_scale = true;
>> >             intel_plane->max_downscale = 2;
>> >             intel_plane->update_plane = ivb_update_plane;
>> >             intel_plane->disable_plane = ivb_disable_plane;
>>
>> Yeah, looks fine.
>
> Queued for -next, thanks for the patch.

After testing this, I see we also need to patch ivb_disable_plane to
not write SPRSCALE. Everything else looks correct, so no problem to
include this in a separate patch.


> -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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0942fb4..97c714d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -221,6 +221,7 @@  struct intel_plane {
 	struct drm_plane base;
 	enum pipe pipe;
 	struct drm_i915_gem_object *obj;
+	bool can_scale;
 	int max_downscale;
 	u32 lut_r[1024], lut_g[1024], lut_b[1024];
 	void (*update_plane)(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f2ca943..ccfaa6f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -136,7 +136,8 @@  ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
 		I915_WRITE(SPRLINOFF(pipe), offset);
 	}
 	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
-	I915_WRITE(SPRSCALE(pipe), sprscale);
+	if (intel_plane->can_scale)
+		I915_WRITE(SPRSCALE(pipe), sprscale);
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_MODIFY_DISPBASE(SPRSURF(pipe), obj->gtt_offset);
 	POSTING_READ(SPRSURF(pipe));
@@ -474,6 +475,12 @@  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		goto out;
 
 	/*
+	 * We may not have a scaler, eg. HSW does not have it any more
+	 */
+	if (!intel_plane->can_scale && (crtc_w != src_w || crtc_h != src_h))
+		return -EINVAL;
+
+	/*
 	 * We can take a larger source and scale it down, but
 	 * only so much...  16x is the max on SNB.
 	 */
@@ -666,6 +673,7 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe)
 	switch (INTEL_INFO(dev)->gen) {
 	case 5:
 	case 6:
+		intel_plane->can_scale = true;
 		intel_plane->max_downscale = 16;
 		intel_plane->update_plane = ilk_update_plane;
 		intel_plane->disable_plane = ilk_disable_plane;
@@ -682,6 +690,10 @@  intel_plane_init(struct drm_device *dev, enum pipe pipe)
 		break;
 
 	case 7:
+		if (IS_HASWELL(dev))
+			intel_plane->can_scale = false;
+		else
+			intel_plane->can_scale = true;
 		intel_plane->max_downscale = 2;
 		intel_plane->update_plane = ivb_update_plane;
 		intel_plane->disable_plane = ivb_disable_plane;