[PATCH 0/3] A few cursor fixups after the multi-size patches

[PATCH 0/3] A few cursor fixups after the multi-size patches

[Damien Lespiau]

Noticed that, if I'm not mistaken, we were misunderstanding the intention of
mode_config.cursor_{width,height}, and those 3 patches are the resulting fixes.

Damien Lespiau (3):
  drm/i915: Don't set mode_config's cursor size
  drm/i915: Remove max_cursor_{width,height} from the crtc
  drm/i915: Use the actual cursor width for WM computation

 drivers/gpu/drm/i915/intel_display.c | 10 ----------
 drivers/gpu/drm/i915/intel_drv.h     |  7 -------
 drivers/gpu/drm/i915/intel_pm.c      |  2 +-
 3 files changed, 1 insertion(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

Those fields are supposed to be a good default value for the cursor
size, intended for the case where the hardware doesn't support 64x64
cursors, for use with a hw agnostic DDX driver for instance.

We're fine with 64x64 cursors though and don't need to set those fields
(DRM core will return 64 is we don't). If we declare 256x256, that
generic driver will use a big buffer for the cursor, without any good
reason.

Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 80dd1c2..7a47657 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10557,8 +10557,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->max_cursor_width = CURSOR_WIDTH;
 		intel_crtc->max_cursor_height = CURSOR_HEIGHT;
 	}
-	dev->mode_config.cursor_width = intel_crtc->max_cursor_width;
-	dev->mode_config.cursor_height = intel_crtc->max_cursor_height;
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.3.1

[PATCH 2/3] drm/i915: Remove max_cursor_{width, height} from the crtc

[Damien Lespiau]

We don't need them anymore as they were used to initialize DRM's
mode_config.

As a side note, it was a bit strange to put them on the CRTC object,
they are global values, not specific to a CRTC.

As another side note, those values were only used in that one function,
so we didn't need to store them elsewhere.

Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 drivers/gpu/drm/i915/intel_drv.h     | 7 -------
 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7a47657..3d1ecd0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10550,14 +10550,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
 
-	if (IS_GEN2(dev)) {
-		intel_crtc->max_cursor_width = GEN2_CURSOR_WIDTH;
-		intel_crtc->max_cursor_height = GEN2_CURSOR_HEIGHT;
-	} else {
-		intel_crtc->max_cursor_width = CURSOR_WIDTH;
-		intel_crtc->max_cursor_height = CURSOR_HEIGHT;
-	}
-
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
 		intel_crtc->lut_r[i] = i;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fa99104..60ffad3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -78,12 +78,6 @@
 #define MAX_OUTPUTS 6
 /* maximum connectors per crtcs in the mode set */
 
-/* Maximum cursor sizes */
-#define GEN2_CURSOR_WIDTH 64
-#define GEN2_CURSOR_HEIGHT 64
-#define CURSOR_WIDTH 256
-#define CURSOR_HEIGHT 256
-
 #define INTEL_I2C_BUS_DVO 1
 #define INTEL_I2C_BUS_SDVO 2
 
@@ -373,7 +367,6 @@ struct intel_crtc {
 	uint32_t cursor_addr;
 	int16_t cursor_x, cursor_y;
 	int16_t cursor_width, cursor_height;
-	int16_t max_cursor_width, max_cursor_height;
 	bool cursor_visible;
 
 	struct intel_plane_config plane_config;
-- 
1.8.3.1

[PATCH 3/3] drm/i915: Use the actual cursor width for WM computation

[Damien Lespiau]

Now that we can use different cursor sizes, we can't hardcode 64 pixels
as the cursor width anymore.

Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index faa059a..0ed2a7b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2120,7 +2120,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 		p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
 		p->cur.bytes_per_pixel = 4;
 		p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
-		p->cur.horiz_pixels = 64;
+		p->cur.horiz_pixels = intel_crtc->cursor_width;
 		/* TODO: for now, assume primary and cursor planes are always enabled. */
 		p->pri.enabled = true;
 		p->cur.enabled = true;
-- 
1.8.3.1

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Chris Wilson]

On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> Those fields are supposed to be a good default value for the cursor
> size, intended for the case where the hardware doesn't support 64x64
> cursors, for use with a hw agnostic DDX driver for instance.
> 
> We're fine with 64x64 cursors though and don't need to set those fields
> (DRM core will return 64 is we don't). If we declare 256x256, that
> generic driver will use a big buffer for the cursor, without any good
> reason.

How does userspace now know that 256x256 cursors are support for HiDPI?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

On Tue, Mar 25, 2014 at 02:54:56PM +0000, Chris Wilson wrote:
> On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> > Those fields are supposed to be a good default value for the cursor
> > size, intended for the case where the hardware doesn't support 64x64
> > cursors, for use with a hw agnostic DDX driver for instance.
> > 
> > We're fine with 64x64 cursors though and don't need to set those fields
> > (DRM core will return 64 is we don't). If we declare 256x256, that
> > generic driver will use a big buffer for the cursor, without any good
> > reason.
> 
> How does userspace now know that 256x256 cursors are support for HiDPI?

It doesn't currently? a property on the cursor plane with the list of
supported formats in the brave new full drm_plane world seems like a
good option to me.

-- 
Damien

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Chris Wilson]

On Tue, Mar 25, 2014 at 02:59:18PM +0000, Damien Lespiau wrote:
> On Tue, Mar 25, 2014 at 02:54:56PM +0000, Chris Wilson wrote:
> > On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> > > Those fields are supposed to be a good default value for the cursor
> > > size, intended for the case where the hardware doesn't support 64x64
> > > cursors, for use with a hw agnostic DDX driver for instance.
> > > 
> > > We're fine with 64x64 cursors though and don't need to set those fields
> > > (DRM core will return 64 is we don't). If we declare 256x256, that
> > > generic driver will use a big buffer for the cursor, without any good
> > > reason.
> > 
> > How does userspace now know that 256x256 cursors are support for HiDPI?
> 
> It doesn't currently? a property on the cursor plane with the list of
> supported formats in the brave new full drm_plane world seems like a
> good option to me.

Userspace currently uses this method to determine the largest cursor
supported by the driver. That userspace is inept at resize the cursor bo
as required is a probably that won't be solved by simply exposing it
later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1490 bytes --]

On Tue, 2014-03-25 at 14:49 +0000, Damien Lespiau wrote:
> Those fields are supposed to be a good default value for the cursor
> size, intended for the case where the hardware doesn't support 64x64
> cursors, for use with a hw agnostic DDX driver for instance.
> 
> We're fine with 64x64 cursors though and don't need to set those fields
> (DRM core will return 64 is we don't). If we declare 256x256, that
> generic driver will use a big buffer for the cursor, without any good
> reason.
> 
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 80dd1c2..7a47657 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10557,8 +10557,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		intel_crtc->max_cursor_width = CURSOR_WIDTH;
>  		intel_crtc->max_cursor_height = CURSOR_HEIGHT;
>  	}
> -	dev->mode_config.cursor_width = intel_crtc->max_cursor_width;
> -	dev->mode_config.cursor_height = intel_crtc->max_cursor_height;

I thought drm_getcap should return the max cursor size, using these two
fields.

--Imre

>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Sagar Arun Kamble]

On Tue, 2014-03-25 at 14:59 +0000, Damien Lespiau wrote:
> On Tue, Mar 25, 2014 at 02:54:56PM +0000, Chris Wilson wrote:
> > On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> > > Those fields are supposed to be a good default value for the cursor
> > > size, intended for the case where the hardware doesn't support 64x64
> > > cursors, for use with a hw agnostic DDX driver for instance.
> > > 
> > > We're fine with 64x64 cursors though and don't need to set those fields
> > > (DRM core will return 64 is we don't). If we declare 256x256, that
> > > generic driver will use a big buffer for the cursor, without any good
> > > reason.
> > 
> > How does userspace now know that 256x256 cursors are support for HiDPI?
> 
> It doesn't currently? a property on the cursor plane with the list of
> supported formats in the brave new full drm_plane world seems like a
> good option to me.
How do we handle cursor size cap that was added with
http://lists.freedesktop.org/archives/dri-devel/2014-February/053770.html.
Will change i-g-t kms_cursor_crc accordingly as well since it depends on
this cap.
> 

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

On Tue, Mar 25, 2014 at 03:09:16PM +0000, Chris Wilson wrote:
> On Tue, Mar 25, 2014 at 02:59:18PM +0000, Damien Lespiau wrote:
> > On Tue, Mar 25, 2014 at 02:54:56PM +0000, Chris Wilson wrote:
> > > On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> > > > Those fields are supposed to be a good default value for the cursor
> > > > size, intended for the case where the hardware doesn't support 64x64
> > > > cursors, for use with a hw agnostic DDX driver for instance.
> > > > 
> > > > We're fine with 64x64 cursors though and don't need to set those fields
> > > > (DRM core will return 64 is we don't). If we declare 256x256, that
> > > > generic driver will use a big buffer for the cursor, without any good
> > > > reason.
> > > 
> > > How does userspace now know that 256x256 cursors are support for HiDPI?
> > 
> > It doesn't currently? a property on the cursor plane with the list of
> > supported formats in the brave new full drm_plane world seems like a
> > good option to me.
> 
> Userspace currently uses this method to determine the largest cursor
> supported by the driver. That userspace is inept at resize the cursor bo
> as required is a probably that won't be solved by simply exposing it
> later.

That doesn't seem to be the intention of the original patch?

http://lists.freedesktop.org/archives/dri-devel/2014-February/053770.html

Are you saying the Intel DDX currently derives a different meaning to
the intented behaviour? in which case it can still be changed to not do
that?

-- 
Damien

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Chris Wilson]

On Tue, Mar 25, 2014 at 04:05:29PM +0000, Damien Lespiau wrote:
> On Tue, Mar 25, 2014 at 03:09:16PM +0000, Chris Wilson wrote:
> > On Tue, Mar 25, 2014 at 02:59:18PM +0000, Damien Lespiau wrote:
> > > On Tue, Mar 25, 2014 at 02:54:56PM +0000, Chris Wilson wrote:
> > > > On Tue, Mar 25, 2014 at 02:49:30PM +0000, Damien Lespiau wrote:
> > > > > Those fields are supposed to be a good default value for the cursor
> > > > > size, intended for the case where the hardware doesn't support 64x64
> > > > > cursors, for use with a hw agnostic DDX driver for instance.
> > > > > 
> > > > > We're fine with 64x64 cursors though and don't need to set those fields
> > > > > (DRM core will return 64 is we don't). If we declare 256x256, that
> > > > > generic driver will use a big buffer for the cursor, without any good
> > > > > reason.
> > > > 
> > > > How does userspace now know that 256x256 cursors are support for HiDPI?
> > > 
> > > It doesn't currently? a property on the cursor plane with the list of
> > > supported formats in the brave new full drm_plane world seems like a
> > > good option to me.
> > 
> > Userspace currently uses this method to determine the largest cursor
> > supported by the driver. That userspace is inept at resize the cursor bo
> > as required is a probably that won't be solved by simply exposing it
> > later.
> 
> That doesn't seem to be the intention of the original patch?
> 
> http://lists.freedesktop.org/archives/dri-devel/2014-February/053770.html

For the record,

16:30 < agd5f> ickle, our GPUs don't have selectable cursor sizes
16:31 < agd5f> so on the newer ones, xf86-video-modesetting, etc. would
allocate a 64x64 cursor and it would look squashed and funky since the
hw expects 128x128

Which means I was confused when I thought part of the reasoning was
indeed HiDPI support. (I'm still seem to remember that was part of the
argument for large cursors anyway.)

> Are you saying the Intel DDX currently derives a different meaning to
> the intented behaviour? in which case it can still be changed to not do
> that?

I still disagree though. This provides all the information I need to
support variable sized cursors and we can use large cursors today.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

On Tue, Mar 25, 2014 at 04:38:24PM +0000, Chris Wilson wrote:
> For the record,
> 
> 16:30 < agd5f> ickle, our GPUs don't have selectable cursor sizes
> 16:31 < agd5f> so on the newer ones, xf86-video-modesetting, etc. would
> allocate a 64x64 cursor and it would look squashed and funky since the
> hw expects 128x128
> 
> Which means I was confused when I thought part of the reasoning was
> indeed HiDPI support. (I'm still seem to remember that was part of the
> argument for large cursors anyway.)
> 
> > Are you saying the Intel DDX currently derives a different meaning to
> > the intented behaviour? in which case it can still be changed to not do
> > that?
> 
> I still disagree though. This provides all the information I need to
> support variable sized cursors and we can use large cursors today.

I'd love the game to be about defining clear semantics more than "by
interpreting that value this way, I got what I always wanted" :)

We can resolve that today with MAX_CURSOR_WIDTH, MAX_CURSOR_HEIGHT caps
as well (if you're alluding at the fact that drm_planes may still be a
few decades away).

We'll still need to expose the full list of supported cursor sizes for
compositors at some point or another, my preferred way would be with a
property in the exposed cursor drm_plane.

-- 
Damien

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

On Tue, Mar 25, 2014 at 04:38:24PM +0000, Chris Wilson wrote:
> > Are you saying the Intel DDX currently derives a different meaning to
> > the intented behaviour? in which case it can still be changed to not do
> > that?
> 
> I still disagree though. This provides all the information I need to
> support variable sized cursors and we can use large cursors today.

Note that I won't fight if you find it useful and people are fine with
that new meaning. Can we just throw a patch actually documented what you
want those values to be?

The WM patch still needs to take the actual cursor size though. Keeping
those global limits on the crtc struct still looks weird.

-- 
Damien

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Daniel Vetter]

On Tue, Mar 25, 2014 at 04:57:04PM +0000, Damien Lespiau wrote:
> On Tue, Mar 25, 2014 at 04:38:24PM +0000, Chris Wilson wrote:
> > For the record,
> > 
> > 16:30 < agd5f> ickle, our GPUs don't have selectable cursor sizes
> > 16:31 < agd5f> so on the newer ones, xf86-video-modesetting, etc. would
> > allocate a 64x64 cursor and it would look squashed and funky since the
> > hw expects 128x128
> > 
> > Which means I was confused when I thought part of the reasoning was
> > indeed HiDPI support. (I'm still seem to remember that was part of the
> > argument for large cursors anyway.)
> > 
> > > Are you saying the Intel DDX currently derives a different meaning to
> > > the intented behaviour? in which case it can still be changed to not do
> > > that?
> > 
> > I still disagree though. This provides all the information I need to
> > support variable sized cursors and we can use large cursors today.
> 
> I'd love the game to be about defining clear semantics more than "by
> interpreting that value this way, I got what I always wanted" :)
> 
> We can resolve that today with MAX_CURSOR_WIDTH, MAX_CURSOR_HEIGHT caps
> as well (if you're alluding at the fact that drm_planes may still be a
> few decades away).
> 
> We'll still need to expose the full list of supported cursor sizes for
> compositors at some point or another, my preferred way would be with a
> property in the exposed cursor drm_plane.

For the record I'll restate my comment here about the larger problem:

Atm we have no way for userspace to figure out per-plane limits on
sizes/strides, we only expose a lists of supported pixel formats.

Imo exposing cursor limits is just part of this larger issue, so if we can
get the current stuff working with some legalese, then I'm ok with that.
Solving the larger issue is much more work, and it's better to have a few
more odd cases working than not. For planes I guess we could have a few
limits:

min/max width height in pixels
min/max stride
alignment of stride

And a pile of flags
- needs pot stride/width/height
- needs square size

That should be enough to cover most single-plane things. For multiplanar I
guess we might either just need 2nd/3rd set of those or some additional
stuff expressed in flags (e.g. subsampled strides must be half of the Y
stride). Or we'll throw our hands in the air for multi-planar for now ;-)

Or we simply do this per-pixel format with one for each framebuffer plane,
i.e.

struct drm_get_plane_fb_limits {
	uint32_t plane_id; /* in */
	uint32_t fourcc; /* in */
	struct drm_plane_limits limits[MAX_FOURCC_PLANES];
	/* the stuff above for all possible planes of a fourcc code */
}

Saner drivers could then return the same thing for all fourccs codes in
their backend.

Just something to ponder.

Adding dri-devel, maybe we can get this discussion started.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Daniel Vetter]

On Tue, Mar 25, 2014 at 07:23:26PM +0100, Daniel Vetter wrote:
> On Tue, Mar 25, 2014 at 04:57:04PM +0000, Damien Lespiau wrote:
> > On Tue, Mar 25, 2014 at 04:38:24PM +0000, Chris Wilson wrote:
> > > For the record,
> > > 
> > > 16:30 < agd5f> ickle, our GPUs don't have selectable cursor sizes
> > > 16:31 < agd5f> so on the newer ones, xf86-video-modesetting, etc. would
> > > allocate a 64x64 cursor and it would look squashed and funky since the
> > > hw expects 128x128
> > > 
> > > Which means I was confused when I thought part of the reasoning was
> > > indeed HiDPI support. (I'm still seem to remember that was part of the
> > > argument for large cursors anyway.)
> > > 
> > > > Are you saying the Intel DDX currently derives a different meaning to
> > > > the intented behaviour? in which case it can still be changed to not do
> > > > that?
> > > 
> > > I still disagree though. This provides all the information I need to
> > > support variable sized cursors and we can use large cursors today.
> > 
> > I'd love the game to be about defining clear semantics more than "by
> > interpreting that value this way, I got what I always wanted" :)
> > 
> > We can resolve that today with MAX_CURSOR_WIDTH, MAX_CURSOR_HEIGHT caps
> > as well (if you're alluding at the fact that drm_planes may still be a
> > few decades away).
> > 
> > We'll still need to expose the full list of supported cursor sizes for
> > compositors at some point or another, my preferred way would be with a
> > property in the exposed cursor drm_plane.
> 
> For the record I'll restate my comment here about the larger problem:
> 
> Atm we have no way for userspace to figure out per-plane limits on
> sizes/strides, we only expose a lists of supported pixel formats.
> 
> Imo exposing cursor limits is just part of this larger issue, so if we can
> get the current stuff working with some legalese, then I'm ok with that.
> Solving the larger issue is much more work, and it's better to have a few
> more odd cases working than not. For planes I guess we could have a few
> limits:
> 
> min/max width height in pixels
> min/max stride
> alignment of stride
> 
> And a pile of flags
> - needs pot stride/width/height
> - needs square size

Already new ones:
- stride must match width*bpp

This will be fun if we ever do it ...
-Daniel

> 
> That should be enough to cover most single-plane things. For multiplanar I
> guess we might either just need 2nd/3rd set of those or some additional
> stuff expressed in flags (e.g. subsampled strides must be half of the Y
> stride). Or we'll throw our hands in the air for multi-planar for now ;-)
> 
> Or we simply do this per-pixel format with one for each framebuffer plane,
> i.e.
> 
> struct drm_get_plane_fb_limits {
> 	uint32_t plane_id; /* in */
> 	uint32_t fourcc; /* in */
> 	struct drm_plane_limits limits[MAX_FOURCC_PLANES];
> 	/* the stuff above for all possible planes of a fourcc code */
> }
> 
> Saner drivers could then return the same thing for all fourccs codes in
> their backend.
> 
> Just something to ponder.
> 
> Adding dri-devel, maybe we can get this discussion started.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Damien Lespiau]

