Re: [PATCH 3/5] drm/i915/skl: Support secondary (rotated) frame buffer mapping

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx