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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 25, 2015 at 10:44:44AM +0100, Tvrtko Ursulin wrote:
> 
> 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@intel.com>
> >>
> >> Just adding the rotated UV plane at the end of the rotated Y plane.
> >>
> >> v2: Rebase.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> ---
> >>   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?

Well, it's not difficult per say, but it would have to know the tile
dimensions to do it. When using a linear offset, the tile dimensions are
mostly meaningless, although some of the hw restrictions (stride must be
tile aligned, and CbCr plane must start at somewhat aligned offset
at least sometimes) does blow a hole to that theory a bit.

I suppose it's not really a huge deal which way we go, we just have to
pick one and stick with it, and document it.

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