On Tue, Mar 25, 2014 at 07:23:26PM +0100, Daniel Vetter wrote:
> Or we simply do this per-pixel format with one for each framebuffer plane,
> i.e.
> 
> struct drm_get_plane_fb_limits {
> 	uint32_t plane_id; /* in */
> 	uint32_t fourcc; /* in */
> 	struct drm_plane_limits limits[MAX_FOURCC_PLANES];
> 	/* the stuff above for all possible planes of a fourcc code */
> }
> 
> Saner drivers could then return the same thing for all fourccs codes in
> their backend.

Some of the limits are definitely per format. Plane max dimensions are a
good example of a limit that can change per-format (8bpp Vs 10bpp to be
contained within the same max bandwidth of the hw).

-- 
Damien

Re: [PATCH 1/3] drm/i915: Don't set mode_config's cursor size

[Daniel Vetter]

On Tue, Mar 25, 2014 at 7:51 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Tue, Mar 25, 2014 at 07:23:26PM +0100, Daniel Vetter wrote:
>> Or we simply do this per-pixel format with one for each framebuffer plane,
>> i.e.
>>
>> struct drm_get_plane_fb_limits {
>>       uint32_t plane_id; /* in */
>>       uint32_t fourcc; /* in */
>>       struct drm_plane_limits limits[MAX_FOURCC_PLANES];
>>       /* the stuff above for all possible planes of a fourcc code */
>> }
>>
>> Saner drivers could then return the same thing for all fourccs codes in
>> their backend.
>
> Some of the limits are definitely per format. Plane max dimensions are a
> good example of a limit that can change per-format (8bpp Vs 10bpp to be
> contained within the same max bandwidth of the hw).

