[PATCH] drm/i915: Support variable cursor height on ivb+

[PATCH] drm/i915: Support variable cursor height on ivb+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

IVB introduced the CUR_FBC_CTL register which allows reducing the cursor
height down to 8 lines from the otherwise square cursor dimensions.
Implement support for it.

Commandeer the otherwise unused intel_crtc->cursor_size to track the
current value of CUR_FBC_CTL so that we can avoid duplicating the
complicated device type checks in i9xx_update_cursor().

One caveat to note is that CUR_FBC_CTL can't be used with 180 degree
rotation, so when cursor rotation gets introduced some extra checks are
needed.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 15c0eaa..572001b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4169,7 +4169,9 @@ enum punit_power_well {
 #define   CURSOR_POS_SIGN       0x8000
 #define   CURSOR_X_SHIFT        0
 #define   CURSOR_Y_SHIFT        16
-#define CURSIZE			0x700a0
+#define CURSIZE			0x700a0 /* 845/865 */
+#define _CUR_FBC_CTL_A		0x700a0 /* ivb+ */
+#define   CUR_FBC_CTL_EN	(1 << 31)
 #define _CURBCNTR		0x700c0
 #define _CURBBASE		0x700c4
 #define _CURBPOS		0x700c8
@@ -4185,6 +4187,7 @@ enum punit_power_well {
 #define CURCNTR(pipe) _CURSOR2(pipe, _CURACNTR)
 #define CURBASE(pipe) _CURSOR2(pipe, _CURABASE)
 #define CURPOS(pipe) _CURSOR2(pipe, _CURAPOS)
+#define CUR_FBC_CTL(pipe) _CURSOR2(pipe, _CUR_FBC_CTL_A)
 
 #define CURSOR_A_OFFSET 0x70080
 #define CURSOR_B_OFFSET 0x700c0
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca729e5..9c000fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8197,6 +8197,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl;
+	uint32_t fbc_ctl = 0;
 
 	cntl = 0;
 	if (base) {
@@ -8217,6 +8218,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		}
 		cntl |= pipe << 28; /* Connect to correct pipe */
 	}
+
+	if (intel_crtc->cursor_height != intel_crtc->cursor_width)
+		fbc_ctl = CUR_FBC_CTL_EN | (intel_crtc->cursor_height - 1);
+
+	if (intel_crtc->cursor_size != fbc_ctl) {
+		I915_WRITE(CUR_FBC_CTL(pipe), fbc_ctl);
+		intel_crtc->cursor_size = fbc_ctl;
+		intel_crtc->cursor_cntl = 0;
+	}
+
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		cntl |= CURSOR_PIPE_CSC_ENABLE;
 
@@ -8293,6 +8304,8 @@ static bool cursor_size_ok(struct drm_device *dev,
 	 * the width of their cursors, the height is arbitrary up to
 	 * the precision of the register. Everything else requires
 	 * square cursors, limited to a few power-of-two sizes.
+	 * ivb+ have CUR_FBC_CTL which allows reducing the cursor
+	 * height down to a minimum of 8 lines.
 	 */
 	if (IS_845G(dev) || IS_I865G(dev)) {
 		if ((width & 63) != 0)
@@ -8304,7 +8317,7 @@ static bool cursor_size_ok(struct drm_device *dev,
 		if (height > 1023)
 			return false;
 	} else {
-		switch (width | height) {
+		switch (width) {
 		case 256:
 		case 128:
 			if (IS_GEN2(dev))
@@ -8314,6 +8327,16 @@ static bool cursor_size_ok(struct drm_device *dev,
 		default:
 			return false;
 		}
+
+		if (IS_IVYBRIDGE(dev) ||
+		    IS_HASWELL(dev) || IS_BROADWELL(dev) ||
+		    INTEL_INFO(dev)->gen >= 9) {
+			if (height < 8 || height > width)
+				return false;
+		} else {
+			if (height != width)
+				return false;
+		}
 	}
 
 	return true;
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Support variable cursor height on ivb+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

IVB introduced the CUR_FBC_CTL register which allows reducing the cursor
height down to 8 lines from the otherwise square cursor dimensions.
Implement support for it.

Commandeer the otherwise unused intel_crtc->cursor_size to track the
current value of CUR_FBC_CTL so that we can avoid duplicating the
complicated device type checks in i9xx_update_cursor().

One caveat to note is that CUR_FBC_CTL can't be used with 180 degree
rotation, so when cursor rotation gets introduced some extra checks are
needed.

v2: Reverse the gen check to make it sane

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 15c0eaa..572001b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4169,7 +4169,9 @@ enum punit_power_well {
 #define   CURSOR_POS_SIGN       0x8000
 #define   CURSOR_X_SHIFT        0
 #define   CURSOR_Y_SHIFT        16
-#define CURSIZE			0x700a0
+#define CURSIZE			0x700a0 /* 845/865 */
+#define _CUR_FBC_CTL_A		0x700a0 /* ivb+ */
+#define   CUR_FBC_CTL_EN	(1 << 31)
 #define _CURBCNTR		0x700c0
 #define _CURBBASE		0x700c4
 #define _CURBPOS		0x700c8
@@ -4185,6 +4187,7 @@ enum punit_power_well {
 #define CURCNTR(pipe) _CURSOR2(pipe, _CURACNTR)
 #define CURBASE(pipe) _CURSOR2(pipe, _CURABASE)
 #define CURPOS(pipe) _CURSOR2(pipe, _CURAPOS)
+#define CUR_FBC_CTL(pipe) _CURSOR2(pipe, _CUR_FBC_CTL_A)
 
 #define CURSOR_A_OFFSET 0x70080
 #define CURSOR_B_OFFSET 0x700c0
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7809177..092aba3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8208,6 +8208,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl;
+	uint32_t fbc_ctl = 0;
 
 	cntl = 0;
 	if (base) {
@@ -8228,6 +8229,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		}
 		cntl |= pipe << 28; /* Connect to correct pipe */
 	}
+
+	if (intel_crtc->cursor_height != intel_crtc->cursor_width)
+		fbc_ctl = CUR_FBC_CTL_EN | (intel_crtc->cursor_height - 1);
+
+	if (intel_crtc->cursor_size != fbc_ctl) {
+		I915_WRITE(CUR_FBC_CTL(pipe), fbc_ctl);
+		intel_crtc->cursor_size = fbc_ctl;
+		intel_crtc->cursor_cntl = 0;
+	}
+
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		cntl |= CURSOR_PIPE_CSC_ENABLE;
 
@@ -8304,6 +8315,8 @@ static bool cursor_size_ok(struct drm_device *dev,
 	 * the width of their cursors, the height is arbitrary up to
 	 * the precision of the register. Everything else requires
 	 * square cursors, limited to a few power-of-two sizes.
+	 * ivb+ have CUR_FBC_CTL which allows reducing the cursor
+	 * height down to a minimum of 8 lines.
 	 */
 	if (IS_845G(dev) || IS_I865G(dev)) {
 		if ((width & 63) != 0)
@@ -8315,7 +8328,7 @@ static bool cursor_size_ok(struct drm_device *dev,
 		if (height > 1023)
 			return false;
 	} else {
-		switch (width | height) {
+		switch (width) {
 		case 256:
 		case 128:
 			if (IS_GEN2(dev))
@@ -8325,6 +8338,14 @@ static bool cursor_size_ok(struct drm_device *dev,
 		default:
 			return false;
 		}
+
+		if (INTEL_INFO(dev)->gen < 7 || IS_VALLEYVIEW(dev)) {
+			if (height != width)
+				return false;
+		} else {
+			if (height < 8 || height > width)
+				return false;
+		}
 	}
 
 	return true;
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/4] drm/i915: Move the cursor_base setup to i{845, 9xx}_update_cursor()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To make the code a bit more undestandable move the
intel_crtc->cursor_base assignment into the low level update cursor
routines. That's were we compare the current value with the new one
so immediately seeing that it gets assigned only afterwards helps
one to understand that it gets assigned only after the comparison.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41986cc..8a5cb6b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8274,8 +8274,10 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
 		intel_crtc->cursor_cntl = 0;
 	}
 
-	if (intel_crtc->cursor_base != base)
+	if (intel_crtc->cursor_base != base) {
 		I915_WRITE(_CURABASE, base);
+		intel_crtc->cursor_base = base;
+	}
 
 	if (intel_crtc->cursor_size != size) {
 		I915_WRITE(CURSIZE, size);
@@ -8328,6 +8330,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	/* and commit changes on next vblank */
 	I915_WRITE(CURBASE(pipe), base);
 	POSTING_READ(CURBASE(pipe));
+
+	intel_crtc->cursor_base = base;
 }
 
 /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
@@ -8378,7 +8382,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 		i845_update_cursor(crtc, base);
 	else
 		i9xx_update_cursor(crtc, base);
-	intel_crtc->cursor_base = base;
 }
 
 static bool cursor_size_ok(struct drm_device *dev,
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/4] drm/i915: Only set CURSOR_PIPE_CSC_ENABLE when cursor is enabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

It seems cleaner if we keep CURCNTR at 0 when the cursor is disabled,
so don't set the CURSOR_PIPE_CSC_ENABLE bit unless the cursor is
enabled.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8a5cb6b..82c0ad1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8317,9 +8317,10 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 				return;
 		}
 		cntl |= pipe << 28; /* Connect to correct pipe */
+
+		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+			cntl |= CURSOR_PIPE_CSC_ENABLE;
 	}
-	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		cntl |= CURSOR_PIPE_CSC_ENABLE;
 
 	if (intel_crtc->cursor_cntl != cntl) {
 		I915_WRITE(CURCNTR(pipe), cntl);
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/4] drm/i915: Only set CURSOR_PIPE_CSC_ENABLE when cursor is enabled

[Chris Wilson]

On Fri, Sep 12, 2014 at 08:53:33PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> It seems cleaner if we keep CURCNTR at 0 when the cursor is disabled,
> so don't set the CURSOR_PIPE_CSC_ENABLE bit unless the cursor is
> enabled.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 2/4] drm/i915: Only set CURSOR_PIPE_CSC_ENABLE when cursor is enabled

[Daniel Vetter]

On Sat, Sep 13, 2014 at 05:17:29PM +0100, Chris Wilson wrote:
> On Fri, Sep 12, 2014 at 08:53:33PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > It seems cleaner if we keep CURCNTR at 0 when the cursor is disabled,
> > so don't set the CURSOR_PIPE_CSC_ENABLE bit unless the cursor is
> > enabled.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged the first 2 patches from this series to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3 3/4] drm/i915: Skip CURBASE write when nothing changed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Only write CURBASE when something about the cursor changed. Also
eliminate the unnecessary posting read after writing CURCNTR.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 82c0ad1..60c1aa4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 			cntl |= CURSOR_PIPE_CSC_ENABLE;
 	}
 
-	if (intel_crtc->cursor_cntl != cntl) {
+	if (intel_crtc->cursor_cntl != cntl)
 		I915_WRITE(CURCNTR(pipe), cntl);
-		POSTING_READ(CURCNTR(pipe));
-		intel_crtc->cursor_cntl = cntl;
-	}
+
+	if (intel_crtc->cursor_cntl == cntl &&
+	    intel_crtc->cursor_base == base)
+		return;
 
 	/* and commit changes on next vblank */
 	I915_WRITE(CURBASE(pipe), base);
 	POSTING_READ(CURBASE(pipe));
 
+	intel_crtc->cursor_cntl = cntl;
 	intel_crtc->cursor_base = base;
 }
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/4] drm/i915: Skip CURBASE write when nothing changed

