| Message ID | 1394304546-19433-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 8, 2014 at 1:49 PM, <sagar.a.kamble@intel.com> wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > With this patch we allow larger cursor planes of sizes 128x128 > and 256x256. > > v2: Added more precise check on size while setting cursor plane. > > v3: Changes related to restructuring cursor size restrictions > and DRM_DEBUG usage. > I'm not sure how useful it is since you appear to be able to selectively adjust the cursor size, but I recently added support for exposing the cursor size as a drm cap: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8716ed4e7bed4e4c7e3f37940e950ddc0362f450 Alex > Testcase: igt/kms_cursor_crc > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: G, Pallavi <pallavi.g@intel.com> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++----- > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 146609a..aee8258 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3551,7 +3551,11 @@ enum punit_power_well { > /* New style CUR*CNTR flags */ > #define CURSOR_MODE 0x27 > #define CURSOR_MODE_DISABLE 0x00 > +#define CURSOR_MODE_128_32B_AX 0x02 > +#define CURSOR_MODE_256_32B_AX 0x03 > #define CURSOR_MODE_64_32B_AX 0x07 > +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) > +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) > #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) > #define MCURSOR_PIPE_SELECT (1 << 28) > #define MCURSOR_PIPE_A 0x00 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0868afb..e59e8fd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7440,10 +7440,22 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > bool visible = base != 0; > > if (intel_crtc->cursor_visible != visible) { > + int16_t width = intel_crtc->cursor_width; > uint32_t cntl = I915_READ(CURCNTR(pipe)); > if (base) { > cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + > + switch (width) { > + case 64: > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 128: > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 256: > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + } > cntl |= pipe << 28; /* Connect to correct pipe */ > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > @@ -7468,10 +7480,22 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) > bool visible = base != 0; > > if (intel_crtc->cursor_visible != visible) { > + int16_t width = intel_crtc->cursor_width; > uint32_t cntl = I915_READ(CURCNTR_IVB(pipe)); > if (base) { > cntl &= ~CURSOR_MODE; > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + > + switch (width) { > + case 64: > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 128: > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 256: > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + } > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > cntl |= CURSOR_MODE_DISABLE; > @@ -7567,9 +7591,11 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > goto finish; > } > > - /* Currently we only support 64x64 cursors */ > - if (width != 64 || height != 64) { > - DRM_ERROR("we currently only support 64x64 cursors\n"); > + /* Check for which cursor types we support */ > + if (!((width == 64 && height == 64) || > + (width == 128 && height == 128 && !IS_GEN2(dev)) || > + (width == 256 && height == 256 && !IS_GEN2(dev)))) { > + DRM_DEBUG("Cursor dimension not supported\n"); > return -EINVAL; > } > > -- > 1.8.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, 2014-03-08 at 13:51 -0500, Alex Deucher wrote: > On Sat, Mar 8, 2014 at 1:49 PM, <sagar.a.kamble@intel.com> wrote: > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > With this patch we allow larger cursor planes of sizes 128x128 > > and 256x256. > > > > v2: Added more precise check on size while setting cursor plane. > > > > v3: Changes related to restructuring cursor size restrictions > > and DRM_DEBUG usage. > > > > I'm not sure how useful it is since you appear to be able to > selectively adjust the cursor size, but I recently added support for > exposing the cursor size as a drm cap: > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8716ed4e7bed4e4c7e3f37940e950ddc0362f450 > > Alex Thanks Alex. This is useful for cursor test to enumerate sub-tests based on these caps. > > > Testcase: igt/kms_cursor_crc > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: G, Pallavi <pallavi.g@intel.com> > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++----- > > 2 files changed, 35 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 146609a..aee8258 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3551,7 +3551,11 @@ enum punit_power_well { > > /* New style CUR*CNTR flags */ > > #define CURSOR_MODE 0x27 > > #define CURSOR_MODE_DISABLE 0x00 > > +#define CURSOR_MODE_128_32B_AX 0x02 > > +#define CURSOR_MODE_256_32B_AX 0x03 > > #define CURSOR_MODE_64_32B_AX 0x07 > > +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) > > +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) > > #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) > > #define MCURSOR_PIPE_SELECT (1 << 28) > > #define MCURSOR_PIPE_A 0x00 > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 0868afb..e59e8fd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -7440,10 +7440,22 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > bool visible = base != 0; > > > > if (intel_crtc->cursor_visible != visible) { > > + int16_t width = intel_crtc->cursor_width; > > uint32_t cntl = I915_READ(CURCNTR(pipe)); > > if (base) { > > cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + > > + switch (width) { > > + case 64: > > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + case 128: > > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + case 256: > > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + } > > cntl |= pipe << 28; /* Connect to correct pipe */ > > } else { > > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > > @@ -7468,10 +7480,22 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) > > bool visible = base != 0; > > > > if (intel_crtc->cursor_visible != visible) { > > + int16_t width = intel_crtc->cursor_width; > > uint32_t cntl = I915_READ(CURCNTR_IVB(pipe)); > > if (base) { > > cntl &= ~CURSOR_MODE; > > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + > > + switch (width) { > > + case 64: > > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + case 128: > > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + case 256: > > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > + break; > > + } > > } else { > > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > > cntl |= CURSOR_MODE_DISABLE; > > @@ -7567,9 +7591,11 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > > goto finish; > > } > > > > - /* Currently we only support 64x64 cursors */ > > - if (width != 64 || height != 64) { > > - DRM_ERROR("we currently only support 64x64 cursors\n"); > > + /* Check for which cursor types we support */ > > + if (!((width == 64 && height == 64) || > > + (width == 128 && height == 128 && !IS_GEN2(dev)) || > > + (width == 256 && height == 256 && !IS_GEN2(dev)))) { > > + DRM_DEBUG("Cursor dimension not supported\n"); > > return -EINVAL; > > } > > > > -- > > 1.8.5 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, 2014-03-09 at 00:34 +0530, Sagar Arun Kamble wrote: > On Sat, 2014-03-08 at 13:51 -0500, Alex Deucher wrote: > > On Sat, Mar 8, 2014 at 1:49 PM, <sagar.a.kamble@intel.com> wrote: > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > With this patch we allow larger cursor planes of sizes 128x128 > > > and 256x256. > > > > > > v2: Added more precise check on size while setting cursor plane. > > > > > > v3: Changes related to restructuring cursor size restrictions > > > and DRM_DEBUG usage. > > > > > > > I'm not sure how useful it is since you appear to be able to > > selectively adjust the cursor size, but I recently added support for > > exposing the cursor size as a drm cap: > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8716ed4e7bed4e4c7e3f37940e950ddc0362f450 > > > > Alex > Thanks Alex. This is useful for cursor test to enumerate sub-tests based > on these caps. Realized after sending mail that we just get to know current cursor width and height. Can we have capability that exposes all supported cursor sizes? > > > > > Testcase: igt/kms_cursor_crc > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: linux-kernel@vger.kernel.org > > > Signed-off-by: G, Pallavi <pallavi.g@intel.com> > > > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++----- > > > 2 files changed, 35 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 146609a..aee8258 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -3551,7 +3551,11 @@ enum punit_power_well { > > > /* New style CUR*CNTR flags */ > > > #define CURSOR_MODE 0x27 > > > #define CURSOR_MODE_DISABLE 0x00 > > > +#define CURSOR_MODE_128_32B_AX 0x02 > > > +#define CURSOR_MODE_256_32B_AX 0x03 > > > #define CURSOR_MODE_64_32B_AX 0x07 > > > +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) > > > +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) > > > #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) > > > #define MCURSOR_PIPE_SELECT (1 << 28) > > > #define MCURSOR_PIPE_A 0x00 > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 0868afb..e59e8fd 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -7440,10 +7440,22 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > bool visible = base != 0; > > > > > > if (intel_crtc->cursor_visible != visible) { > > > + int16_t width = intel_crtc->cursor_width; > > > uint32_t cntl = I915_READ(CURCNTR(pipe)); > > > if (base) { > > > cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > > > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + > > > + switch (width) { > > > + case 64: > > > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + case 128: > > > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + case 256: > > > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + } > > > cntl |= pipe << 28; /* Connect to correct pipe */ > > > } else { > > > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > > > @@ -7468,10 +7480,22 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) > > > bool visible = base != 0; > > > > > > if (intel_crtc->cursor_visible != visible) { > > > + int16_t width = intel_crtc->cursor_width; > > > uint32_t cntl = I915_READ(CURCNTR_IVB(pipe)); > > > if (base) { > > > cntl &= ~CURSOR_MODE; > > > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + > > > + switch (width) { > > > + case 64: > > > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + case 128: > > > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + case 256: > > > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > > > + break; > > > + } > > > } else { > > > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > > > cntl |= CURSOR_MODE_DISABLE; > > > @@ -7567,9 +7591,11 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > > > goto finish; > > > } > > > > > > - /* Currently we only support 64x64 cursors */ > > > - if (width != 64 || height != 64) { > > > - DRM_ERROR("we currently only support 64x64 cursors\n"); > > > + /* Check for which cursor types we support */ > > > + if (!((width == 64 && height == 64) || > > > + (width == 128 && height == 128 && !IS_GEN2(dev)) || > > > + (width == 256 && height == 256 && !IS_GEN2(dev)))) { > > > + DRM_DEBUG("Cursor dimension not supported\n"); > > > return -EINVAL; > > > } > > > > > > -- > > > 1.8.5 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Sun, Mar 09, 2014 at 12:37:49AM +0530, Sagar Arun Kamble wrote: > On Sun, 2014-03-09 at 00:34 +0530, Sagar Arun Kamble wrote: > > On Sat, 2014-03-08 at 13:51 -0500, Alex Deucher wrote: > > > On Sat, Mar 8, 2014 at 1:49 PM, <sagar.a.kamble@intel.com> wrote: > > > > From: Sagar Kamble <sagar.a.kamble@intel.com> > > > > > > > > With this patch we allow larger cursor planes of sizes 128x128 > > > > and 256x256. > > > > > > > > v2: Added more precise check on size while setting cursor plane. > > > > > > > > v3: Changes related to restructuring cursor size restrictions > > > > and DRM_DEBUG usage. > > > > > > > > > > I'm not sure how useful it is since you appear to be able to > > > selectively adjust the cursor size, but I recently added support for > > > exposing the cursor size as a drm cap: > > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8716ed4e7bed4e4c7e3f37940e950ddc0362f450 > > > > > > Alex > > Thanks Alex. This is useful for cursor test to enumerate sub-tests based > > on these caps. > Realized after sending mail that we just get to know current cursor > width and height. Can we have capability that exposes all supported > cursor sizes? The cap exposes the max cursor size. Knowing our hardware we can infer that pot sizes from 64 to max are supported, but it would be better if we did expose that information through the cap as well. -Chris
On Mon, Mar 10, 2014 at 10:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Realized after sending mail that we just get to know current cursor >> width and height. Can we have capability that exposes all supported >> cursor sizes? > > The cap exposes the max cursor size. Knowing our hardware we can infer > that pot sizes from 64 to max are supported, but it would be better if > we did expose that information through the cap as well. I think the right approach here is to expose this through the cursor-as-real-plane interface, which kinda has this already. On top of that we can then add a few fourcc enumerations of the fixed rgba cursor layouts like 64x64, 128x128, ... This helps if the plane is a general one with very high limits, but also with special support for cursor formats. If anyone wants to go crazy we could then also add new fourccs for all the other cursor layouts - atm only rgba with fixed dimensions can be support with the current cursor ioctl. So reviewing the overall situation I actually _don't_ want a new cap/ioctl/prop here just for now. As long as we need to go with intel specific hacks userspace might as well probe things manually and act upon the -EINVAL. -Daniel
On Sun, Mar 09, 2014 at 12:19:06AM +0530, sagar.a.kamble@intel.com wrote: > From: Sagar Kamble <sagar.a.kamble@intel.com> > > With this patch we allow larger cursor planes of sizes 128x128 > and 256x256. > > v2: Added more precise check on size while setting cursor plane. > > v3: Changes related to restructuring cursor size restrictions > and DRM_DEBUG usage. > > Testcase: igt/kms_cursor_crc > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: G, Pallavi <pallavi.g@intel.com> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++----- > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 146609a..aee8258 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3551,7 +3551,11 @@ enum punit_power_well { > /* New style CUR*CNTR flags */ > #define CURSOR_MODE 0x27 > #define CURSOR_MODE_DISABLE 0x00 > +#define CURSOR_MODE_128_32B_AX 0x02 > +#define CURSOR_MODE_256_32B_AX 0x03 > #define CURSOR_MODE_64_32B_AX 0x07 > +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) > +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) > #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) > #define MCURSOR_PIPE_SELECT (1 << 28) > #define MCURSOR_PIPE_A 0x00 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0868afb..e59e8fd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7440,10 +7440,22 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > bool visible = base != 0; > > if (intel_crtc->cursor_visible != visible) { > + int16_t width = intel_crtc->cursor_width; > uint32_t cntl = I915_READ(CURCNTR(pipe)); > if (base) { > cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + > + switch (width) { > + case 64: > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; We don't needlessly indent case 64: and the following code. Even with that this indents a bit too far, so I suggest you extract the width->flags computation into a little helper and use it in both places. Also please extract the GAMMA flag, it's something else. Finally please add a default: case and put a WARN_ON(1) in there, to make sure our code doesn't let invalid cases slip through. -Daniel > + case 128: > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 256: > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + } > cntl |= pipe << 28; /* Connect to correct pipe */ > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > @@ -7468,10 +7480,22 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) > bool visible = base != 0; > > if (intel_crtc->cursor_visible != visible) { > + int16_t width = intel_crtc->cursor_width; > uint32_t cntl = I915_READ(CURCNTR_IVB(pipe)); > if (base) { > cntl &= ~CURSOR_MODE; > - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + > + switch (width) { > + case 64: > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 128: > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + case 256: > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + break; > + } > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > cntl |= CURSOR_MODE_DISABLE; > @@ -7567,9 +7591,11 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > goto finish; > } > > - /* Currently we only support 64x64 cursors */ > - if (width != 64 || height != 64) { > - DRM_ERROR("we currently only support 64x64 cursors\n"); > + /* Check for which cursor types we support */ > + if (!((width == 64 && height == 64) || > + (width == 128 && height == 128 && !IS_GEN2(dev)) || > + (width == 256 && height == 256 && !IS_GEN2(dev)))) { > + DRM_DEBUG("Cursor dimension not supported\n"); > return -EINVAL; > } > > -- > 1.8.5 >
On Mon, Mar 10, 2014 at 10:55:40AM +0100, Daniel Vetter wrote: > On Mon, Mar 10, 2014 at 10:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> Realized after sending mail that we just get to know current cursor > >> width and height. Can we have capability that exposes all supported > >> cursor sizes? > > > > The cap exposes the max cursor size. Knowing our hardware we can infer > > that pot sizes from 64 to max are supported, but it would be better if > > we did expose that information through the cap as well. > > I think the right approach here is to expose this through the > cursor-as-real-plane interface, which kinda has this already. On top > of that we can then add a few fourcc enumerations of the fixed rgba > cursor layouts like 64x64, 128x128, ... This helps if the plane is a > general one with very high limits, but also with special support for > cursor formats. > > If anyone wants to go crazy we could then also add new fourccs for all > the other cursor layouts - atm only rgba with fixed dimensions can be > support with the current cursor ioctl. > > So reviewing the overall situation I actually _don't_ want a new > cap/ioctl/prop here just for now. As long as we need to go with intel > specific hacks userspace might as well probe things manually and act > upon the -EINVAL. Adding the cap allows existing userspace to dtrt... -Chris
On Mon, Mar 10, 2014 at 11:04 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > The cap exposes the max cursor size. Knowing our hardware we can infer >> > that pot sizes from 64 to max are supported, but it would be better if >> > we did expose that information through the cap as well. >> >> I think the right approach here is to expose this through the >> cursor-as-real-plane interface, which kinda has this already. On top >> of that we can then add a few fourcc enumerations of the fixed rgba >> cursor layouts like 64x64, 128x128, ... This helps if the plane is a >> general one with very high limits, but also with special support for >> cursor formats. >> >> If anyone wants to go crazy we could then also add new fourccs for all >> the other cursor layouts - atm only rgba with fixed dimensions can be >> support with the current cursor ioctl. >> >> So reviewing the overall situation I actually _don't_ want a new >> cap/ioctl/prop here just for now. As long as we need to go with intel >> specific hacks userspace might as well probe things manually and act >> upon the -EINVAL. > > Adding the cap allows existing userspace to dtrt... 1. Move cursor off-screen. 2. Check desired cursor layout with set_cursor. 3. Pick real cursor, set it. Or 1. Use set_cursor on disabled crtc. Both aren't pretty, but that ugly should help to move cursors-as-real-planes forward ;-) -Daniel
On Mon, Mar 10, 2014 at 11:07 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Mon, Mar 10, 2014 at 11:04 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>> > The cap exposes the max cursor size. Knowing our hardware we can infer >>> > that pot sizes from 64 to max are supported, but it would be better if >>> > we did expose that information through the cap as well. >>> >>> I think the right approach here is to expose this through the >>> cursor-as-real-plane interface, which kinda has this already. On top >>> of that we can then add a few fourcc enumerations of the fixed rgba >>> cursor layouts like 64x64, 128x128, ... This helps if the plane is a >>> general one with very high limits, but also with special support for >>> cursor formats. >>> >>> If anyone wants to go crazy we could then also add new fourccs for all >>> the other cursor layouts - atm only rgba with fixed dimensions can be >>> support with the current cursor ioctl. >>> >>> So reviewing the overall situation I actually _don't_ want a new >>> cap/ioctl/prop here just for now. As long as we need to go with intel >>> specific hacks userspace might as well probe things manually and act >>> upon the -EINVAL. >> >> Adding the cap allows existing userspace to dtrt... > > 1. Move cursor off-screen. > 2. Check desired cursor layout with set_cursor. > 3. Pick real cursor, set it. > > Or > > 1. Use set_cursor on disabled crtc. > > Both aren't pretty, but that ugly should help to move > cursors-as-real-planes forward ;-) I stand corrected, there's now a cursor size cap in the drm-next branch (which is included in -nightly), so we need support this and also make sure it works correctly in the igt. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 146609a..aee8258 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3551,7 +3551,11 @@ enum punit_power_well { /* New style CUR*CNTR flags */ #define CURSOR_MODE 0x27 #define CURSOR_MODE_DISABLE 0x00 +#define CURSOR_MODE_128_32B_AX 0x02 +#define CURSOR_MODE_256_32B_AX 0x03 #define CURSOR_MODE_64_32B_AX 0x07 +#define CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX) +#define CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX) #define CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX) #define MCURSOR_PIPE_SELECT (1 << 28) #define MCURSOR_PIPE_A 0x00 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0868afb..e59e8fd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7440,10 +7440,22 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) bool visible = base != 0; if (intel_crtc->cursor_visible != visible) { + int16_t width = intel_crtc->cursor_width; uint32_t cntl = I915_READ(CURCNTR(pipe)); if (base) { cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + + switch (width) { + case 64: + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + case 128: + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + case 256: + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + } cntl |= pipe << 28; /* Connect to correct pipe */ } else { cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); @@ -7468,10 +7480,22 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) bool visible = base != 0; if (intel_crtc->cursor_visible != visible) { + int16_t width = intel_crtc->cursor_width; uint32_t cntl = I915_READ(CURCNTR_IVB(pipe)); if (base) { cntl &= ~CURSOR_MODE; - cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + + switch (width) { + case 64: + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + case 128: + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + case 256: + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; + break; + } } else { cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); cntl |= CURSOR_MODE_DISABLE; @@ -7567,9 +7591,11 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, goto finish; } - /* Currently we only support 64x64 cursors */ - if (width != 64 || height != 64) { - DRM_ERROR("we currently only support 64x64 cursors\n"); + /* Check for which cursor types we support */ + if (!((width == 64 && height == 64) || + (width == 128 && height == 128 && !IS_GEN2(dev)) || + (width == 256 && height == 256 && !IS_GEN2(dev)))) { + DRM_DEBUG("Cursor dimension not supported\n"); return -EINVAL; }