Re: [Intel-gfx] [RFC 4/4] drm/i915: Switch to unified plane cursor handling

2014-05-16 Thread Chris Wilson
On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
 The DRM core will translate calls to legacy cursor ioctls into universal
 cursor calls automatically, so there's no need to maintain the legacy
 cursor support.  This greatly simplifies the transition since we don't
 have to handle reference counting differently depending on which cursor
 interface was called.
 
 The aim here is to transition to the universal plane interface with
 minimal code change.  There's a lot of cleanup that can be done (e.g.,
 using state stored in crtc-cursor-fb rather than intel_crtc) that is
 left to future patches.
 
 Signed-off-by: Matt Roper matthew.d.ro...@intel.com
 ---
 +static int
 +intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 +   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 +   unsigned int crtc_w, unsigned int crtc_h,
 +   uint32_t src_x, uint32_t src_y,
 +   uint32_t src_w, uint32_t src_h)
 +{
 + struct drm_device *dev = crtc-dev;
 + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 + struct drm_i915_gem_object *obj = intel_fb-obj;
 + struct drm_rect dest = {
 + /* integer pixels */
 + .x1 = crtc_x,
 + .y1 = crtc_y,
 + .x2 = crtc_x + crtc_w,
 + .y2 = crtc_y + crtc_h,
 + };
 + struct drm_rect src = {
 + /* 16.16 fixed point */
 + .x1 = src_x,
 + .y1 = src_y,
 + .x2 = src_x + src_w,
 + .y2 = src_y + src_h,
 + };
 + const struct drm_rect clip = {
 + /* integer pixels */
 + .x2 = intel_crtc-config.pipe_src_w,
 + .y2 = intel_crtc-config.pipe_src_h,
 + };
 + int hscale, vscale;
 + bool visible;
 +
 + /* Check scaling */
 + hscale = drm_rect_calc_hscale(src, dest,
 +   DRM_PLANE_HELPER_NO_SCALING,
 +   DRM_PLANE_HELPER_NO_SCALING);
 + vscale = drm_rect_calc_vscale(src, dest,
 +   DRM_PLANE_HELPER_NO_SCALING,
 +   DRM_PLANE_HELPER_NO_SCALING);
 + if (hscale  0 || vscale  0) {
 + DRM_DEBUG_KMS(Invalid scaling of cursor plane\n);
 + return -ERANGE;
 + }
 +
 + /* Check dimensions */
 + if (!((crtc_w == 64  crtc_h == 64) ||
 + (crtc_w == 128  crtc_h == 128  !IS_GEN2(dev)) ||
 + (crtc_w == 256  crtc_h == 256  !IS_GEN2(dev {
 + DRM_DEBUG_KMS(Cursor dimension not supported: %dx%d\n,
 +   crtc_w, crtc_h);
 + return -EINVAL;
 + }
 +
 + /* Clip to display; if no longer visible after clipping, disable */
 + visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
 +
 + crtc-cursor_x = crtc_x;
 + crtc-cursor_y = crtc_y;
 + if (fb != crtc-cursor-fb) {
 + return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
 +  crtc_w, crtc_h);
 + } else {
 + intel_crtc_update_cursor(crtc, true);
 + return 0;
 + }

Does this do the right thing for a cursor that is no longer visible, and
vice versa? It is not immediately clear how clipping now works with
intel_crtc_update_cursor().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [RFC 4/4] drm/i915: Switch to unified plane cursor handling

2014-05-16 Thread Daniel Vetter
On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
 The DRM core will translate calls to legacy cursor ioctls into universal
 cursor calls automatically, so there's no need to maintain the legacy
 cursor support.  This greatly simplifies the transition since we don't
 have to handle reference counting differently depending on which cursor
 interface was called.
 
 The aim here is to transition to the universal plane interface with
 minimal code change.  There's a lot of cleanup that can be done (e.g.,
 using state stored in crtc-cursor-fb rather than intel_crtc) that is
 left to future patches.
 
 Signed-off-by: Matt Roper matthew.d.ro...@intel.com

Bunch of comments below. I think for testing we have good coverage of
functional cursor tests already, and with the backwards-compat code in the
drm core we'll excerise the cursor plane code sufficiently imo.

But there's a bit of corner-case checking:
- Interactions between legacy cursor ioctl and the new stuff to check the
  refcounting and correctness wrt set_bo and visible and fun like that.
- Maybe some checks for the no-upscaling and no-clipping stuff. Hopefully
  you can easily copypaste this from the primary plane tests.

But in the end you're worked on this and are the expert, so if you
think a different focus gives a better payoff then I'm all ears - we want
to write tests that have a good chance of catching real bugs after all ;-)

Cheers, Daniel
 ---
  drivers/gpu/drm/i915/intel_display.c | 194 
 ---
  drivers/gpu/drm/i915/intel_drv.h |   1 -
  2 files changed, 136 insertions(+), 59 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 7e75b13..d145130 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
   DRM_FORMAT_ABGR2101010,
  };
  
 +/* Cursor formats */
 +static const uint32_t intel_cursor_formats[] = {
 + DRM_FORMAT_ARGB,
 +};
 +
  #define DIV_ROUND_CLOSEST_ULL(ll, d) \
  ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
  
 @@ -7967,8 +7972,8 @@ static void intel_crtc_update_cursor(struct drm_crtc 
 *crtc,
   struct drm_i915_private *dev_priv = dev-dev_private;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   int pipe = intel_crtc-pipe;
 - int x = intel_crtc-cursor_x;
 - int y = intel_crtc-cursor_y;
 + int x = crtc-cursor_x;
 + int y = crtc-cursor_y;
   u32 base = 0, pos = 0;
   bool visible;
  
 @@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc 
 *crtc,
   * @height: cursor height
   *
   * Programs a crtc's cursor plane with the specified GEM object.
 - * intel_crtc_cursor_set() should be used instead if we have a userspace 
 buffer
 - * handle rather than the actual GEM object.
   */
  static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
struct drm_i915_gem_object *obj,
 @@ -8143,48 +8146,6 @@ fail:
   return ret;
  }
  
 -/**
 - * intel_crtc_cursor_set - Update cursor buffer
 - * @crtc: crtc to set cursor for
 - * @handle: userspace handle of buffer to set cursor to
 - * @width: cursor width
 - * @height: cursor height
 - *
 - * Programs a crtc's cursor plane with the buffer referred to by userspace
 - * handle @handle.
 - */
 -static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 -  struct drm_file *file,
 -  uint32_t handle,
 -  uint32_t width, uint32_t height)
 -{
 - struct drm_device *dev = crtc-dev;
 - struct drm_i915_gem_object *obj;
 -
 - if (handle) {
 - obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
 - if (obj-base == NULL)
 - return -ENOENT;
 - } else {
 - obj = NULL;
 - }
 -
 - return intel_crtc_cursor_set_obj(crtc, obj, width, height);
 -}
 -
 -static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 -{
 - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 -
 - intel_crtc-cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
 - intel_crtc-cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
 -
 - if (intel_crtc-active)
 - intel_crtc_update_cursor(crtc, intel_crtc-cursor_bo != NULL);
 -
 - return 0;
 -}
 -
  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
u16 *blue, uint32_t start, uint32_t size)
  {
 @@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
   kfree(work);
   }
  
 - intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
 + intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);

This shouldn't be needed any more with the plane cleanup code - all the
refcounting is now done with fbs.
  
   drm_crtc_cleanup(crtc);
  
 @@ -10784,8 +10745,6 @@