[Chris Wilson]

On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Only write CURBASE when something about the cursor changed. Also
> eliminate the unnecessary posting read after writing CURCNTR.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 82c0ad1..60c1aa4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  			cntl |= CURSOR_PIPE_CSC_ENABLE;
>  	}
>  
> -	if (intel_crtc->cursor_cntl != cntl) {
> +	if (intel_crtc->cursor_cntl != cntl)
>  		I915_WRITE(CURCNTR(pipe), cntl);
> -		POSTING_READ(CURCNTR(pipe));
> -		intel_crtc->cursor_cntl = cntl;
> -	}
> +
> +	if (intel_crtc->cursor_cntl == cntl &&
> +	    intel_crtc->cursor_base == base)
> +		return;

I'd vote for doing this first and then

I915_WRITE(CURCNTR(pipe), cntl);

unconditionally along with the CURBASE flush.

>  	/* and commit changes on next vblank */
>  	I915_WRITE(CURBASE(pipe), base);
>  	POSTING_READ(CURBASE(pipe));
>  
> +	intel_crtc->cursor_cntl = cntl;
>  	intel_crtc->cursor_base = base;
>  }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 3/4] drm/i915: Skip CURBASE write when nothing changed

[Ville Syrjälä]

On Sat, Sep 13, 2014 at 05:20:10PM +0100, Chris Wilson wrote:
> On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Only write CURBASE when something about the cursor changed. Also
> > eliminate the unnecessary posting read after writing CURCNTR.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 82c0ad1..60c1aa4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  			cntl |= CURSOR_PIPE_CSC_ENABLE;
> >  	}
> >  
> > -	if (intel_crtc->cursor_cntl != cntl) {
> > +	if (intel_crtc->cursor_cntl != cntl)
> >  		I915_WRITE(CURCNTR(pipe), cntl);
> > -		POSTING_READ(CURCNTR(pipe));
> > -		intel_crtc->cursor_cntl = cntl;
> > -	}
> > +
> > +	if (intel_crtc->cursor_cntl == cntl &&
> > +	    intel_crtc->cursor_base == base)
> > +		return;
> 
> I'd vote for doing this first and then
> 
> I915_WRITE(CURCNTR(pipe), cntl);
> 
> unconditionally along with the CURBASE flush.

