Re: [PATCH v3] drm/i915: Compare GGTT view structs instead of types

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

Hi,

On 03/27/2015 10:03 AM, Joonas Lahtinen wrote:
> To allow for views where the view type is not defined by the view type only,
> like it is in stereo or rotated 90 degree view, change the semantic to require
> the whole view structure for comparison when we match a GGTT view.
>
> This allows including parameters like offset to be included in the view which
> is useful for eg. partial views.
>
> v3:
> - Rely on ggtt_view type being 0 for non-GGTT vma's, which equals to
>    I915_GGTT_VIEW_NORMAL. (Daniel Vetter)
> - Do not use potentially slower comparison when we only want to know if
>    something is or is not a normal view.
> - Rebase on top of rotated view patches. Add rotated view singleton.
> - If one view is missing in comparison they're equal only if both are missing.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> (v2)
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  8 ++++----
>   drivers/gpu/drm/i915/i915_gem.c      | 13 +++++++------
>   drivers/gpu/drm/i915/i915_gem_gtt.c  |  3 +++
>   drivers/gpu/drm/i915/i915_gem_gtt.h  |  8 ++++++++
>   drivers/gpu/drm/i915/intel_display.c |  8 +++-----
>   5 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 256369f..5aeba60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2794,19 +2794,19 @@ void i915_gem_restore_fences(struct drm_device *dev);
>
>   unsigned long
>   i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> -			      enum i915_ggtt_view_type view);
> +			      const struct i915_ggtt_view *view);
>   unsigned long
>   i915_gem_obj_offset(struct drm_i915_gem_object *o,
>   		    struct i915_address_space *vm);
>   static inline unsigned long
>   i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
>   {
> -	return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
> +	return i915_gem_obj_ggtt_offset_view(o, &i915_ggtt_view_normal);
>   }
>
>   bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
>   bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> -				  enum i915_ggtt_view_type view);
> +				  const struct i915_ggtt_view *view);
>   bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
>   			struct i915_address_space *vm);
>
> @@ -2854,7 +2854,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>
>   static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
>   {
> -	return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
> +	return i915_gem_obj_ggtt_bound_view(obj, &i915_ggtt_view_normal);
>   }
>
>   static inline unsigned long
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3300963..8c32b1d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4131,7 +4131,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>
>   		if (i915_vma_misplaced(vma, alignment, flags)) {
>   			unsigned long offset;
> -			offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> +			offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view) :
>   					     i915_gem_obj_offset(obj, vm);
>   			WARN(vma->pin_count,
>   			     "bo is already pinned in %s with incorrect alignment:"
> @@ -4230,7 +4230,7 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>
>   	BUG_ON(!vma);
>   	WARN_ON(vma->pin_count == 0);
> -	WARN_ON(!i915_gem_obj_ggtt_bound_view(obj, view->type));
> +	WARN_ON(!i915_gem_obj_ggtt_bound_view(obj, view));
>
>   	if (--vma->pin_count == 0 && view->type == I915_GGTT_VIEW_NORMAL)
>   		obj->pin_mappable = false;

No comparison helper in i915_gem_obj_to_ggtt_view()?

> @@ -5109,13 +5109,14 @@ i915_gem_obj_offset(struct drm_i915_gem_object *o,
>
>   unsigned long
>   i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> -			      enum i915_ggtt_view_type view)
> +			      const struct i915_ggtt_view *view)
>   {
>   	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
>   	struct i915_vma *vma;
>
>   	list_for_each_entry(vma, &o->vma_list, vma_link)
> -		if (vma->vm == ggtt && vma->ggtt_view.type == view)
> +		if (vma->vm == ggtt &&
> +		    i915_ggtt_view_equal(&vma->ggtt_view, view))
>   			return vma->node.start;
>
>   	WARN(1, "global vma for this object not found.\n");
> @@ -5139,14 +5140,14 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
>   }
>
>   bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> -				  enum i915_ggtt_view_type view)
> +				  const struct i915_ggtt_view *view)
>   {
>   	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
>   	struct i915_vma *vma;
>
>   	list_for_each_entry(vma, &o->vma_list, vma_link)
>   		if (vma->vm == ggtt &&
> -		    vma->ggtt_view.type == view &&
> +		    i915_ggtt_view_equal(&vma->ggtt_view, view) &&
>   		    drm_mm_node_allocated(&vma->node))
>   			return true;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9903bb0..13eb769 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,9 @@
>    */
>
>   const struct i915_ggtt_view i915_ggtt_view_normal;
> +const struct i915_ggtt_view i915_ggtt_view_rotated = {
> +        .type = I915_GGTT_VIEW_ROTATED
> +};
>
>   static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
>   static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 0dad426..c1c426a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -138,6 +138,7 @@ struct i915_ggtt_view {
>   };
>
>   extern const struct i915_ggtt_view i915_ggtt_view_normal;
> +extern const struct i915_ggtt_view i915_ggtt_view_rotated;
>
>   enum i915_cache_level;
>
> @@ -422,4 +423,11 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
>   int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
>   void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
>
> +static inline bool
> +i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> +                     const struct i915_ggtt_view *b)
> +{
> +	return !!a && !!b ? a->type == b->type : !a && !b;
> +}

What's the background behind this both NULL check? Will you ever use it 
like that?

I think the code elsewhere makes sure to always pass valid views to this 
internal helper so I don't get it.

> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cb50854..d6c2e34 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2293,8 +2293,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>   			const struct drm_plane_state *plane_state)
>   {
>   	struct intel_rotation_info *info = &view->rotation_info;
> -	static const struct i915_ggtt_view rotated_view =
> -				{ .type = I915_GGTT_VIEW_ROTATED };
>
>   	*view = i915_ggtt_view_normal;
>
> @@ -2304,7 +2302,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>   	if (!intel_rotation_90_or_270(plane_state->rotation))
>   		return 0;
>
> -	*view = rotated_view;
> +	*view = i915_ggtt_view_rotated;
>
>   	info->height = fb->height;
>   	info->pixel_format = fb->pixel_format;
> @@ -2904,10 +2902,10 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>   unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>   				     struct drm_i915_gem_object *obj)
>   {
> -	enum i915_ggtt_view_type view = I915_GGTT_VIEW_NORMAL;
> +	const struct i915_ggtt_view *view = &i915_ggtt_view_normal;
>
>   	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
> -		view = I915_GGTT_VIEW_ROTATED;
> +		view = &i915_ggtt_view_rotated;
>
>   	return i915_gem_obj_ggtt_offset_view(obj, view);
>   }
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx