On ti, 2015-03-17 at 15:45 +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 such 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)
>
> v6:
> * Rebased after preparatory work has been extracted out. (Daniel Vetter)
>
> v7:
> * Slightly simplified tiling geometry calculation.
> * Moved rotated GGTT view implementation into i915_gem_gtt.c (Daniel Vetter)
>
> v8:
> * Do not use i915_gem_obj_size to get object size since that actually
> returns the size of an VMA which may not exist.
> * Rebased for ggtt view changes.
>
> For: VIZ-4726
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx> (v4)
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 113 ++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++
> drivers/gpu/drm/i915/intel_display.c | 29 ++++++++-
> drivers/gpu/drm/i915/intel_drv.h | 4 ++
> 4 files changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f1b9ea6..5381ebf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2413,15 +2413,119 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
>
> }
>
> +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;
> + }
> + }
> +}
> +
> +static
> +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
> + struct drm_i915_gem_object *obj)
> +{
> + struct drm_device *dev = obj->base.dev;
> + 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_pitch, tile_height;
> + unsigned int width_pages, height_pages;
> + int ret = ENOMEM;
> +
> + pages = obj->base.size / PAGE_SIZE;
> +
> + /* Calculate tiling geometry. */
> + tile_height = intel_tile_height(dev, rot_info->pixel_format,
> + rot_info->fb_modifier);
> + tile_pitch = PAGE_SIZE / tile_height;
> + width_pages = DIV_ROUND_UP(rot_info->pitch, tile_pitch);
> + 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_format=0x%x, %ux%u tiles, %lu pages).\n",
> + size, rot_info->pitch, rot_info->height,
> + rot_info->pixel_format, 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_format=0x%x, %ux%u tiles, %lu pages)\n",
> + size, ret, rot_info->pitch, rot_info->height,
> + rot_info->pixel_format, width_pages, height_pages,
> + rot_pages);
> + return ERR_PTR(ret);
> +}
>
> static inline
> int i915_get_ggtt_vma_pages(struct i915_vma *vma)
Same rant about function signatures as on earlier patch, put all on the
same line like most of new the code has it.
> {
> + 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);
> else
> WARN_ONCE(1, "GGTT view %u not implemented!\n",
> vma->ggtt_view.type);
> @@ -2429,10 +2533,15 @@ int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> if (!vma->ggtt_view.pages) {
> DRM_ERROR("Failed to get pages for GGTT 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..a61e8dd 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 height;
> + unsigned int pitch;
> + uint32_t pixel_format;
> + 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;
> + };
In preparation for the memcmp way of comparing views, I would move this
be before the variable struct parts (namely sg_table *pages), and also
wrap it once more so the result would be like this:
[snip]
enum i915_ggtt_view_type type;
union {
struct {
struct intel_rotation_info info;
} rotated;
struct {
...
} partial;
};
// private bits go here, to be wrapped in their struct with view
// type comparing patches
struct sg_table *pages;
[snip]
That way it's clear which view owns what.
> };
>
> 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 fe11e99..c2c3a76 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
> return false;
> }
>
> -static unsigned int
> +unsigned int
> intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> uint64_t fb_format_modifier)
> {
> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
> struct drm_framebuffer *fb,
> const struct drm_plane_state *plane_state)
> {
> + struct intel_rotation_info *info = &view->rotation_info;
> + static const struct i915_ggtt_view rotated_view =
> + { .type = I915_GGTT_VIEW_ROTATED };
> +
> *view = i915_ggtt_view_normal;
>
> - return 0;
> + if (!plane_state)
> + return 0;
> +
> + if (!(plane_state->rotation &
> + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
> + return 0;
Should the rotation checking helper introduced in previous patch be used
here too?
> +
> + *view = rotated_view;
> +
> + info->height = fb->height;
> + info->pixel_format = fb->pixel_format;
> + info->pitch = fb->pitches[0];
> + info->fb_modifier = fb->modifier[0];
> +
> + if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> + info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> + DRM_DEBUG_KMS(
> + "Y or Yf tiling is needed for 90/270 rotation!\n");
> + return -EINVAL;
> + }
> +
> + return 1;
> }
>
> int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3721878..53a1372 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -987,6 +987,10 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
> struct drm_property *property,
> uint64_t val);
>
> +unsigned int
> +intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
> + uint64_t fb_format_modifier);
> +
> /* shared dpll functions */
> struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> void assert_shared_dpll(struct drm_i915_private *dev_priv,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
