On Tue, Sep 02, 2014 at 04:23:46PM -0300, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

This new struct will be the storage of src and dst coordinates between the check and commit stages of a plane update.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/i915/intel_drv.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4ab0d92..59c1675 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -33,6 +33,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_dp_mst_helper.h> +#include <drm/drm_rect.h>

/**

• _wait_for - magic (register) wait macro

@@ -227,6 +228,25 @@ typedef struct dpll { int p; } intel_clock_t;

+struct intel_plane_state {

• 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;

Seems to me we shouldn't need these non-drm_rect coordinates in this struct. I'll comment a bit more on the next patch since that's where this is used.

• struct drm_rect src;
• struct drm_rect dst;
• struct drm_rect clip;
• struct drm_rect orig_src;
• struct drm_rect orig_dst;
• bool visible;

+};

struct intel_plane_config { bool tiled; int size; -- 1.9.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Sep 02, 2014 at 04:23:47PM -0300, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

Due to the upcoming atomic modesetting feature we need to separate some update functions into a check step that can fail and a commit step that should, ideally, never fail.

This commit splits intel_update_plane() and its commit part can still fail due to the fb pinning procedure.

v2: use the new struct intel_plane_state to store information between check and commit stages.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/i915/intel_sprite.c | 236 ++++++++++++++++++++++-------------- 1 file changed, 146 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 4cbe286..062eca2 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -845,57 +845,28 @@ static bool colorkey_enabled(struct intel_plane *intel_plane) }

static int -intel_update_plane(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)
  

+intel_check_sprite_plane(struct drm_plane *plane,

• 	 struct intel_plane_state *pstate)
  

pstate looks a bit funny. Makes me think on cpu pstates. Maybe just call it 'state' everywhere?

{

• struct drm_device *dev = plane->dev;
• struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• struct intel_crtc *intel_crtc = to_intel_crtc(pstate->crtc); struct intel_plane *intel_plane = to_intel_plane(plane);

• enum pipe pipe = intel_crtc->pipe;

• struct drm_framebuffer *fb = pstate->fb; struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); struct drm_i915_gem_object *obj = intel_fb->obj;

• struct drm_i915_gem_object *old_obj = intel_plane->obj;
• int ret;
• bool primary_enabled;
• bool visible;

• int crtc_x = pstate->crtc_x;
• int crtc_y = pstate->crtc_y;
• int crtc_w = pstate->crtc_w;
• int crtc_h = pstate->crtc_h;
• int src_x = pstate->src_x;
• int src_y = pstate->src_y;
• int src_w = pstate->src_w;
• int src_h = pstate->src_h;

AFAICS we don't need to initialize this stuff here. We already populate it all before we need it further down. So I think the only thing we need is add a bit of code to write the src_? coordinates back into pstate->src (at the end of check() like you already do for the dst coordinates). To avoid warning about unitialized variables that probably needs to be wrapped in an 'if (visible)' check.

• struct drm_rect *src = &pstate->src;
• struct drm_rect *dst = &pstate->dst;
• struct drm_rect *clip = &pstate->clip;

Please keep 'clip' const here to prevent silly accidents.

int hscale, vscale; int max_scale, min_scale; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);

• struct drm_rect src = {

• /* sample coordinates in 16.16 fixed point */
  

• .x1 = src_x,
  

• .x2 = src_x + src_w,
  

• .y1 = src_y,
  

• .y2 = src_y + src_h,
  

• };

• struct drm_rect dst = {

• /* integer pixels */
  

• .x1 = crtc_x,
  

• .x2 = crtc_x + crtc_w,
  

• .y1 = crtc_y,
  

• .y2 = crtc_y + crtc_h,
  

• };

• const struct drm_rect clip = {

• .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
  

• .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
  

• };

• const struct {

• int crtc_x, crtc_y;
  

• unsigned int crtc_w, crtc_h;
  

• uint32_t src_x, src_y, src_w, src_h;
  

• } orig = {

• .crtc_x = crtc_x,
  

• .crtc_y = crtc_y,
  

• .crtc_w = crtc_w,
  

• .crtc_h = crtc_h,
  

• .src_x = src_x,
  

• .src_y = src_y,
  

• .src_w = src_w,
  

• .src_h = src_h,
  

• };

  /* Don't modify another pipe's plane */ if (intel_plane->pipe != intel_crtc->pipe) {

@@ -927,55 +898,55 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, max_scale = intel_plane->max_downscale << 16; min_scale = intel_plane->can_scale ? 1 : (1 << 16);

• drm_rect_rotate(&src, fb->width << 16, fb->height << 16,

• drm_rect_rotate(src, fb->width << 16, fb->height << 16, intel_plane->rotation);

• hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);

• hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale); BUG_ON(hscale < 0);

• vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);

• vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); BUG_ON(vscale < 0);

• visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);

• pstate->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);

• crtc_x = dst.x1;
• crtc_y = dst.y1;
• crtc_w = drm_rect_width(&dst);
• crtc_h = drm_rect_height(&dst);

• crtc_x = dst->x1;
• crtc_y = dst->y1;
• crtc_w = drm_rect_width(dst);
• crtc_h = drm_rect_height(dst);

• if (visible) {

• if (pstate->visible) { /* check again in case clipping clamped the results */

• hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
  

• hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
  

  if (hscale < 0) { DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");

• 	drm_rect_debug_print(&src, true);
  

• 	drm_rect_debug_print(&dst, false);
  

• 	drm_rect_debug_print(src, true);
  

• 	drm_rect_debug_print(dst, false);
  
  return hscale;
  

  }

• vscale = drm_rect_calc_vscale(&src, &dst, min_scale, max_scale);
  

• vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
  

  if (vscale < 0) { DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");

• 	drm_rect_debug_print(&src, true);
  

• 	drm_rect_debug_print(&dst, false);
  

• 	drm_rect_debug_print(src, true);
  

• 	drm_rect_debug_print(dst, false);
  
  return vscale;
  

  }

  /* Make the source viewport size an exact multiple of the scaling factors. */

• drm_rect_adjust_size(&src,
  

• 		     drm_rect_width(&dst) * hscale - drm_rect_width(&src),
  

• 		     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
  

• drm_rect_adjust_size(src,
  

• 		     drm_rect_width(dst) * hscale - drm_rect_width(src),
  

• 		     drm_rect_height(dst) * vscale - drm_rect_height(src));
  
  

• drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
  

• drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
  	    intel_plane->rotation);
  
  

  /* sanity check to make sure the src viewport wasn't enlarged */

• WARN_ON(src.x1 < (int) src_x ||
  

• 	src.y1 < (int) src_y ||
  

• 	src.x2 > (int) (src_x + src_w) ||
  

• 	src.y2 > (int) (src_y + src_h));
  

• WARN_ON(src->x1 < (int) src_x ||
  

• 	src->y1 < (int) src_y ||
  

• 	src->x2 > (int) (src_x + src_w) ||
  

• 	src->y2 > (int) (src_y + src_h));
  
  

  /*

  • Hardware doesn't handle subpixel coordinates.

@@ -983,10 +954,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, * increase the source viewport size, because that could * push the downscaling factor out of bounds. */

• src_x = src.x1 >> 16;
  

• src_w = drm_rect_width(&src) >> 16;
  

• src_y = src.y1 >> 16;
  

• src_h = drm_rect_height(&src) >> 16;
  

• src_x = src->x1 >> 16;
  

• src_w = drm_rect_width(src) >> 16;
  

• src_y = src->y1 >> 16;
  

• src_h = drm_rect_height(src) >> 16;
  
  

  if (format_is_yuv(fb->pixel_format)) { src_x &= ~1;

@@ -1000,12 +971,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, crtc_w &= ~1;

	if (crtc_w == 0)

• 		visible = false;
  

• 		pstate->visible = false;
  

  } }

  /* Check size restrictions when scaling */

• if (visible && (src_w != crtc_w || src_h != crtc_h)) {

• if (pstate->visible && (src_w != crtc_w || src_h != crtc_h)) { unsigned int width_bytes;

  WARN_ON(!intel_plane->can_scale);

@@ -1013,10 +984,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, /* FIXME interlacing min height is 6 */

if (crtc_w < 3 || crtc_h < 3)

• 	visible = false;
  

• 	pstate->visible = false;
  
  

  if (src_w < 3 || src_h < 3)

• 	visible = false;
  

• 	pstate->visible = false;
  
  

  width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;

@@ -1027,17 +998,55 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, } }

• dst.x1 = crtc_x;
• dst.x2 = crtc_x + crtc_w;
• dst.y1 = crtc_y;
• dst.y2 = crtc_y + crtc_h;

• dst->x1 = crtc_x;
• dst->x2 = crtc_x + crtc_w;
• dst->y1 = crtc_y;
• dst->y2 = crtc_y + crtc_h;
• pstate->src_x = src_x;
• pstate->src_y = src_y;
• pstate->src_w = src_w;
• pstate->src_h = src_h;
• pstate->crtc_x = crtc_x;
• pstate->crtc_y = crtc_y;
• pstate->crtc_w = crtc_w;
• pstate->crtc_x = crtc_h;
• return 0;

+}

+static int +intel_commit_sprite_plane(struct drm_plane *plane,

• 	  struct intel_plane_state *pstate)
  

+{

• struct drm_device *dev = plane->dev;
• struct drm_crtc *crtc = pstate->crtc;
• struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
• struct intel_plane *intel_plane = to_intel_plane(plane);
• enum pipe pipe = intel_crtc->pipe;
• struct drm_framebuffer *fb = pstate->fb;
• struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
• struct drm_i915_gem_object *obj = intel_fb->obj;
• struct drm_i915_gem_object *old_obj = intel_plane->obj;
• int crtc_x = pstate->crtc_x;
• int crtc_y = pstate->crtc_y;
• int crtc_w = pstate->crtc_w;
• int crtc_h = pstate->crtc_h;
• int src_x = pstate->src_x;
• int src_y = pstate->src_y;
• int src_w = pstate->src_w;
• int src_h = pstate->src_h;

As I mentioned on the previous patch I don't think we need the coordinates stored twice in the plane config.

Just do: crtc_x = pstate->dst.x1; ... crtc_w = drm_rect_width(&pstate->dst); ...

etc.

• struct drm_rect *dst = &pstate->dst;

• struct drm_rect *clip = &pstate->clip;

• int ret;

• bool primary_enabled;

  /*

  • If the sprite is completely covering the primary plane,
  • we can disable the primary and save power.

  */

• primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
• WARN_ON(!primary_enabled && !visible && intel_crtc->active);

• primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);

• WARN_ON(!primary_enabled && !pstate->visible && intel_crtc->active);

  mutex_lock(&dev->struct_mutex);

@@ -1055,14 +1064,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret;

• intel_plane->crtc_x = orig.crtc_x;
• intel_plane->crtc_y = orig.crtc_y;
• intel_plane->crtc_w = orig.crtc_w;
• intel_plane->crtc_h = orig.crtc_h;
• intel_plane->src_x = orig.src_x;
• intel_plane->src_y = orig.src_y;
• intel_plane->src_w = orig.src_w;
• intel_plane->src_h = orig.src_h;

• intel_plane->crtc_x = pstate->orig_dst.x1;

• intel_plane->crtc_y = pstate->orig_dst.y1;

• intel_plane->crtc_w = drm_rect_width(&pstate->orig_dst);

• intel_plane->crtc_h = drm_rect_height(&pstate->orig_dst);

• intel_plane->src_x = pstate->orig_src.x1;

• intel_plane->src_y = pstate->orig_src.y1;

• intel_plane->src_w = drm_rect_width(&pstate->orig_src);

• intel_plane->src_h = drm_rect_height(&pstate->orig_src); intel_plane->obj = obj;

  if (intel_crtc->active) {

@@ -1076,7 +1085,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (primary_was_enabled && !primary_enabled) intel_pre_disable_primary(crtc);

• if (visible)
  

• if (pstate->visible)
  intel_plane->update_plane(plane, crtc, fb, obj,
  			  crtc_x, crtc_y, crtc_w, crtc_h,
  			  src_x, src_y, src_w, src_h);
  

@@ -1109,6 +1118,53 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }

static int +intel_update_plane(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 intel_plane_state pstate;
• struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
• int ret;
• pstate.crtc = crtc;
• pstate.fb = fb;
• pstate.crtc_x = crtc_x;
• pstate.crtc_y = crtc_y;
• pstate.crtc_w = crtc_w;
• pstate.crtc_h = crtc_h;
• pstate.src_x = src_x;
• pstate.src_y = src_y;
• pstate.src_w = src_w;
• pstate.src_h = src_h;
• pstate.src.x1 = src_x,
• pstate.src.x2 = src_x + src_w,
• pstate.src.y1 = src_y,
• pstate.src.y2 = src_y + src_h,
• pstate.dst.x1 = crtc_x,
• pstate.dst.x2 = crtc_x + crtc_w,
• pstate.dst.y1 = crtc_y,
• pstate.dst.y2 = crtc_y + crtc_h,
• pstate.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
• pstate.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,

You need to initialize clip x1 and y1 too since pstate isn't zero initialized.

• pstate.orig_src.x1 = src_x,
• pstate.orig_src.x2 = src_x + src_w,
• pstate.orig_src.y1 = src_y,
• pstate.orig_src.y2 = src_y + src_h,
• pstate.orig_dst.x1 = crtc_x,
• pstate.orig_dst.x2 = crtc_x + crtc_w,
• pstate.orig_dst.y1 = crtc_y,
• pstate.orig_dst.y2 = crtc_y + crtc_h,

s/,/;/ in a lots of places here.

But I think we can simplify this a bit like so:

pstate.orig.src... = ..; pstate.orig.dst... = ..;

pstate.src = pstate.orig.src; pstate.dst = pstate.orig.dst;

• ret = intel_check_sprite_plane(plane, &pstate);
• if (ret)

• return ret;
  

• return intel_commit_sprite_plane(plane, &pstate);

+}

+static int intel_disable_plane(struct drm_plane *plane) { struct drm_device *dev = plane->dev; -- 1.9.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Sep 03, 2014 at 11:18:27AM +0300, Ville Syrjälä wrote:

On Tue, Sep 02, 2014 at 04:23:44PM -0300, Gustavo Padovan wrote:

...

From: Derek Foreman derek.foreman@collabora.co.uk

Really just for completeness - old init function ends up making the plane exactly the same way due to the way the enums are set up.

Signed-off-by: Derek Foreman derek.foreman@collabora.co.uk

Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

Queued for -next, thanks for the patch. -Daniel

...

---

drivers/gpu/drm/i915/intel_sprite.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0bdb00b..4cbe286 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1375,10 +1375,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->plane = plane; intel_plane->rotation = BIT(DRM_ROTATE_0); possible_crtcs = (1 << pipe);

• ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,

• 	     &intel_plane_funcs,
  

• 	     plane_formats, num_plane_formats,
  

• 	     false);
  

• ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,

• 		       &intel_plane_funcs,
  

• 		       plane_formats, num_plane_formats,
  

• 		       DRM_PLANE_TYPE_OVERLAY);
  

  if (ret) { kfree(intel_plane); goto out;

-- 1.9.3

---

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

-- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Sep 03, 2014 at 10:38:20AM -0300, Gustavo Padovan wrote:

From: Derek Foreman derek.foreman@collabora.co.uk

Really just for completeness - old init function ends up making the plane exactly the same way due to the way the enums are set up.

Signed-off-by: Derek Foreman derek.foreman@collabora.co.uk Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Queued for -next, thanks for the patch. -Daniel

---

drivers/gpu/drm/i915/intel_sprite.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0bdb00b..4cbe286 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1375,10 +1375,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->plane = plane; intel_plane->rotation = BIT(DRM_ROTATE_0); possible_crtcs = (1 << pipe);

• ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,

• 	     &intel_plane_funcs,
  

• 	     plane_formats, num_plane_formats,
  

• 	     false);
  

• ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,

• 		       &intel_plane_funcs,
  

• 		       plane_formats, num_plane_formats,
  

• 		       DRM_PLANE_TYPE_OVERLAY);
  

  if (ret) { kfree(intel_plane); goto out;

-- 1.9.3