[3/4] drm/i915: Support NV12 in rotated GGTT mapping
diff mbox

Message ID 1442828735-9448-4-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Sept. 21, 2015, 9:45 a.m. UTC
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(-)

Comments

Joonas Lahtinen Sept. 21, 2015, 11:14 a.m. UTC | #1
On ma, 2015-09-21 at 10:45 +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>
> 

One comment below, otherwise.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.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;

Could be initialized to zero here already as majority of the time it'll
be unchanged.

>  > 	> 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];
>  > 	> info->fb_modifier = fb->modifier[0];
>  
>  > 	> tile_height = intel_tile_height(fb->dev, fb->pixel_format,
> @@ -2272,6 +2273,17 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view
> *view, struct drm_framebuffer *fb,
>  > 	> info->height_pages = DIV_ROUND_UP(fb->height, tile_height);
>  > 	> info->size = info->width_pages * info->height_pages *
> PAGE_SIZE;
>  
> +> 	> if (info->pixel_format == DRM_FORMAT_NV12) {
> +> 	> 	> tile_height = intel_tile_height(fb->dev, fb
> ->pixel_format,
> +> 	> 	> 	> 	> 	> 	> fb->modifier[0], 1);
> +> 	> 	> tile_pitch = PAGE_SIZE / tile_height;
> +> 	> 	> info->width_pages_uv = DIV_ROUND_UP(fb->pitches[0],
> tile_pitch);
> +> 	> 	> info->height_pages_uv = DIV_ROUND_UP(fb->height / 2,
> +> 	> 	> 	> 	> 	> 	>      tile_height);
> +> 	> 	> info->size_uv = info->width_pages_uv * info
> ->height_pages_uv *
> +> 	> 	> 	> 	> PAGE_SIZE;
> +> 	> }
> +
>  > 	> return 0;
>  }
>
Daniel Vetter Sept. 23, 2015, 3:31 p.m. UTC | #2
On Mon, Sep 21, 2015 at 02:14:47PM +0300, Joonas Lahtinen wrote:
> On ma, 2015-09-21 at 10:45 +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>
> > 
> 
> One comment below, otherwise.
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.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;
> 
> Could be initialized to zero here already as majority of the time it'll
> be unchanged.

Personally (and this is really pretty clearly in bikeshed territory) I
prefer it like Tvrtko has done since if you ever rework the code for new
platform support and forget one case gcc will complain about potentially
uninitialized variable. But if you initialize it at first that will paper
over such a bug. And gcc will optimize this anyway for you (besides that
generally performance in modeset code just doesn't really matter all that
much).

And something funny is going on with your mail replies, you insert piles
of spurious > even though it's just one reply level ...
-Daniel
Ville Syrjala Sept. 24, 2015, 4:35 p.m. UTC | #3
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?


>  	info->fb_modifier = fb->modifier[0];
>  
>  	tile_height = intel_tile_height(fb->dev, fb->pixel_format,
> @@ -2272,6 +2273,17 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>  	info->height_pages = DIV_ROUND_UP(fb->height, tile_height);
>  	info->size = info->width_pages * info->height_pages * PAGE_SIZE;
>  
> +	if (info->pixel_format == DRM_FORMAT_NV12) {
> +		tile_height = intel_tile_height(fb->dev, fb->pixel_format,
> +						fb->modifier[0], 1);
> +		tile_pitch = PAGE_SIZE / tile_height;
> +		info->width_pages_uv = DIV_ROUND_UP(fb->pitches[0], tile_pitch);
> +		info->height_pages_uv = DIV_ROUND_UP(fb->height / 2,
> +						     tile_height);
> +		info->size_uv = info->width_pages_uv * info->height_pages_uv *
> +				PAGE_SIZE;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tvrtko Ursulin Sept. 25, 2015, 9:44 a.m. UTC | #4
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?

Regards,

Tvrtko
Ville Syrjala Sept. 25, 2015, 11:29 a.m. UTC | #5
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.
Daniel Vetter Sept. 28, 2015, 8:37 a.m. UTC | #6
On Fri, Sep 25, 2015 at 02:29:59PM +0300, Ville Syrjälä wrote:
> 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.

Hm I kinda assumed that for tiled layouts plane offsets must be
tile-aligned for everyone. Since tiling is now a separate part with the fb
modifiers I'd go with using it as the linear offset. But definitely needs
to be documented with a kerneldoc pathc somewhere ...
-Daniel
Ville Syrjala Sept. 28, 2015, 12:41 p.m. UTC | #7
On Mon, Sep 28, 2015 at 10:37:24AM +0200, Daniel Vetter wrote:
> On Fri, Sep 25, 2015 at 02:29:59PM +0300, Ville Syrjälä wrote:
> > 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.
> 
> Hm I kinda assumed that for tiled layouts plane offsets must be
> tile-aligned for everyone.

Nah, we should be able to have it just (macro)pixel aligned. I have
it as stride aligned in my tile_size branch just because I didn't want
to think about the extra x offset initially. But I suppose the extra x
offset shouldn't really cause any issues. I also didn't (yet) properly
deal with the fact that the rotated view wouldn't necessarily cover
the entire object.

> Since tiling is now a separate part with the fb
> modifiers I'd go with using it as the linear offset. But definitely needs
> to be documented with a kerneldoc pathc somewhere ...
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch
diff mbox

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];
 	info->fb_modifier = fb->modifier[0];
 
 	tile_height = intel_tile_height(fb->dev, fb->pixel_format,
@@ -2272,6 +2273,17 @@  intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 	info->height_pages = DIV_ROUND_UP(fb->height, tile_height);
 	info->size = info->width_pages * info->height_pages * PAGE_SIZE;
 
+	if (info->pixel_format == DRM_FORMAT_NV12) {
+		tile_height = intel_tile_height(fb->dev, fb->pixel_format,
+						fb->modifier[0], 1);
+		tile_pitch = PAGE_SIZE / tile_height;
+		info->width_pages_uv = DIV_ROUND_UP(fb->pitches[0], tile_pitch);
+		info->height_pages_uv = DIV_ROUND_UP(fb->height / 2,
+						     tile_height);
+		info->size_uv = info->width_pages_uv * info->height_pages_uv *
+				PAGE_SIZE;
+	}
+
 	return 0;
 }