One thing I've completely missed btw is scaling limits. How we then
need to factor in bandwidth I have no idea about ... I guess at one
point it boils down to "try it and give up if it doesn't work". And I
think we need a few scaling related flags like "can't scale at all" or
sub-sampling restrictions. Who knows ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: Use the actual cursor width for WM computation

[Chris Wilson]

On Tue, Mar 25, 2014 at 02:49:32PM +0000, Damien Lespiau wrote:
> Now that we can use different cursor sizes, we can't hardcode 64 pixels
> as the cursor width anymore.

We also need fixes for i965 and g4x (normal and srwm). Not sure about
vlv, it has a hardcoded 64 for computing the cursor wm, but I don't know
if that is actually the cursor width.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Use the actual cursor width for WM computation

[Chris Wilson]

On Tue, Mar 25, 2014 at 02:49:32PM +0000, Damien Lespiau wrote:
> Now that we can use different cursor sizes, we can't hardcode 64 pixels
> as the cursor width anymore.

Also missing is recomputing the WM when the cursor size changes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915: Compute WM for current cursor size

[Chris Wilson]

Now that we can use different cursor size, we can not hardcode 64 pixels
as the cursor width anymore.

v2: Apply to 965gm/g4x paths as well

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6872895fb42..22134558c452 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1136,7 +1136,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	/* Use the large buffer method to calculate cursor watermark */
 	line_time_us = max(htotal * 1000 / clock, 1);
 	line_count = (cursor_latency_ns / line_time_us + 1000) / 1000;
