[Intel-gfx] [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Mar 3 01:59:31 PST 2015
On 03/02/2015 06:21 PM, Daniel Vetter wrote:
> On Mon, Mar 02, 2015 at 02:43:50PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> 90/270 rotated scanout needs a rotated GTT view of the framebuffer.
>>
>> This is put in a separate VMA with a dedicated ggtt_view and wired suchs that
>> it is created when a framebuffer is pinned to a 90/270 rotated plane.
>>
>> Rotation is only possible with Yb/Yf buffers and error is propagated to
>> user space in case of a mismatch.
>>
>> Special rotated page view is constructed at the VMA creation time by
>> borrowing the DMA addresses from obj->pages.
>>
>> v2:
>> * Do not bother with pages for rotated sg list, just populate the DMA
>> addresses. (Daniel Vetter)
>> * Checkpatch cleanup.
>>
>> v3:
>> * Rebased on top of new plane handling (create rotated mapping when
>> setting the rotation property).
>> * Unpin rotated VMA on unpinning from display plane.
>> * Simplify rotation check using bitwise AND. (Chris Wilson)
>>
>> v4:
>> * Fix unpinning of optional rotated mapping so it is really considered
>> to be optional.
>>
>> v5:
>> * Rebased for fb modifier changes.
>> * Rebased for atomic commit.
>> * Only pin needed view for display. (Ville Syrjälä, Daniel Vetter)
>>
>> For: VIZ-4726
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Reviewed-by: Michel Thierry <michel.thierry at intel.com> (v4)
>
> Bunch of nitpicks below. Also I think it'd be good to split this patch
> into the rote refactoring work to add view parameters all over the place
> and the actual implementation.
Ok will split it. Rest below...
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
>> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
>> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
>> drivers/gpu/drm/i915/intel_drv.h | 4 +
>> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
>> 8 files changed, 263 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e07a1cb..79d3f2c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>> int __must_check
>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> u32 alignment,
>> - struct intel_engine_cs *pipelined);
>> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>> + struct intel_engine_cs *pipelined,
>> + const struct i915_ggtt_view *view);
>> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>> + const struct i915_ggtt_view *view);
>> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>> int align);
>> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
>> @@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>> &i915_ggtt_view_normal);
>> }
>>
>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view);
>> +static inline
>> +struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>> +{
>> + return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
>> +}
>> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
>> struct i915_vma *vma;
>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>> @@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>> alignment, flags | PIN_GLOBAL);
>> }
>>
>> +static inline int __must_check
>> +i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
>> + uint32_t alignment,
>> + unsigned flags,
>> + const struct i915_ggtt_view *ggtt_view)
>> +{
>> + return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
>> + alignment, flags | PIN_GLOBAL,
>> + ggtt_view);
>> +}
>> +
>> static inline int
>> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>> {
>> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
>> }
>>
>> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view);
>> +static inline void
>> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>> +{
>> + i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
>> +}
>>
>> /* i915_gem_context.c */
>> int __must_check i915_gem_context_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 0107c2a..04c0cb1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>> int
>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> u32 alignment,
>> - struct intel_engine_cs *pipelined)
>> + struct intel_engine_cs *pipelined,
>> + const struct i915_ggtt_view *view)
>> {
>> u32 old_read_domains, old_write_domain;
>> bool was_pin_display;
>> @@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>> * (e.g. libkms for the bootup splash), we have to ensure that we
>> * always use map_and_fenceable for all scanout buffers.
>> */
>> - ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
>> + ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
>> + view->type == I915_GGTT_VIEW_NORMAL ?
>> + PIN_MAPPABLE : 0, view);
>> if (ret)
>> goto err_unpin_display;
>>
>> @@ -4002,9 +4005,11 @@ err_unpin_display:
>> }
>>
>> void
>> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
>> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>> + const struct i915_ggtt_view *view)
>> {
>> - i915_gem_object_ggtt_unpin(obj);
>> + i915_gem_object_ggtt_unpin_view(obj, view->type);
>> +
>> obj->pin_display = is_pin_display(obj);
>> }
>>
>> @@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>> }
>>
>> void
>> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view)
>> {
>> - struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
>> + struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>>
>> BUG_ON(!vma);
>> BUG_ON(vma->pin_count == 0);
>> - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
>> + BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
>>
>> - if (--vma->pin_count == 0)
>> + if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
>> obj->pin_mappable = false;
>> }
>>
>> @@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>> return NOTIFY_DONE;
>> }
>>
>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>> + enum i915_ggtt_view_type view)
>> {
>> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>> struct i915_vma *vma;
>>
>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>> if (vma->vm == ggtt &&
>> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>> + vma->ggtt_view.type == view)
>> return vma;
>>
>> return NULL;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index bd95776..9336142 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
>> static inline
>> int i915_get_vma_pages(struct i915_vma *vma)
>> {
>> + int ret = 0;
>> +
>> if (vma->ggtt_view.pages)
>> return 0;
>>
>> if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>> vma->ggtt_view.pages = vma->obj->pages;
>> + else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
>> + vma->ggtt_view.pages =
>> + intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
>
> There's a bit a layering confusion going on, ggtt view code should be here
> in the gem code with an appropriate prefix. Just moving the code is all
> that's imo needed.
You would move intel_rotate_fb_obj_pages implementation into
i915_gem_gtt.c? Interesting, because I saw it differently - as a display
internal _implementation_ using the GEM GGTT view framework - and as
such does not belong in core GEM. Could you explain why you think so?
>> else
>> WARN_ONCE(1, "GGTT view %u not implemented!\n",
>> vma->ggtt_view.type);
>> @@ -2387,10 +2392,15 @@ int i915_get_vma_pages(struct i915_vma *vma)
>> if (!vma->ggtt_view.pages) {
>> DRM_ERROR("Failed to get pages for VMA view type %u!\n",
>> vma->ggtt_view.type);
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + } else if (IS_ERR(vma->ggtt_view.pages)) {
>> + ret = PTR_ERR(vma->ggtt_view.pages);
>> + vma->ggtt_view.pages = NULL;
>> + DRM_ERROR("Failed to get pages for VMA view type %u (%d)!\n",
>> + vma->ggtt_view.type, ret);
>> }
>>
>> - return 0;
>> + return ret;
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index c9e93f5..56a2356 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -111,12 +111,24 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>>
>> enum i915_ggtt_view_type {
>> I915_GGTT_VIEW_NORMAL = 0,
>> + I915_GGTT_VIEW_ROTATED
>> +};
>> +
>> +struct intel_rotation_info {
>> + unsigned int pixel_size;
>> + unsigned int height;
>> + unsigned int pitch;
>> + uint64_t fb_modifier;
>> };
>>
>> struct i915_ggtt_view {
>> enum i915_ggtt_view_type type;
>>
>> struct sg_table *pages;
>> +
>> + union {
>> + struct intel_rotation_info rotation_info;
>> + };
>> };
>>
>> extern const struct i915_ggtt_view i915_ggtt_view_normal;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 7a5d0a7..cb4dae8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2244,18 +2244,151 @@ intel_fb_align_height(struct drm_device *dev, int height, uint32_t pixel_format,
>> fb_format_modifier));
>> }
>>
>> -int
>> -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>> - struct drm_framebuffer *fb,
>> - struct intel_engine_cs *pipelined)
>> +static
>> +void rotate_pages(dma_addr_t *in, unsigned int width, unsigned int height,
>> + struct sg_table *st)
>> +{
>> + unsigned int column, row;
>> + unsigned int src_idx;
>> + struct scatterlist *sg = st->sgl;
>> +
>> + st->nents = 0;
>> +
>> + for (column = 0; column < width; column++) {
>> + src_idx = width * (height - 1) + column;
>> + for (row = 0; row < height; row++) {
>> + st->nents++;
>> + /* We don't need the pages, but need to initialize
>> + * the entries so the sg list can be happily traversed.
>> + * The only thing we need are DMA addresses.
>> + */
>> + sg_set_page(sg, NULL, PAGE_SIZE, 0);
>> + sg_dma_address(sg) = in[src_idx];
>> + sg_dma_len(sg) = PAGE_SIZE;
>> + sg = sg_next(sg);
>> + src_idx -= width;
>> + }
>> + }
>> +}
>> +
>> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
>> + struct drm_i915_gem_object *obj)
>> {
>> - struct drm_device *dev = fb->dev;
>> + struct drm_device *dev = obj->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> - u32 alignment;
>> - int ret;
>> + struct i915_address_space *ggtt = &dev_priv->gtt.base;
>> + struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
>> + unsigned long size, pages, rot_pages;
>> + struct sg_page_iter sg_iter;
>> + unsigned long i;
>> + dma_addr_t *page_addr_list;
>> + struct sg_table *st;
>> + unsigned int tile_width, tile_height;
>> + unsigned int width_pages, height_pages;
>> + int ret = ENOMEM;
>>
>> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> + pages = i915_gem_obj_size(obj, ggtt) / PAGE_SIZE;
>> +
>> + /* Calculate tiling geometry. */
>> + tile_height = intel_tile_height(dev, rot_info->pixel_size * 8,
>> + rot_info->fb_modifier);
>> + tile_width = PAGE_SIZE / rot_info->pixel_size / tile_height;
>> + width_pages = DIV_ROUND_UP(rot_info->pitch / rot_info->pixel_size,
>> + tile_width);
>> + height_pages = DIV_ROUND_UP(rot_info->height, tile_height);
>> + rot_pages = width_pages * height_pages;
>> + size = rot_pages * PAGE_SIZE;
>> +
>> + /* Allocate a temporary list of source pages for random access. */
>> + page_addr_list = drm_malloc_ab(pages, sizeof(dma_addr_t));
>> + if (!page_addr_list)
>> + return ERR_PTR(ret);
>> +
>> + /* Allocate target SG list. */
>> + st = kmalloc(sizeof(*st), GFP_KERNEL);
>> + if (!st)
>> + goto err_st_alloc;
>> +
>> + ret = sg_alloc_table(st, rot_pages, GFP_KERNEL);
>> + if (ret)
>> + goto err_sg_alloc;
>> +
>> + /* Populate source page list from the object. */
>> + i = 0;
>> + for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
>> + page_addr_list[i] = sg_page_iter_dma_address(&sg_iter);
>> + i++;
>> + }
>> +
>> + /* Rotate the pages. */
>> + rotate_pages(page_addr_list, width_pages, height_pages, st);
>> +
>> + DRM_DEBUG_KMS(
>> + "Created rotated page mapping for object size %lu (pitch=%u, height=%u, pixel_size=%u, %ux%u tiles, %lu pages).\n",
>> + size, rot_info->pitch, rot_info->height,
>> + rot_info->pixel_size, width_pages, height_pages,
>> + rot_pages);
>> +
>> + drm_free_large(page_addr_list);
>> +
>> + return st;
>> +
>> +err_sg_alloc:
>> + kfree(st);
>> +err_st_alloc:
>> + drm_free_large(page_addr_list);
>> +
>> + DRM_DEBUG_KMS(
>> + "Failed to create rotated mapping for object size %lu! (%d) (pitch=%u, height=%u, pixel_size=%u, %ux%u tiles, %lu pages)\n",
>> + size, ret, rot_info->pitch, rot_info->height,
>> + rot_info->pixel_size, width_pages, height_pages,
>> + rot_pages);
>> + return ERR_PTR(ret);
>> +}
>> +
>> +static
>> +const struct i915_ggtt_view i915_rotated_ggtt_view_template = {
>> + .type = I915_GGTT_VIEW_ROTATED,
>> +};
>
> Not really needed, you can just use a struct assignment instaed of the
> memcpy below, i.e.
>
> *view = {.type = I915_GGTT_VIEW_ROTATED};
>
> gcc then clears everyhing else.
Will change.
>> +
>> +static
>> +int intel_fb_ggtt_view(struct drm_plane_state *plane_state,
>> + struct drm_framebuffer *fb,
>> + struct i915_ggtt_view *view)
>
> I think intel_fill_fb_ggtt_view would be a clearer name for what this
> does. Also generally we put the struct a function operations on first (to
> fake OOP principles) and group later parameters by topic and then object
> first and flags/data later. So here I'd go with (view, fb, plane_state).
Makes sense, I just mindlessly expanded it in this version without
taking a step back to re-evalute the bigger picture.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list