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@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?
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