-	entries = line_count * 64 * pixel_size;
+	entries = line_count * to_intel_crtc(crtc)->cursor_width * pixel_size;
 	tlb_miss = cursor->fifo_size*cursor->cacheline_size - hdisplay * 8;
 	if (tlb_miss > 0)
 		entries += tlb_miss;
@@ -1222,7 +1222,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 	*display_wm = entries + display->guard_size;
 
 	/* calculate the self-refresh watermark for display cursor */
-	entries = line_count * pixel_size * 64;
+	entries = line_count * pixel_size * to_intel_crtc(crtc)->cursor_width;
 	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
 	*cursor_wm = entries + cursor->guard_size;
 
@@ -1457,7 +1457,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 			      entries, srwm);
 
 		entries = (((sr_latency_ns / line_time_us) + 1000) / 1000) *
-			pixel_size * 64;
+			pixel_size * to_intel_crtc(crtc)->cursor_width;
 		entries = DIV_ROUND_UP(entries,
 					  i965_cursor_wm_info.cacheline_size);
 		cursor_sr = i965_cursor_wm_info.fifo_size -
@@ -2120,7 +2120,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 		p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
 		p->cur.bytes_per_pixel = 4;
 		p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
-		p->cur.horiz_pixels = 64;
+		p->cur.horiz_pixels = intel_crtc->cursor_width;
 		/* TODO: for now, assume primary and cursor planes are always enabled. */
 		p->pri.enabled = true;
 		p->cur.enabled = true;