So you want to write all the cursor registers unconditionally when
anything changes? Would work for CURCNTR, but it would also make
CUR_FBC_CTL stand out like a sore thumb since it can't be written
uncoditionally.

> 
> >  	/* and commit changes on next vblank */
> >  	I915_WRITE(CURBASE(pipe), base);
> >  	POSTING_READ(CURBASE(pipe));
> >  
> > +	intel_crtc->cursor_cntl = cntl;
> >  	intel_crtc->cursor_base = base;
> >  }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 3/4] drm/i915: Skip CURBASE write when nothing changed

[Chris Wilson]

On Mon, Sep 15, 2014 at 04:36:38PM +0300, Ville Syrjälä wrote:
> On Sat, Sep 13, 2014 at 05:20:10PM +0100, Chris Wilson wrote:
> > On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Only write CURBASE when something about the cursor changed. Also
> > > eliminate the unnecessary posting read after writing CURCNTR.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 82c0ad1..60c1aa4 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  			cntl |= CURSOR_PIPE_CSC_ENABLE;
> > >  	}
> > >  
> > > -	if (intel_crtc->cursor_cntl != cntl) {
> > > +	if (intel_crtc->cursor_cntl != cntl)
> > >  		I915_WRITE(CURCNTR(pipe), cntl);
> > > -		POSTING_READ(CURCNTR(pipe));
> > > -		intel_crtc->cursor_cntl = cntl;
> > > -	}
> > > +
> > > +	if (intel_crtc->cursor_cntl == cntl &&
> > > +	    intel_crtc->cursor_base == base)
> > > +		return;
> > 
> > I'd vote for doing this first and then
> > 
> > I915_WRITE(CURCNTR(pipe), cntl);
> > 
> > unconditionally along with the CURBASE flush.
> 
> So you want to write all the cursor registers unconditionally when
> anything changes? Would work for CURCNTR, but it would also make
> CUR_FBC_CTL stand out like a sore thumb since it can't be written
> uncoditionally.

