Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
On Tue, Mar 17, 2015 at 02:19:18PM +, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> >GGTT views are only applicable when dealing with GGTT. Change the code to
> >reject ggtt_view where it should not be used and require it when it should
> >be.
> >
> >v2:
> >- Dropped _ppgtt_ infixes, allow both types to be passed
> >- Disregard other but normal views when no view is specified
> >- More checks that valid parameters are passed
> >- More readable error checking
> >
> >v3:
> >- Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> [snip]
>
> >+unsigned long
> >+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> >+ enum i915_ggtt_view_type view)
> > {
> >+struct drm_i915_private *dev_priv = o->base.dev->dev_private;
>
> Unused variable slipped through, oops! :)
Indeed, fixed up.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
Hi,
On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
GGTT views are only applicable when dealing with GGTT. Change the code to
reject ggtt_view where it should not be used and require it when it should
be.
v2:
- Dropped _ppgtt_ infixes, allow both types to be passed
- Disregard other but normal views when no view is specified
- More checks that valid parameters are passed
- More readable error checking
v3:
- Prefer WARN_ONCE over BUG_ON when there is code path for failure
[snip]
+unsigned long
+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
+ enum i915_ggtt_view_type view)
{
+ struct drm_i915_private *dev_priv = o->base.dev->dev_private;
Unused variable slipped through, oops! :)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
On ti, 2015-03-17 at 07:31 +0200, Joonas Lahtinen wrote: > On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote: > > On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote: > > [snip] > > > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level > > > cache_level, > > > u32 flags); > > > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct > > > drm_device *dev, > > > > > > void i915_gem_restore_fences(struct drm_device *dev); > > > > > > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, > > > -struct i915_address_space *vm, > > > -enum i915_ggtt_view_type view); > > > -static inline > > > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, > > > - struct i915_address_space *vm) > > > +static inline bool i915_is_ggtt(struct i915_address_space *vm); Indeed it is. Thanks! Regards, Joonas PS. Sorry for spam, the enter key is so close to the space bar. > > > > This forward decl seems unneeded leftover from earlier patch iterations. > > I've removed it and merged the patch. > > > > Thanks, Daniel > > > > [snip] - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote:
> On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> [snip]
> > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > u32 flags);
> > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct
> > drm_device *dev,
> >
> > void i915_gem_restore_fences(struct drm_device *dev);
> >
> > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm,
> > - enum i915_ggtt_view_type view);
> > -static inline
> > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm)
> > +static inline bool i915_is_ggtt(struct i915_address_space *vm);
>
> This forward decl seems unneeded leftover from earlier patch iterations.
> I've removed it and merged the patch.
>
> Thanks, Daniel
>
> > +
> > +unsigned long
> > +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type 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_offset_view(o, vm, 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_bound_view(struct drm_i915_gem_object *o,
> > -struct i915_address_space *vm,
> > -enum i915_ggtt_view_type view);
> > -static inline
> > +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type view);
> > bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> > -}
> > + struct i915_address_space *vm);
> >
> > unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> > struct i915_address_space *vm);
> > -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view);
> > -static inline
> > -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > -struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> > -}
> > -
> > struct i915_vma *
> > -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view);
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
> >
> > -static inline
> > struct i915_vma *
> > i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> > - &i915_ggtt_view_normal);
> > -}
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
> >
> > -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> > -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)
> > - if (vma->pin_count > 0)
> > - return true;
> > - return false;
> > +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);
> > }
> > +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
> >
> > /* Some GGTT VM helpers */
> > #define i915_obj_to_ggtt(obj) \
> > @@ -2876,13 +2860,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_bound(obj, i915_obj_to_ggtt(obj));
> > -}
> > -
> > -static inline unsigned long
> > -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> > -{
> > - return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> > + return
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> Signed-off-by: Joonas Lahtinen
> ---
> drivers/gpu/drm/i915/i915_drv.h | 102 +
> drivers/gpu/drm/i915/i915_gem.c | 175
>
> drivers/gpu/drm/i915/i915_gem_gtt.c | 71 +++
> 3 files changed, 229 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a76c6ee..996f6a1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2623,20 +2623,16 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
> #define PIN_GLOBAL 0x4
> #define PIN_OFFSET_BIAS 0x8
> #define PIN_OFFSET_MASK (~4095)
> -int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - uint32_t alignment,
> - uint64_t flags,
> - const struct i915_ggtt_view *view);
> -static inline
> -int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - uint32_t alignment,
> - uint64_t flags)
> -{
> - return i915_gem_object_pin_view(obj, vm, alignment, flags,
> - &i915_ggtt_view_normal);
> -}
> +int __must_check
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + uint32_t alignment,
> + uint64_t flags);
> +int __must_check
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view,
> + uint32_t alignment,
> + uint64_t flags);
>
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> u32 flags);
> @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct
> drm_device *dev,
>
> void i915_gem_restore_fences(struct drm_device *dev);
>
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> -struct i915_address_space *vm,
> -enum i915_ggtt_view_type view);
> -static inline
> -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm)
> +static inline bool i915_is_ggtt(struct i915_address_space *vm);
This forward decl seems unneeded leftover from earlier patch iterations.
I've removed it and merged the patch.
Thanks, Daniel
> +
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type 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_offset_view(o, vm, 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_bound_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view);
> -static inline
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view);
> bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm)
> -{
> - return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> -}
> + struct i915_address_space *vm);
>
> unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> struct i915_address_space *vm);
> -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view);
> -static inline
> -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm)
> -{
> - return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> -}
> -
> struct
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
On 03/16/2015 02:48 PM, Joonas Lahtinen wrote:
Hi,
Regression testing completed without problems for BYT, HSW and BDW
already.
On ma, 2015-03-16 at 13:26 +, Tvrtko Ursulin wrote:
Hi,
On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
GGTT views are only applicable when dealing with GGTT. Change the code to
reject ggtt_view where it should not be used and require it when it should
be.
v2:
- Dropped _ppgtt_ infixes, allow both types to be passed
- Disregard other but normal views when no view is specified
- More checks that valid parameters are passed
- More readable error checking
v3:
- Prefer WARN_ONCE over BUG_ON when there is code path for failure
[snip]
+i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm);
+struct i915_vma *
+i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view);
Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
vma in the name I think.
The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
would cause a lot of changes all around code to change), so I decided to
stay with the same convention. In that sense it would add more
confusion, compared to the current *_view function being the same as
without _view, but with explicitly specified view.
Yes, I overlooked that.
+struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+struct i915_address_space *vm)
{
struct i915_vma *vma;
- list_for_each_entry(vma, &obj->vma_list, vma_link)
- if (vma->vm == vm && vma->ggtt_view.type == view->type)
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
Since there are 4-5 instances of this check it may make sense to add a
helper like i915_is_normal_ggtt_view(vma), but it is not that important
for me.
This will be done in following patch that makes the view struct (minus
implementation parts like the pages sg_table) define the view.
Cool.
The rest looks good to me.
Sound like you could R-B this then?
Yes,
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5957 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 275/275 275/275 ILK 303/303 303/303 SNB -1 279/279 278/279 IVB 343/343 343/343 BYT 287/287 287/287 HSW -1 361/361 360/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *SNB igt_gem_persistent_relocs_forked-interruptible-thrash-inactive PASS(2) DMESG_WARN(1)PASS(1) HSW igt_gem_storedw_loop_bsd DMESG_WARN(1)PASS(1) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
Hi,
Regression testing completed without problems for BYT, HSW and BDW
already.
On ma, 2015-03-16 at 13:26 +, Tvrtko Ursulin wrote:
> Hi,
>
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> > GGTT views are only applicable when dealing with GGTT. Change the code to
> > reject ggtt_view where it should not be used and require it when it should
> > be.
> >
> > v2:
> > - Dropped _ppgtt_ infixes, allow both types to be passed
> > - Disregard other but normal views when no view is specified
> > - More checks that valid parameters are passed
> > - More readable error checking
> >
> > v3:
> > - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> [snip]
>
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
>
> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
> vma in the name I think.
>
The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
would cause a lot of changes all around code to change), so I decided to
stay with the same convention. In that sense it would add more
confusion, compared to the current *_view function being the same as
without _view, but with explicitly specified view.
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > +struct i915_address_space *vm)
> > {
> > struct i915_vma *vma;
> > - list_for_each_entry(vma, &obj->vma_list, vma_link)
> > - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> > + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
>
> Since there are 4-5 instances of this check it may make sense to add a
> helper like i915_is_normal_ggtt_view(vma), but it is not that important
> for me.
>
This will be done in following patch that makes the view struct (minus
implementation parts like the pages sg_table) define the view.
> The rest looks good to me.
>
Sound like you could R-B this then?
Best Regards,
Joonas
> Regards,
>
> Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
Hi,
On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
GGTT views are only applicable when dealing with GGTT. Change the code to
reject ggtt_view where it should not be used and require it when it should
be.
v2:
- Dropped _ppgtt_ infixes, allow both types to be passed
- Disregard other but normal views when no view is specified
- More checks that valid parameters are passed
- More readable error checking
v3:
- Prefer WARN_ONCE over BUG_ON when there is code path for failure
[snip]
+i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm);
+struct i915_vma *
+i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view);
Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
vma in the name I think.
+struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+struct i915_address_space *vm)
{
struct i915_vma *vma;
- list_for_each_entry(vma, &obj->vma_list, vma_link)
- if (vma->vm == vm && vma->ggtt_view.type == view->type)
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
Since there are 4-5 instances of this check it may make sense to add a
helper like i915_is_normal_ggtt_view(vma), but it is not that important
for me.
The rest looks good to me.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
