| Message ID | 1392718186-12453-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 18, 2014 at 03:39:46PM +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. Planning to extend kms_cursor_crc test for verifying > these larger planes. > > 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 Let's not spam everyone. Just intel-gfx is enough for such patches. Well, maybe keep dri-devel too since cursor size seems to be a hot topic across other drivers currently. > 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 | 25 ++++++++++++++++++++----- > 2 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2f564ce..2fee3a2 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3522,7 +3522,11 @@ > /* 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 f19e6ea..00b51f3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7411,10 +7411,18 @@ 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; > + > + if (width == 64) > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + else if (width == 128) > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + else if (width == 256) > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + > cntl |= pipe << 28; /* Connect to correct pipe */ > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > @@ -7439,10 +7447,17 @@ 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; > + > + if (width == 64) > + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + else if (width == 128) > + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; > + else if (width == 256) > + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; > } else { > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > cntl |= CURSOR_MODE_DISABLE; > @@ -7538,9 +7553,9 @@ 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 > 256 || height > 256) { This has to check explicitly for 64x64, 128x128, or 256x256. Any other combination of width and height is illegal. Additionally gen2 supports only 64x64, so that has to be checked also. Another issue is how to tell userspace about the cursor size. There was a patch on dri-devel recently adding cursor width/height capability queries to drm. I guess that should be enough for xorg since AFAICS it only uses a single fixed cursor size. The downside is that if the actual cursor image would fit one of the smaller sizes, we end up doing needless memory fetches for the extra pixels. Once we have drm_planes for cursors, I was thinking we might add some kind of enum property that lists all the supported sizes for the plane. > + DRM_ERROR("We currently only support 64x64, 128x128, 256x256 cursors\n"); > return -EINVAL; > } > > -- > 1.8.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Feb 18, 2014 at 03:13:33PM +0200, Ville Syrjälä wrote: [...] > Once we have drm_planes for cursors, I was thinking we might add some kind > of enum property that lists all the supported sizes for the plane. This comment was intriguing, so I was wondering whether you could elaborate a little on this. The reason why I ask is that we have a fairly similar issue on Tegra, where recent hardware supports 128x128 and 256x256 hardware cursors, whereas older generations support only 32x32 and 64x64. Also very early generations don't support ARGB cursors, but a somewhat funky pixel format (2 bpp, background, foreground, transparent, invert). With the current set of cursor IOCTLs it isn't really practical to support the legacy kind of cursors, but representing the cursor as a plane would have the benefit of attaching a number of supported formats as well as resolutions to it. That would make it a whole lot easier to support these additional modes. As for the supported sizes enumeration, how do you intend to handle the correlation between horizontal and vertical sizes, since an enumeration property is only a single 32-bit value. I suppose if we limit ourselves to supporting only cursors with 64Kx64K we could stash horizontal and vertical dimensions into a single 32-bit value. Also, is there a plan to add a type field to the planes so that userspace can determine if it's a proper overlay or a cursor? Thierry
On Tue, Feb 25, 2014 at 12:35:21PM +0100, Thierry Reding wrote: > On Tue, Feb 18, 2014 at 03:13:33PM +0200, Ville Syrjälä wrote: > [...] > > Once we have drm_planes for cursors, I was thinking we might add some kind > > of enum property that lists all the supported sizes for the plane. > > This comment was intriguing, so I was wondering whether you could > elaborate a little on this. The reason why I ask is that we have a > fairly similar issue on Tegra, where recent hardware supports 128x128 > and 256x256 hardware cursors, whereas older generations support only > 32x32 and 64x64. Also very early generations don't support ARGB cursors, > but a somewhat funky pixel format (2 bpp, background, foreground, > transparent, invert). Yeah there a a bunch of legacy cursor modes on most hardware which might be supportable. Whether or not they would actually get used is another matter though. I think these days everyone expects the cursor to have alpha at least. I guess userspace could figure out if the legacy formats are enough to represent the cursor image, and maybe even decide to take a small quality hit by converting the ARGB image to one of the legacy formats if it's "close enough". I think for i915 we'll just not bother with that since all our hardware has ARGB cursor support. > > With the current set of cursor IOCTLs it isn't really practical to > support the legacy kind of cursors, but representing the cursor as a > plane would have the benefit of attaching a number of supported formats > as well as resolutions to it. That would make it a whole lot easier to > support these additional modes. > > As for the supported sizes enumeration, how do you intend to handle the > correlation between horizontal and vertical sizes, since an enumeration > property is only a single 32-bit value. I suppose if we limit ourselves > to supporting only cursors with 64Kx64K we could stash horizontal and > vertical dimensions into a single 32-bit value. Prop values are 64bit so you could stick a 32x32 dimensions into a single value. Or you could even ignore the actual value, and just stick the dimensions to the enum name as "%ux%u". But userspace would need to parse that, so maybe doing both is the best option. The only issue here is that enum properties must have a current value. That doesn't really make sense in this case. So maybe we actually want to come up with some kind of plane caps mechanism that would be more fitting for this purpose. OTOH I'm not sure adding yet another mechanism is worth it. > > Also, is there a plan to add a type field to the planes so that > userspace can determine if it's a proper overlay or a cursor? First you have to define what's the differnece between a cursor and a proper overlay.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2f564ce..2fee3a2 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3522,7 +3522,11 @@ /* 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 f19e6ea..00b51f3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7411,10 +7411,18 @@ 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; + + if (width == 64) + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + else if (width == 128) + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; + else if (width == 256) + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; + cntl |= pipe << 28; /* Connect to correct pipe */ } else { cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); @@ -7439,10 +7447,17 @@ 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; + + if (width == 64) + cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE; + else if (width == 128) + cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE; + else if (width == 256) + cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE; } else { cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); cntl |= CURSOR_MODE_DISABLE; @@ -7538,9 +7553,9 @@ 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 > 256 || height > 256) { + DRM_ERROR("We currently only support 64x64, 128x128, 256x256 cursors\n"); return -EINVAL; }