Alternatively, something like

	bool dirty = intel_crtc->cursor_base != base;

	if (intel_crtc->cursor_cntl != cntl) {
		I915_WRITE(CURCNTR(pipe), cntl);
		dirty = true;
	}

	/* ... */

	if (!dirty)
		return;

	I915_WRITE(CURBASE(pipe), base);
	POSTING_READ(CURBASE(pipe));

	intel_crtc->cursor_cntl = cntl;
	intel_crtc->cursor_base = base;

I think would have clearer control flow.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 4/4] drm/i915: Support variable cursor height on ivb+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

IVB introduced the CUR_FBC_CTL register which allows reducing the cursor
height down to 8 lines from the otherwise square cursor dimensions.
Implement support for it.

Commandeer the otherwise unused intel_crtc->cursor_size to track the
current value of CUR_FBC_CTL so that we can avoid duplicating the
complicated device type checks in i9xx_update_cursor().

One caveat to note is that CUR_FBC_CTL can't be used with 180 degree
rotation, so when cursor rotation gets introduced some extra checks are
needed.

v2: Reverse the gen check to make it sane
v3: Only enable CUR_FBC_CTL when cursor is enabled, adapt to
    earlier code changes which means we now actually turn off
    the cursor when we're supposed to unlike v2

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ad8179b..4bb2c31 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4181,7 +4181,9 @@ enum punit_power_well {
 #define   CURSOR_POS_SIGN       0x8000
 #define   CURSOR_X_SHIFT        0
 #define   CURSOR_Y_SHIFT        16
-#define CURSIZE			0x700a0
+#define CURSIZE			0x700a0 /* 845/865 */
+#define _CUR_FBC_CTL_A		0x700a0 /* ivb+ */
+#define   CUR_FBC_CTL_EN	(1 << 31)
 #define _CURBCNTR		0x700c0
 #define _CURBBASE		0x700c4
 #define _CURBPOS		0x700c8
@@ -4197,6 +4199,7 @@ enum punit_power_well {
 #define CURCNTR(pipe) _CURSOR2(pipe, _CURACNTR)
 #define CURBASE(pipe) _CURSOR2(pipe, _CURABASE)
 #define CURPOS(pipe) _CURSOR2(pipe, _CURAPOS)
+#define CUR_FBC_CTL(pipe) _CURSOR2(pipe, _CUR_FBC_CTL_A)
 
 #define CURSOR_A_OFFSET 0x70080
 #define CURSOR_B_OFFSET 0x700c0
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60c1aa4..7c9c514 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8297,9 +8297,8 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	uint32_t cntl;
+	uint32_t cntl = 0, fbc_ctl = 0;
 
-	cntl = 0;
 	if (base) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (intel_crtc->cursor_width) {
@@ -8320,12 +8319,19 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 
 		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 			cntl |= CURSOR_PIPE_CSC_ENABLE;
+
+		if (intel_crtc->cursor_height != intel_crtc->cursor_width)
+			fbc_ctl = CUR_FBC_CTL_EN | (intel_crtc->cursor_height - 1);
 	}
 
+	if (intel_crtc->cursor_size != fbc_ctl)
+		I915_WRITE(CUR_FBC_CTL(pipe), fbc_ctl);
+
 	if (intel_crtc->cursor_cntl != cntl)
 		I915_WRITE(CURCNTR(pipe), cntl);
 
-	if (intel_crtc->cursor_cntl == cntl &&
+	if (intel_crtc->cursor_size == fbc_ctl &&
+	    intel_crtc->cursor_cntl == cntl &&
 	    intel_crtc->cursor_base == base)
 		return;
 
@@ -8333,6 +8339,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	I915_WRITE(CURBASE(pipe), base);
 	POSTING_READ(CURBASE(pipe));
 
+	intel_crtc->cursor_size = fbc_ctl;
 	intel_crtc->cursor_cntl = cntl;
 	intel_crtc->cursor_base = base;
 }
@@ -8398,6 +8405,8 @@ static bool cursor_size_ok(struct drm_device *dev,
 	 * the width of their cursors, the height is arbitrary up to
 	 * the precision of the register. Everything else requires
 	 * square cursors, limited to a few power-of-two sizes.
+	 * ivb+ have CUR_FBC_CTL which allows reducing the cursor
+	 * height down to a minimum of 8 lines.
 	 */
 	if (IS_845G(dev) || IS_I865G(dev)) {
 		if ((width & 63) != 0)
@@ -8409,7 +8418,7 @@ static bool cursor_size_ok(struct drm_device *dev,
 		if (height > 1023)
 			return false;
 	} else {
-		switch (width | height) {
+		switch (width) {
 		case 256:
 		case 128:
 			if (IS_GEN2(dev))
@@ -8419,6 +8428,14 @@ static bool cursor_size_ok(struct drm_device *dev,
 		default:
 			return false;
 		}
+
+		if (INTEL_INFO(dev)->gen < 7 || IS_VALLEYVIEW(dev)) {
+			if (height != width)
+				return false;
+		} else {
+			if (height < 8 || height > width)
+				return false;
+		}
 	}
 
 	return true;
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/4] drm/i915: Support variable cursor height on ivb+

[Chris Wilson]

On Fri, Sep 12, 2014 at 08:53:35PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> IVB introduced the CUR_FBC_CTL register which allows reducing the cursor
> height down to 8 lines from the otherwise square cursor dimensions.
> Implement support for it.
> 
> Commandeer the otherwise unused intel_crtc->cursor_size to track the
> current value of CUR_FBC_CTL so that we can avoid duplicating the
> complicated device type checks in i9xx_update_cursor().
> 
> One caveat to note is that CUR_FBC_CTL can't be used with 180 degree
> rotation, so when cursor rotation gets introduced some extra checks are
> needed.

Where would be a good place to put that note into a comment?

So other than the mystery of rotated cursors, the code looks clear
enough. One side question, is the CHV/VLV conflation correct here?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 4/4] drm/i915: Support variable cursor height on ivb+

[Ville Syrjälä]

On Sat, Sep 13, 2014 at 05:23:23PM +0100, Chris Wilson wrote:
> On Fri, Sep 12, 2014 at 08:53:35PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > IVB introduced the CUR_FBC_CTL register which allows reducing the cursor
> > height down to 8 lines from the otherwise square cursor dimensions.
> > Implement support for it.
> > 
> > Commandeer the otherwise unused intel_crtc->cursor_size to track the
> > current value of CUR_FBC_CTL so that we can avoid duplicating the
> > complicated device type checks in i9xx_update_cursor().
> > 
> > One caveat to note is that CUR_FBC_CTL can't be used with 180 degree
> > rotation, so when cursor rotation gets introduced some extra checks are
> > needed.
> 
> Where would be a good place to put that note into a comment?

I guess I could stick it into cursor_size_ok().

> 
> So other than the mystery of rotated cursors, the code looks clear
> enough. One side question, is the CHV/VLV conflation correct here?

Yes. It's still the same old g4x derived display controller.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 1/4] drm/i915: Move the cursor_base setup to i{845, 9xx}_update_cursor()

[Chris Wilson]

On Fri, Sep 12, 2014 at 08:53:32PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To make the code a bit more undestandable move the
> intel_crtc->cursor_base assignment into the low level update cursor
> routines. That's were we compare the current value with the new one
> so immediately seeing that it gets assigned only afterwards helps
> one to understand that it gets assigned only after the comparison.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'd been tempted to do this myself, but lacked justification.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre