Re: [PATCH 3/4] drm/i915: Support NV12 in rotated GGTT mapping

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Fri, 25 Sep 2015 10:44:44 +0100

On 09/24/2015 05:35 PM, Ville Syrjälä wrote:
On Mon, Sep 21, 2015 at 10:45:34AM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Just adding the rotated UV plane at the end of the rotated Y plane.

v2: Rebase.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_gem_gtt.c  | 37 ++++++++++++++++++++++++++++++------
  drivers/gpu/drm/i915/i915_gem_gtt.h  |  3 +++
  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
  3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 59c934fb9230..2df9d16dcefd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3272,10 +3272,13 @@ intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
  {
  	struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
  	unsigned int size_pages = rot_info->size >> PAGE_SHIFT;
+	unsigned int size_pages_uv;
  	struct sg_page_iter sg_iter;
  	unsigned long i;
  	dma_addr_t *page_addr_list;
  	struct sg_table *st;
+	unsigned int uv_start_page;
+	struct scatterlist *sg;
  	int ret = -ENOMEM;

  	/* Allocate a temporary list of source pages for random access. */
@@ -3284,12 +3287,18 @@ intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
  	if (!page_addr_list)
  		return ERR_PTR(ret);

+	/* Account for UV plane with NV12. */
+	if (rot_info->pixel_format == DRM_FORMAT_NV12)
+		size_pages_uv = rot_info->size_uv >> PAGE_SHIFT;
+	else
+		size_pages_uv = 0;
+
  	/* Allocate target SG list. */
  	st = kmalloc(sizeof(*st), GFP_KERNEL);
  	if (!st)
  		goto err_st_alloc;

-	ret = sg_alloc_table(st, size_pages, GFP_KERNEL);
+	ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL);
  	if (ret)
  		goto err_sg_alloc;

@@ -3301,15 +3310,30 @@ intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
  	}

  	/* Rotate the pages. */
-	rotate_pages(page_addr_list, 0,
+	sg = rotate_pages(page_addr_list, 0,
  		     rot_info->width_pages, rot_info->height_pages,
  		     st, NULL);

+	/* Append the UV plane if NV12. */
+	if (rot_info->pixel_format == DRM_FORMAT_NV12) {
+		uv_start_page = size_pages;
+
+		/* Check for tile-row un-alignment. */
+		if (offset_in_page(rot_info->uv_offset))
+			uv_start_page--;
+
+		rotate_pages(page_addr_list, uv_start_page,
+			     rot_info->width_pages_uv,
+			     rot_info->height_pages_uv,
+			     st, sg);
+	}
+
  	DRM_DEBUG_KMS(
-		      "Created rotated page mapping for object size %zu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %u pages).\n",
+		      "Created rotated page mapping for object size %zu (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %u pages (%u plane 0)).\n",
  		      obj->base.size, rot_info->pitch, rot_info->height,
  		      rot_info->pixel_format, rot_info->width_pages,
-		      rot_info->height_pages, size_pages);
+		      rot_info->height_pages, size_pages + size_pages_uv,
+		      size_pages);

  	drm_free_large(page_addr_list);

@@ -3321,10 +3345,11 @@ err_st_alloc:
  	drm_free_large(page_addr_list);

  	DRM_DEBUG_KMS(
-		      "Failed to create rotated mapping for object size %zu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %u pages)\n",
+		      "Failed to create rotated mapping for object size %zu! (%d) (pitch=%u, height=%u, pixel_format=0x%x, %ux%u tiles, %u pages (%u plane 0))\n",
  		      obj->base.size, ret, rot_info->pitch, rot_info->height,
  		      rot_info->pixel_format, rot_info->width_pages,
-		      rot_info->height_pages, size_pages);
+		      rot_info->height_pages, size_pages + size_pages_uv,
+		      size_pages);
  	return ERR_PTR(ret);
  }

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 82750073d5b3..197183d5c543 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -138,10 +138,13 @@ enum i915_ggtt_view_type {
  struct intel_rotation_info {
  	unsigned int height;
  	unsigned int pitch;
+	unsigned int uv_offset;
  	uint32_t pixel_format;
  	uint64_t fb_modifier;
  	unsigned int width_pages, height_pages;
  	uint64_t size;
+	unsigned int width_pages_uv, height_pages_uv;
+	uint64_t size_uv;
  };

  struct i915_ggtt_view {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e19b8e699c00..2db7cc42539c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2263,6 +2263,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
  	info->height = fb->height;
  	info->pixel_format = fb->pixel_format;
  	info->pitch = fb->pitches[0];
+	info->uv_offset = fb->offsets[1];

OK, so this already came up a bit when I was looking at Chandra's stuff,
but we really have to define what fb->offsets[] means now.

On one hand, it would be very logical for it to be a raw byte offset
into the object where the fb starts. But on the other hand that perhaps
makes it a bit more difficult for userspace to compute offsets[1] for
the CbCr plane. Also we have fences to consider, and currently we even
require that the fence stride matches the fb stride, even though that
may not really be necessary. So maybe it should be the linear offset
instead.

So which way should we go?

No idea, I'm afraid I don't have the wide enough knowledge on this one 
to contribute hugely.

Why would it be difficult for userspace to compute a byte offset to the 
UV plane though? Presumably it had to know it already to have rendered 
anything - otherwise why and how would it be giving this frame buffer to 
the kernel?

Regards,

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