-- 
1.9.1

[PATCH 2/2] drm/i915: Recompute WM when the cursor size changes

[Chris Wilson]

If the cursor width is changed, we may need to recompute our WM to
prevent untold flickering. We hope that the registers are flushed on the
same vblank to prevent underruns...

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c8c52d105b8c..e15b3abe1350 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7763,6 +7763,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_i915_gem_object *obj;
+	unsigned old_width;
 	uint32_t addr;
 	int ret;
 
@@ -7853,13 +7854,18 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 	mutex_unlock(&dev->struct_mutex);
 
+	old_width = intel_crtc->cursor_width;
+
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 	intel_crtc->cursor_width = width;
 	intel_crtc->cursor_height = height;
 
-	if (intel_crtc->active)
+	if (intel_crtc->active) {
+		if (old_width != width)
+			intel_update_watermarks(crtc);
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
+	}
 
 	return 0;
 fail_unpin:
-- 
1.9.1

Re: [PATCH 2/2] drm/i915: Recompute WM when the cursor size changes

[Daniel Vetter]

On Wed, Mar 26, 2014 at 12:38:15PM +0000, Chris Wilson wrote:
> If the cursor width is changed, we may need to recompute our WM to
> prevent untold flickering. We hope that the registers are flushed on the
> same vblank to prevent underruns...
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c8c52d105b8c..e15b3abe1350 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7763,6 +7763,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_gem_object *obj;
> +	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
> @@ -7853,13 +7854,18 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	old_width = intel_crtc->cursor_width;
> +
>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = obj;
>  	intel_crtc->cursor_width = width;
>  	intel_crtc->cursor_height = height;
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->active) {
> +		if (old_width != width)
> +			intel_update_watermarks(crtc);

Shouldn't we recompute when shrinking after and when growing before we
change the cursor size? vblank-syncing maybe even. Given all the amotic
foo we currently have I expect that cursors are as underrun prone as all
the other stuff is, too.

But can be done in a later patch, together with the nasty igt which walks
the cursor all over the place while changing the size ;-) And I guess
Ville gets to pimp his atomic update code and wm 2-stage update for this
stuff here, too.

