| Message ID | 1442828735-9448-4-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } >
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
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
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
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.
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
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
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; }