[Intel-gfx] [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Mar 4 07:04:46 PST 2015
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 at intel.com>
>>>>
>>>> 90/270 rotated scanout needs a rotated GTT view of the framebuffer.
>>>>
>>>> This is put in a separate VMA with a dedicated ggtt_view and wired suchs that
>>>> it is created when a framebuffer is pinned to a 90/270 rotated plane.
>>>>
>>>> Rotation is only possible with Yb/Yf buffers and error is propagated to
>>>> user space in case of a mismatch.
>>>>
>>>> Special rotated page view is constructed at the VMA creation time by
>>>> borrowing the DMA addresses from obj->pages.
>>>>
>>>> v2:
>>>> * Do not bother with pages for rotated sg list, just populate the DMA
>>>> addresses. (Daniel Vetter)
>>>> * Checkpatch cleanup.
>>>>
>>>> v3:
>>>> * Rebased on top of new plane handling (create rotated mapping when
>>>> setting the rotation property).
>>>> * Unpin rotated VMA on unpinning from display plane.
>>>> * Simplify rotation check using bitwise AND. (Chris Wilson)
>>>>
>>>> v4:
>>>> * Fix unpinning of optional rotated mapping so it is really considered
>>>> to be optional.
>>>>
>>>> v5:
>>>> * Rebased for fb modifier changes.
>>>> * Rebased for atomic commit.
>>>> * Only pin needed view for display. (Ville Syrjälä, Daniel Vetter)
>>>>
>>>> For: VIZ-4726
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Reviewed-by: Michel Thierry <michel.thierry at intel.com> (v4)
>>>
>>> Bunch of nitpicks below. Also I think it'd be good to split this patch
>>> into the rote refactoring work to add view parameters all over the place
>>> and the actual implementation.
>>
>> Ok will split it. Rest below...
>>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h | 33 +++++-
>>>> drivers/gpu/drm/i915/i915_gem.c | 27 +++--
>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++-
>>>> drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
>>>> drivers/gpu/drm/i915/intel_display.c | 204 +++++++++++++++++++++++++++++++----
>>>> drivers/gpu/drm/i915/intel_drv.h | 4 +
>>>> drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>>>> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
>>>> 8 files changed, 263 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index e07a1cb..79d3f2c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2743,8 +2743,10 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>>>> int __must_check
>>>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> u32 alignment,
>>>> - struct intel_engine_cs *pipelined);
>>>> -void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>>>> + struct intel_engine_cs *pipelined,
>>>> + const struct i915_ggtt_view *view);
>>>> +void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>>>> + const struct i915_ggtt_view *view);
>>>> int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>>>> int align);
>>>> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
>>>> @@ -2813,7 +2815,13 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>>>> &i915_ggtt_view_normal);
>>>> }
>>>>
>>>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>>>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view);
>>>> +static inline
>>>> +struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>>>> +{
>>>> + return i915_gem_obj_to_ggtt_view(obj, I915_GGTT_VIEW_NORMAL);
>>>> +}
>>>> static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
>>>> struct i915_vma *vma;
>>>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>> @@ -2867,13 +2875,30 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>>>> alignment, flags | PIN_GLOBAL);
>>>> }
>>>>
>>>> +static inline int __must_check
>>>> +i915_gem_obj_ggtt_pin_view(struct drm_i915_gem_object *obj,
>>>> + uint32_t alignment,
>>>> + unsigned flags,
>>>> + const struct i915_ggtt_view *ggtt_view)
>>>> +{
>>>> + return i915_gem_object_pin_view(obj, i915_obj_to_ggtt(obj),
>>>> + alignment, flags | PIN_GLOBAL,
>>>> + ggtt_view);
>>>> +}
>>>> +
>>>> static inline int
>>>> i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
>>>> {
>>>> return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
>>>> }
>>>>
>>>> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>>>> +void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view);
>>>> +static inline void
>>>> +i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>>>> +{
>>>> + i915_gem_object_ggtt_unpin_view(obj, I915_GGTT_VIEW_NORMAL);
>>>> +}
>>>>
>>>> /* i915_gem_context.c */
>>>> int __must_check i915_gem_context_init(struct drm_device *dev);
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 0107c2a..04c0cb1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -3938,7 +3938,8 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>>>> int
>>>> i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> u32 alignment,
>>>> - struct intel_engine_cs *pipelined)
>>>> + struct intel_engine_cs *pipelined,
>>>> + const struct i915_ggtt_view *view)
>>>> {
>>>> u32 old_read_domains, old_write_domain;
>>>> bool was_pin_display;
>>>> @@ -3974,7 +3975,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>>> * (e.g. libkms for the bootup splash), we have to ensure that we
>>>> * always use map_and_fenceable for all scanout buffers.
>>>> */
>>>> - ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
>>>> + ret = i915_gem_obj_ggtt_pin_view(obj, alignment,
>>>> + view->type == I915_GGTT_VIEW_NORMAL ?
>>>> + PIN_MAPPABLE : 0, view);
>>>> if (ret)
>>>> goto err_unpin_display;
>>>>
>>>> @@ -4002,9 +4005,11 @@ err_unpin_display:
>>>> }
>>>>
>>>> void
>>>> -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj)
>>>> +i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
>>>> + const struct i915_ggtt_view *view)
>>>> {
>>>> - i915_gem_object_ggtt_unpin(obj);
>>>> + i915_gem_object_ggtt_unpin_view(obj, view->type);
>>>> +
>>>> obj->pin_display = is_pin_display(obj);
>>>> }
>>>>
>>>> @@ -4241,15 +4246,16 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>>>> }
>>>>
>>>> void
>>>> -i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
>>>> +i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view)
>>>> {
>>>> - struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);
>>>> + struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
>>>>
>>>> BUG_ON(!vma);
>>>> BUG_ON(vma->pin_count == 0);
>>>> - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
>>>> + BUG_ON(!i915_gem_obj_bound_view(obj, i915_obj_to_ggtt(obj), view));
>>>>
>>>> - if (--vma->pin_count == 0)
>>>> + if (--vma->pin_count == 0 && view == I915_GGTT_VIEW_NORMAL)
>>>> obj->pin_mappable = false;
>>>> }
>>>>
>>>> @@ -5306,14 +5312,15 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
>>>> return NOTIFY_DONE;
>>>> }
>>>>
>>>> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>>>> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>>> + enum i915_ggtt_view_type view)
>>>> {
>>>> struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
>>>> struct i915_vma *vma;
>>>>
>>>> list_for_each_entry(vma, &obj->vma_list, vma_link)
>>>> if (vma->vm == ggtt &&
>>>> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>>> + vma->ggtt_view.type == view)
>>>> return vma;
>>>>
>>>> return NULL;
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> index bd95776..9336142 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> @@ -2375,11 +2375,16 @@ i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
>>>> static inline
>>>> int i915_get_vma_pages(struct i915_vma *vma)
>>>> {
>>>> + int ret = 0;
>>>> +
>>>> if (vma->ggtt_view.pages)
>>>> return 0;
>>>>
>>>> if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>>> vma->ggtt_view.pages = vma->obj->pages;
>>>> + else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
>>>> + vma->ggtt_view.pages =
>>>> + intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
>>>
>>> There's a bit a layering confusion going on, ggtt view code should be here
>>> in the gem code with an appropriate prefix. Just moving the code is all
>>> that's imo needed.
>>
>> You would move intel_rotate_fb_obj_pages implementation into i915_gem_gtt.c?
>> Interesting, because I saw it differently - as a display internal
>> _implementation_ using the GEM GGTT view framework - and as such does not
>> belong in core GEM. Could you explain why you think so?
>
> 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
More information about the Intel-gfx
mailing list