I get a feeling this isn't quite as simple as Sagar's first patch
suggested ...
-Daniel
>  		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> +	}
>  
>  	return 0;
>  fail_unpin:
> -- 
> 1.9.1
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915: Recompute WM when the cursor size changes

[Damien Lespiau]

On Wed, Mar 26, 2014 at 12:38:15PM +0000, Chris Wilson wrote:
> If the cursor width is changed, we may need to recompute our WM to
> prevent untold flickering. We hope that the registers are flushed on the
> same vblank to prevent underruns...
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Indeed, I'll learn to see the bigger picture one day, one day...

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c8c52d105b8c..e15b3abe1350 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7763,6 +7763,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_gem_object *obj;
> +	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
> @@ -7853,13 +7854,18 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	old_width = intel_crtc->cursor_width;
> +
>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = obj;
>  	intel_crtc->cursor_width = width;
>  	intel_crtc->cursor_height = height;
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->active) {
> +		if (old_width != width)
> +			intel_update_watermarks(crtc);
>  		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> +	}
>  
>  	return 0;
>  fail_unpin:
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] drm/i915: Compute WM for current cursor size

[Damien Lespiau]

On Wed, Mar 26, 2014 at 12:38:14PM +0000, Chris Wilson wrote:
> Now that we can use different cursor size, we can not hardcode 64 pixels
> as the cursor width anymore.
> 
> v2: Apply to 965gm/g4x paths as well
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b6872895fb42..22134558c452 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1136,7 +1136,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>  	/* Use the large buffer method to calculate cursor watermark */
>  	line_time_us = max(htotal * 1000 / clock, 1);
>  	line_count = (cursor_latency_ns / line_time_us + 1000) / 1000;
> -	entries = line_count * 64 * pixel_size;
> +	entries = line_count * to_intel_crtc(crtc)->cursor_width * pixel_size;
>  	tlb_miss = cursor->fifo_size*cursor->cacheline_size - hdisplay * 8;
>  	if (tlb_miss > 0)
>  		entries += tlb_miss;
> @@ -1222,7 +1222,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
>  	*display_wm = entries + display->guard_size;
>  
>  	/* calculate the self-refresh watermark for display cursor */
> -	entries = line_count * pixel_size * 64;
> +	entries = line_count * pixel_size * to_intel_crtc(crtc)->cursor_width;
>  	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
>  	*cursor_wm = entries + cursor->guard_size;
>  
> @@ -1457,7 +1457,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>  			      entries, srwm);
>  
>  		entries = (((sr_latency_ns / line_time_us) + 1000) / 1000) *
> -			pixel_size * 64;
> +			pixel_size * to_intel_crtc(crtc)->cursor_width;
>  		entries = DIV_ROUND_UP(entries,
>  					  i965_cursor_wm_info.cacheline_size);
>  		cursor_sr = i965_cursor_wm_info.fifo_size -
> @@ -2120,7 +2120,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>  		p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
>  		p->cur.bytes_per_pixel = 4;
>  		p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
> -		p->cur.horiz_pixels = 64;
> +		p->cur.horiz_pixels = intel_crtc->cursor_width;
>  		/* TODO: for now, assume primary and cursor planes are always enabled. */
>  		p->pri.enabled = true;
>  		p->cur.enabled = true;
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] drm/i915: Recompute WM when the cursor size changes

