[Intel-gfx] [PATCH v3] drm/i915: Compare GGTT view structs instead of types
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 27 03:41:17 PDT 2015
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 at ffwll.ch>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> (v2)
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen at 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
More information about the Intel-gfx
mailing list