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@xxxxxxxxx> 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@xxxxxxxxx> Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx> (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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
