| Message ID | 1425307432-4955-4-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 02, 2015 at 02:43:50PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > Reviewed-by: Michel Thierry <michel.thierry@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. Cheers, Daniel > --- > 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. > 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. > + > +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). > +{ > + struct intel_rotation_info *info = &view->rotation_info; > + > + memcpy(view, &i915_ggtt_view_normal, sizeof(*view)); > + > + if (!plane_state) > + return 0; > + > + if (plane_state->rotation & > + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) == 0) > + return 0; > + > + memcpy(view, &i915_rotated_ggtt_view_template, sizeof(*view)); > + > + info->height = fb->height; > + info->pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > + 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; > +} > + > +static > +u32 intel_display_obj_alignment(struct drm_device *dev, > + struct drm_framebuffer *fb) > +{ > + int alignment; > > switch (fb->modifier[0]) { > case DRM_FORMAT_MOD_NONE: > @@ -2296,6 +2429,30 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > if (need_vtd_wa(dev) && alignment < 256 * 1024) > alignment = 256 * 1024; > > + return alignment; > +} > + > +int > +intel_pin_and_fence_fb_obj(struct drm_plane *plane, > + struct drm_framebuffer *fb, > + struct drm_plane_state *plane_state, > + struct intel_engine_cs *pipelined) > +{ > + struct drm_device *dev = fb->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct i915_ggtt_view view; > + u32 alignment; > + int ret; > + > + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > + > + alignment = intel_display_obj_alignment(dev, fb); > + > + ret = intel_fb_ggtt_view(plane_state, fb, &view); > + if (ret < 0) > + return ret; > + > /* > * Global gtt pte registers are special registers which actually forward > * writes to a chunk of system memory. Which means that there is no risk > @@ -2306,7 +2463,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > intel_runtime_pm_get(dev_priv); > > dev_priv->mm.interruptible = false; > - ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined); > + ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined, > + &view); > if (ret) > goto err_interruptible; > > @@ -2326,19 +2484,27 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, > return 0; > > err_unpin: > - i915_gem_object_unpin_from_display_plane(obj); > + i915_gem_object_unpin_from_display_plane(obj, &view); > err_interruptible: > dev_priv->mm.interruptible = true; > intel_runtime_pm_put(dev_priv); > return ret; > } > > -static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj) > +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, > + struct drm_plane_state *plane_state) > { > + struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct i915_ggtt_view view; > + int ret; > + > WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); > > + ret = intel_fb_ggtt_view(plane_state, fb, &view); > + WARN_ONCE(ret < 0, "Invalid fb format and rotation combination!"); > + > i915_gem_object_unpin_fence(obj); > - i915_gem_object_unpin_from_display_plane(obj); > + i915_gem_object_unpin_from_display_plane(obj, &view); > } > > /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel > @@ -9223,7 +9389,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) > enum pipe pipe = to_intel_crtc(work->crtc)->pipe; > > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(intel_fb_obj(work->old_fb)); > + intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state); > drm_gem_object_unreference(&work->pending_flip_obj->base); > drm_framebuffer_unreference(work->old_fb); > > @@ -9931,7 +10097,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > ring = &dev_priv->ring[RCS]; > } > > - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring); > + ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, > + crtc->primary->state, ring); > if (ret) > goto cleanup_pending; > > @@ -9971,7 +10138,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > return 0; > > cleanup_unpin: > - intel_unpin_fb_obj(obj); > + intel_unpin_fb_obj(fb, crtc->primary->state); > cleanup_pending: > atomic_dec(&intel_crtc->unpin_work_count); > crtc->primary->fb = old_fb; > @@ -11933,7 +12100,7 @@ intel_prepare_plane_fb(struct drm_plane *plane, > if (ret) > DRM_DEBUG_KMS("failed to attach phys object\n"); > } else { > - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > + ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL); > } > > if (ret == 0) > @@ -11965,7 +12132,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane, > if (plane->type != DRM_PLANE_TYPE_CURSOR || > !INTEL_INFO(dev)->cursor_needs_physical) { > mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(obj); > + intel_unpin_fb_obj(fb, old_state); > mutex_unlock(&dev->struct_mutex); > } > } > @@ -13862,6 +14029,7 @@ void intel_modeset_gem_init(struct drm_device *dev) > > if (intel_pin_and_fence_fb_obj(c->primary, > c->primary->fb, > + c->primary->state, > NULL)) { > DRM_ERROR("failed to pin boot fb on pipe %d\n", > to_intel_crtc(c)->pipe); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c466d43..9c2145d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -955,6 +955,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > struct intel_load_detect_pipe *old); > int intel_pin_and_fence_fb_obj(struct drm_plane *plane, > struct drm_framebuffer *fb, > + struct drm_plane_state *plane_state, > struct intel_engine_cs *pipelined); > struct drm_framebuffer * > __intel_framebuffer_create(struct drm_device *dev, > @@ -979,6 +980,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane, > struct drm_property *property, > uint64_t val); > > +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, > + struct drm_i915_gem_object *obj); > + > /* 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, > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 234a699..d8204ae 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -126,7 +126,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > } > > /* Flush everything out, we'll be doing GTT only from now on */ > - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL); > + ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL); > if (ret) { > DRM_ERROR("failed to pin obj: %d\n", ret); > goto out_fb; > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c > index 823d1d9..dd92122 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay, > if (ret != 0) > return ret; > > - ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL); > + ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, > + &i915_ggtt_view_normal); > if (ret != 0) > return ret; > > -- > 2.3.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@intel.com> >> Reviewed-by: Michel Thierry <michel.thierry@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
On Tue, Mar 03, 2015 at 09:59:31AM +0000, Tvrtko Ursulin wrote: > > 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@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@intel.com> > >>Reviewed-by: Michel Thierry <michel.thierry@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? Mostly so that code in the same layer is in the same source files. We have display code in intel_display which calls into the gem memory management code exclusively through intel_pin_and_fence_fb_obj. Manging of vmas and ptes and all that is gem territory and hence all the different view imo should be there. We might want to eventually extract an i915_gem_vma.c file and put all that stuff in there. Imo the benefit is that if you need to make changes to a view they're all in the same spot and adding another one you have all the examples locally. There's nothing wrong with your approach, it's just how we organize functions into files for i915. Maybe a clearer example would be basic gen enabling. Atm it's all spread out over the different layers and functional parts of the driver, but we could very well extract things and do files per-gen (or maybe per hw block since they don't change in lockstep everywhere). radeon is more organized like that as an example. But for i915 we have all the gtt code in i915_gem_gtt.c, all the reset code in i915_gpu_error.c, all the legacy ringuffer stuff in intel_ringbuffer.c. And hence I also think we should put all the vma and view related things together too. I hope this explains the idea. Cheers, Daniel
On 03/04/2015 02:43 PM, Daniel Vetter wrote: > On Tue, Mar 03, 2015 at 09:59:31AM +0000, Tvrtko Ursulin wrote: >> >> 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@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@intel.com> >>>> Reviewed-by: Michel Thierry <michel.thierry@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? > > Mostly so that code in the same layer is in the same source files. We have > display code in intel_display which calls into the gem memory management > code exclusively through intel_pin_and_fence_fb_obj. > > Manging of vmas and ptes and all that is gem territory and hence all the > different view imo should be there. We might want to eventually extract an > i915_gem_vma.c file and put all that stuff in there. Imo the benefit is > that if you need to make changes to a view they're all in the same spot > and adding another one you have all the examples locally. > > There's nothing wrong with your approach, it's just how we organize > functions into files for i915. Maybe a clearer example would be basic gen > enabling. Atm it's all spread out over the different layers and functional > parts of the driver, but we could very well extract things and do files > per-gen (or maybe per hw block since they don't change in lockstep > everywhere). radeon is more organized like that as an example. But for > i915 we have all the gtt code in i915_gem_gtt.c, all the reset code in > i915_gpu_error.c, all the legacy ringuffer stuff in intel_ringbuffer.c. > > And hence I also think we should put all the vma and view related things > together too. What about creating something like intel_display_rotation.c then to meet half-way? :) Regards, Tvrtko
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); 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, +}; + +static +int intel_fb_ggtt_view(struct drm_plane_state *plane_state, + struct drm_framebuffer *fb, + struct i915_ggtt_view *view) +{ + struct intel_rotation_info *info = &view->rotation_info; + + memcpy(view, &i915_ggtt_view_normal, sizeof(*view)); + + if (!plane_state) + return 0; + + if (plane_state->rotation & + (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) == 0) + return 0; + + memcpy(view, &i915_rotated_ggtt_view_template, sizeof(*view)); + + info->height = fb->height; + info->pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); + 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; +} + +static +u32 intel_display_obj_alignment(struct drm_device *dev, + struct drm_framebuffer *fb) +{ + int alignment; switch (fb->modifier[0]) { case DRM_FORMAT_MOD_NONE: @@ -2296,6 +2429,30 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, if (need_vtd_wa(dev) && alignment < 256 * 1024) alignment = 256 * 1024; + return alignment; +} + +int +intel_pin_and_fence_fb_obj(struct drm_plane *plane, + struct drm_framebuffer *fb, + struct drm_plane_state *plane_state, + struct intel_engine_cs *pipelined) +{ + struct drm_device *dev = fb->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct i915_ggtt_view view; + u32 alignment; + int ret; + + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + + alignment = intel_display_obj_alignment(dev, fb); + + ret = intel_fb_ggtt_view(plane_state, fb, &view); + if (ret < 0) + return ret; + /* * Global gtt pte registers are special registers which actually forward * writes to a chunk of system memory. Which means that there is no risk @@ -2306,7 +2463,8 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, intel_runtime_pm_get(dev_priv); dev_priv->mm.interruptible = false; - ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined); + ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined, + &view); if (ret) goto err_interruptible; @@ -2326,19 +2484,27 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, return 0; err_unpin: - i915_gem_object_unpin_from_display_plane(obj); + i915_gem_object_unpin_from_display_plane(obj, &view); err_interruptible: dev_priv->mm.interruptible = true; intel_runtime_pm_put(dev_priv); return ret; } -static void intel_unpin_fb_obj(struct drm_i915_gem_object *obj) +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, + struct drm_plane_state *plane_state) { + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct i915_ggtt_view view; + int ret; + WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex)); + ret = intel_fb_ggtt_view(plane_state, fb, &view); + WARN_ONCE(ret < 0, "Invalid fb format and rotation combination!"); + i915_gem_object_unpin_fence(obj); - i915_gem_object_unpin_from_display_plane(obj); + i915_gem_object_unpin_from_display_plane(obj, &view); } /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel @@ -9223,7 +9389,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) enum pipe pipe = to_intel_crtc(work->crtc)->pipe; mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(intel_fb_obj(work->old_fb)); + intel_unpin_fb_obj(work->old_fb, work->crtc->primary->state); drm_gem_object_unreference(&work->pending_flip_obj->base); drm_framebuffer_unreference(work->old_fb); @@ -9931,7 +10097,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ring = &dev_priv->ring[RCS]; } - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring); + ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, + crtc->primary->state, ring); if (ret) goto cleanup_pending; @@ -9971,7 +10138,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, return 0; cleanup_unpin: - intel_unpin_fb_obj(obj); + intel_unpin_fb_obj(fb, crtc->primary->state); cleanup_pending: atomic_dec(&intel_crtc->unpin_work_count); crtc->primary->fb = old_fb; @@ -11933,7 +12100,7 @@ intel_prepare_plane_fb(struct drm_plane *plane, if (ret) DRM_DEBUG_KMS("failed to attach phys object\n"); } else { - ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); + ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL); } if (ret == 0) @@ -11965,7 +12132,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane, if (plane->type != DRM_PLANE_TYPE_CURSOR || !INTEL_INFO(dev)->cursor_needs_physical) { mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(obj); + intel_unpin_fb_obj(fb, old_state); mutex_unlock(&dev->struct_mutex); } } @@ -13862,6 +14029,7 @@ void intel_modeset_gem_init(struct drm_device *dev) if (intel_pin_and_fence_fb_obj(c->primary, c->primary->fb, + c->primary->state, NULL)) { DRM_ERROR("failed to pin boot fb on pipe %d\n", to_intel_crtc(c)->pipe); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c466d43..9c2145d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -955,6 +955,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, struct intel_load_detect_pipe *old); int intel_pin_and_fence_fb_obj(struct drm_plane *plane, struct drm_framebuffer *fb, + struct drm_plane_state *plane_state, struct intel_engine_cs *pipelined); struct drm_framebuffer * __intel_framebuffer_create(struct drm_device *dev, @@ -979,6 +980,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane, struct drm_property *property, uint64_t val); +struct sg_table *intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, + struct drm_i915_gem_object *obj); + /* 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, diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 234a699..d8204ae 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -126,7 +126,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, } /* Flush everything out, we'll be doing GTT only from now on */ - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL); + ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL); if (ret) { DRM_ERROR("failed to pin obj: %d\n", ret); goto out_fb; diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c index 823d1d9..dd92122 100644 --- a/drivers/gpu/drm/i915/intel_overlay.c +++ b/drivers/gpu/drm/i915/intel_overlay.c @@ -720,7 +720,8 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay, if (ret != 0) return ret; - ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL); + ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, + &i915_ggtt_view_normal); if (ret != 0) return ret;