[Daniel Vetter]

On Wed, Mar 26, 2014 at 03:21:03PM +0000, Damien Lespiau wrote:
> On Wed, Mar 26, 2014 at 12:38:15PM +0000, Chris Wilson wrote:
> > If the cursor width is changed, we may need to recompute our WM to
> > prevent untold flickering. We hope that the registers are flushed on the
> > same vblank to prevent underruns...
> > 
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Indeed, I'll learn to see the bigger picture one day, one day...
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Both merged, thanks for patches&review.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c8c52d105b8c..e15b3abe1350 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7763,6 +7763,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_i915_gem_object *obj;
> > +	unsigned old_width;
> >  	uint32_t addr;
> >  	int ret;
> >  
> > @@ -7853,13 +7854,18 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> >  
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> > +	old_width = intel_crtc->cursor_width;
> > +
> >  	intel_crtc->cursor_addr = addr;
> >  	intel_crtc->cursor_bo = obj;
> >  	intel_crtc->cursor_width = width;
> >  	intel_crtc->cursor_height = height;
> >  
> > -	if (intel_crtc->active)
> > +	if (intel_crtc->active) {
> > +		if (old_width != width)
> > +			intel_update_watermarks(crtc);
> >  		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> > +	}
> >  
> >  	return 0;
> >  fail_unpin:
> > -- 
> > 1.9.1
> > 
> _______________________________________________
> 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