| Message ID | 20220921070945.27764-4-niranjana.vishwanathapura@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/vm_bind: Add VM_BIND functionality | expand |
On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: > Expose i915_gem_object_max_page_size() function non-static > which will be used by the vm_bind feature. > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index 33673fe7ee0a..3b3ab4abb0a3 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -11,14 +11,24 @@ > #include "pxp/intel_pxp.h" > > #include "i915_drv.h" > +#include "i915_gem_context.h" I can't spot that you are adding any code which would need this? I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. > #include "i915_gem_create.h" > #include "i915_trace.h" > #include "i915_user_extensions.h" > > -static u32 object_max_page_size(struct intel_memory_region **placements, > - unsigned int n_placements) > +/** > + * i915_gem_object_max_page_size() - max of min_page_size of the regions > + * @placements: list of regions > + * @n_placements: number of the placements > + * > + * Calculates the max of the min_page_size of a list of placements passed in. > + * > + * Return: max of the min_page_size > + */ > +u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, > + unsigned int n_placements) > { > - u32 max_page_size = 0; > + u32 max_page_size = I915_GTT_PAGE_SIZE_4K; > int i; > > for (i = 0; i < n_placements; i++) { > @@ -28,7 +38,6 @@ static u32 object_max_page_size(struct intel_memory_region **placements, > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > } > > - GEM_BUG_ON(!max_page_size); > return max_page_size; > } > > @@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct drm_i915_private *i915, u64 size, > > i915_gem_flush_free_objects(i915); > > - size = round_up(size, object_max_page_size(placements, n_placements)); > + size = round_up(size, i915_gem_object_max_page_size(placements, > + n_placements)); > if (size == 0) > return ERR_PTR(-EINVAL); Because of the changes above this path is now unreachable. I suppose it was meant to tell the user "you have supplied no placements"? But then GEM_BUG_ON (which you remove) used to be wrong. Regards, Tvrtko > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 7317d4102955..8c97bddad921 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size) > } > > void i915_gem_init__objects(struct drm_i915_private *i915); > +u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, > + unsigned int n_placements); > > void i915_objects_module_exit(void); > int i915_objects_module_init(void);
On Wed, Sep 21, 2022 at 10:13:12AM +0100, Tvrtko Ursulin wrote: > >On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: >>Expose i915_gem_object_max_page_size() function non-static >>which will be used by the vm_bind feature. >> >>Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> >>Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> >>--- >> drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- >> drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ >> 2 files changed, 17 insertions(+), 5 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>index 33673fe7ee0a..3b3ab4abb0a3 100644 >>--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>@@ -11,14 +11,24 @@ >> #include "pxp/intel_pxp.h" >> #include "i915_drv.h" >>+#include "i915_gem_context.h" > >I can't spot that you are adding any code which would need this? >I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. This include should have been added in a later patch for calling i915_gem_vm_lookup(). But got added here while patch refactoring. Will fix. > >> #include "i915_gem_create.h" >> #include "i915_trace.h" >> #include "i915_user_extensions.h" >>-static u32 object_max_page_size(struct intel_memory_region **placements, >>- unsigned int n_placements) >>+/** >>+ * i915_gem_object_max_page_size() - max of min_page_size of the regions >>+ * @placements: list of regions >>+ * @n_placements: number of the placements >>+ * >>+ * Calculates the max of the min_page_size of a list of placements passed in. >>+ * >>+ * Return: max of the min_page_size >>+ */ >>+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, >>+ unsigned int n_placements) >> { >>- u32 max_page_size = 0; >>+ u32 max_page_size = I915_GTT_PAGE_SIZE_4K; >> int i; >> for (i = 0; i < n_placements; i++) { >>@@ -28,7 +38,6 @@ static u32 object_max_page_size(struct intel_memory_region **placements, >> max_page_size = max_t(u32, max_page_size, mr->min_page_size); >> } >>- GEM_BUG_ON(!max_page_size); >> return max_page_size; >> } >>@@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct drm_i915_private *i915, u64 size, >> i915_gem_flush_free_objects(i915); >>- size = round_up(size, object_max_page_size(placements, n_placements)); >>+ size = round_up(size, i915_gem_object_max_page_size(placements, >>+ n_placements)); >> if (size == 0) >> return ERR_PTR(-EINVAL); > >Because of the changes above this path is now unreachable. I suppose >it was meant to tell the user "you have supplied no placements"? But >then GEM_BUG_ON (which you remove) used to be wrong. > Yah, looks like an existing problem. May be this "size == 0" check should have been made before we do the round_up()? ie., check input 'size' paramter is not 0? I think for now, I will remove this check as it was unreachable anyhow. Niranjana >Regards, > >Tvrtko > >>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>index 7317d4102955..8c97bddad921 100644 >>--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>@@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size) >> } >> void i915_gem_init__objects(struct drm_i915_private *i915); >>+u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, >>+ unsigned int n_placements); >> void i915_objects_module_exit(void); >> int i915_objects_module_init(void);
On 21/09/2022 19:00, Niranjana Vishwanathapura wrote: > On Wed, Sep 21, 2022 at 10:13:12AM +0100, Tvrtko Ursulin wrote: >> >> On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: >>> Expose i915_gem_object_max_page_size() function non-static >>> which will be used by the vm_bind feature. >>> >>> Signed-off-by: Niranjana Vishwanathapura >>> <niranjana.vishwanathapura@intel.com> >>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- >>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ >>> 2 files changed, 17 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> index 33673fe7ee0a..3b3ab4abb0a3 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> @@ -11,14 +11,24 @@ >>> #include "pxp/intel_pxp.h" >>> #include "i915_drv.h" >>> +#include "i915_gem_context.h" >> >> I can't spot that you are adding any code which would need this? >> I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. > > This include should have been added in a later patch for calling > i915_gem_vm_lookup(). But got added here while patch refactoring. > Will fix. > >> >>> #include "i915_gem_create.h" >>> #include "i915_trace.h" >>> #include "i915_user_extensions.h" >>> -static u32 object_max_page_size(struct intel_memory_region >>> **placements, >>> - unsigned int n_placements) >>> +/** >>> + * i915_gem_object_max_page_size() - max of min_page_size of the >>> regions >>> + * @placements: list of regions >>> + * @n_placements: number of the placements >>> + * >>> + * Calculates the max of the min_page_size of a list of placements >>> passed in. >>> + * >>> + * Return: max of the min_page_size >>> + */ >>> +u32 i915_gem_object_max_page_size(struct intel_memory_region >>> **placements, >>> + unsigned int n_placements) >>> { >>> - u32 max_page_size = 0; >>> + u32 max_page_size = I915_GTT_PAGE_SIZE_4K; >>> int i; >>> for (i = 0; i < n_placements; i++) { >>> @@ -28,7 +38,6 @@ static u32 object_max_page_size(struct >>> intel_memory_region **placements, >>> max_page_size = max_t(u32, max_page_size, mr->min_page_size); >>> } >>> - GEM_BUG_ON(!max_page_size); >>> return max_page_size; >>> } >>> @@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct >>> drm_i915_private *i915, u64 size, >>> i915_gem_flush_free_objects(i915); >>> - size = round_up(size, object_max_page_size(placements, >>> n_placements)); >>> + size = round_up(size, i915_gem_object_max_page_size(placements, >>> + n_placements)); >>> if (size == 0) >>> return ERR_PTR(-EINVAL); >> >> Because of the changes above this path is now unreachable. I suppose >> it was meant to tell the user "you have supplied no placements"? But >> then GEM_BUG_ON (which you remove) used to be wrong. >> > > Yah, looks like an existing problem. May be this "size == 0" check > should have been made before we do the round_up()? ie., check input 'size' > paramter is not 0? > I think for now, I will remove this check as it was unreachable anyhow. Hm that's true as well. i915_gem_create_ext_ioctl ensures at least one placement and internal callers do as well. To be safe, instead of removing maybe move to before "size = " and change to "if (GEM_WARN_ON(n_placements == 0))"? Not sure.. Matt any thoughts here given the changes in this patch? Regards, Tvrtko > > Niranjana > >> Regards, >> >> Tvrtko >> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> index 7317d4102955..8c97bddad921 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> @@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size) >>> } >>> void i915_gem_init__objects(struct drm_i915_private *i915); >>> +u32 i915_gem_object_max_page_size(struct intel_memory_region >>> **placements, >>> + unsigned int n_placements); >>> void i915_objects_module_exit(void); >>> int i915_objects_module_init(void);
On 22/09/2022 09:09, Tvrtko Ursulin wrote: > > On 21/09/2022 19:00, Niranjana Vishwanathapura wrote: >> On Wed, Sep 21, 2022 at 10:13:12AM +0100, Tvrtko Ursulin wrote: >>> >>> On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: >>>> Expose i915_gem_object_max_page_size() function non-static >>>> which will be used by the vm_bind feature. >>>> >>>> Signed-off-by: Niranjana Vishwanathapura >>>> <niranjana.vishwanathapura@intel.com> >>>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ >>>> 2 files changed, 17 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>> index 33673fe7ee0a..3b3ab4abb0a3 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>> @@ -11,14 +11,24 @@ >>>> #include "pxp/intel_pxp.h" >>>> #include "i915_drv.h" >>>> +#include "i915_gem_context.h" >>> >>> I can't spot that you are adding any code which would need this? >>> I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. >> >> This include should have been added in a later patch for calling >> i915_gem_vm_lookup(). But got added here while patch refactoring. >> Will fix. >> >>> >>>> #include "i915_gem_create.h" >>>> #include "i915_trace.h" >>>> #include "i915_user_extensions.h" >>>> -static u32 object_max_page_size(struct intel_memory_region >>>> **placements, >>>> - unsigned int n_placements) >>>> +/** >>>> + * i915_gem_object_max_page_size() - max of min_page_size of the >>>> regions >>>> + * @placements: list of regions >>>> + * @n_placements: number of the placements >>>> + * >>>> + * Calculates the max of the min_page_size of a list of placements >>>> passed in. >>>> + * >>>> + * Return: max of the min_page_size >>>> + */ >>>> +u32 i915_gem_object_max_page_size(struct intel_memory_region >>>> **placements, >>>> + unsigned int n_placements) >>>> { >>>> - u32 max_page_size = 0; >>>> + u32 max_page_size = I915_GTT_PAGE_SIZE_4K; >>>> int i; >>>> for (i = 0; i < n_placements; i++) { >>>> @@ -28,7 +38,6 @@ static u32 object_max_page_size(struct >>>> intel_memory_region **placements, >>>> max_page_size = max_t(u32, max_page_size, mr->min_page_size); >>>> } >>>> - GEM_BUG_ON(!max_page_size); >>>> return max_page_size; >>>> } >>>> @@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct >>>> drm_i915_private *i915, u64 size, >>>> i915_gem_flush_free_objects(i915); >>>> - size = round_up(size, object_max_page_size(placements, >>>> n_placements)); >>>> + size = round_up(size, i915_gem_object_max_page_size(placements, >>>> + n_placements)); >>>> if (size == 0) >>>> return ERR_PTR(-EINVAL); >>> >>> Because of the changes above this path is now unreachable. I suppose >>> it was meant to tell the user "you have supplied no placements"? But >>> then GEM_BUG_ON (which you remove) used to be wrong. >>> >> >> Yah, looks like an existing problem. May be this "size == 0" check >> should have been made before we do the round_up()? ie., check input >> 'size' >> paramter is not 0? >> I think for now, I will remove this check as it was unreachable anyhow. > > Hm that's true as well. i915_gem_create_ext_ioctl ensures at least one > placement and internal callers do as well. > > To be safe, instead of removing maybe move to before "size = " and > change to "if (GEM_WARN_ON(n_placements == 0))"? Not sure.. Matt any > thoughts here given the changes in this patch? The check is also to reject a zero sized object with args->size = 0, i.e round_up(0, PAGE_SIZE) == 0. So for sure that is still needed here. > > Regards, > > Tvrtko > >> >> Niranjana >> >>> Regards, >>> >>> Tvrtko >>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> index 7317d4102955..8c97bddad921 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> @@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 >>>> size) >>>> } >>>> void i915_gem_init__objects(struct drm_i915_private *i915); >>>> +u32 i915_gem_object_max_page_size(struct intel_memory_region >>>> **placements, >>>> + unsigned int n_placements); >>>> void i915_objects_module_exit(void); >>>> int i915_objects_module_init(void);
On Thu, Sep 22, 2022 at 05:18:28PM +0100, Matthew Auld wrote: >On 22/09/2022 09:09, Tvrtko Ursulin wrote: >> >>On 21/09/2022 19:00, Niranjana Vishwanathapura wrote: >>>On Wed, Sep 21, 2022 at 10:13:12AM +0100, Tvrtko Ursulin wrote: >>>> >>>>On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: >>>>>Expose i915_gem_object_max_page_size() function non-static >>>>>which will be used by the vm_bind feature. >>>>> >>>>>Signed-off-by: Niranjana Vishwanathapura >>>>><niranjana.vishwanathapura@intel.com> >>>>>Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> >>>>>--- >>>>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- >>>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ >>>>> 2 files changed, 17 insertions(+), 5 deletions(-) >>>>> >>>>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>>b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>>index 33673fe7ee0a..3b3ab4abb0a3 100644 >>>>>--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>>@@ -11,14 +11,24 @@ >>>>> #include "pxp/intel_pxp.h" >>>>> #include "i915_drv.h" >>>>>+#include "i915_gem_context.h" >>>> >>>>I can't spot that you are adding any code which would need this? >>>>I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. >>> >>>This include should have been added in a later patch for calling >>>i915_gem_vm_lookup(). But got added here while patch refactoring. >>>Will fix. >>> >>>> >>>>> #include "i915_gem_create.h" >>>>> #include "i915_trace.h" >>>>> #include "i915_user_extensions.h" >>>>>-static u32 object_max_page_size(struct intel_memory_region >>>>>**placements, >>>>>- unsigned int n_placements) >>>>>+/** >>>>>+ * i915_gem_object_max_page_size() - max of min_page_size of >>>>>the regions >>>>>+ * @placements: list of regions >>>>>+ * @n_placements: number of the placements >>>>>+ * >>>>>+ * Calculates the max of the min_page_size of a list of >>>>>placements passed in. >>>>>+ * >>>>>+ * Return: max of the min_page_size >>>>>+ */ >>>>>+u32 i915_gem_object_max_page_size(struct intel_memory_region >>>>>**placements, >>>>>+ unsigned int n_placements) >>>>> { >>>>>- u32 max_page_size = 0; >>>>>+ u32 max_page_size = I915_GTT_PAGE_SIZE_4K; >>>>> int i; >>>>> for (i = 0; i < n_placements; i++) { >>>>>@@ -28,7 +38,6 @@ static u32 object_max_page_size(struct >>>>>intel_memory_region **placements, >>>>> max_page_size = max_t(u32, max_page_size, mr->min_page_size); >>>>> } >>>>>- GEM_BUG_ON(!max_page_size); >>>>> return max_page_size; >>>>> } >>>>>@@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct >>>>>drm_i915_private *i915, u64 size, >>>>> i915_gem_flush_free_objects(i915); >>>>>- size = round_up(size, object_max_page_size(placements, >>>>>n_placements)); >>>>>+ size = round_up(size, i915_gem_object_max_page_size(placements, >>>>>+ n_placements)); >>>>> if (size == 0) >>>>> return ERR_PTR(-EINVAL); >>>> >>>>Because of the changes above this path is now unreachable. I >>>>suppose it was meant to tell the user "you have supplied no >>>>placements"? But then GEM_BUG_ON (which you remove) used to be >>>>wrong. >>>> >>> >>>Yah, looks like an existing problem. May be this "size == 0" check >>>should have been made before we do the round_up()? ie., check >>>input 'size' >>>paramter is not 0? >>>I think for now, I will remove this check as it was unreachable anyhow. >> >>Hm that's true as well. i915_gem_create_ext_ioctl ensures at least >>one placement and internal callers do as well. >> >>To be safe, instead of removing maybe move to before "size = " and >>change to "if (GEM_WARN_ON(n_placements == 0))"? Not sure.. Matt any >>thoughts here given the changes in this patch? > >The check is also to reject a zero sized object with args->size = 0, >i.e round_up(0, PAGE_SIZE) == 0. So for sure that is still needed >here. Thanks Matt. Yah, we could check for "size == 0" before we round_up, but doing it after like here should be just fine. Will keep it as is. Niranjana > >> >>Regards, >> >>Tvrtko >> >>> >>>Niranjana >>> >>>>Regards, >>>> >>>>Tvrtko >>>> >>>>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>index 7317d4102955..8c97bddad921 100644 >>>>>--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>>>@@ -47,6 +47,8 @@ static inline bool >>>>>i915_gem_object_size_2big(u64 size) >>>>> } >>>>> void i915_gem_init__objects(struct drm_i915_private *i915); >>>>>+u32 i915_gem_object_max_page_size(struct intel_memory_region >>>>>**placements, >>>>>+ unsigned int n_placements); >>>>> void i915_objects_module_exit(void); >>>>> int i915_objects_module_init(void);
On 22/09/2022 17:18, Matthew Auld wrote: > On 22/09/2022 09:09, Tvrtko Ursulin wrote: >> >> On 21/09/2022 19:00, Niranjana Vishwanathapura wrote: >>> On Wed, Sep 21, 2022 at 10:13:12AM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 21/09/2022 08:09, Niranjana Vishwanathapura wrote: >>>>> Expose i915_gem_object_max_page_size() function non-static >>>>> which will be used by the vm_bind feature. >>>>> >>>>> Signed-off-by: Niranjana Vishwanathapura >>>>> <niranjana.vishwanathapura@intel.com> >>>>> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 20 +++++++++++++++----- >>>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++ >>>>> 2 files changed, 17 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> index 33673fe7ee0a..3b3ab4abb0a3 100644 >>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> @@ -11,14 +11,24 @@ >>>>> #include "pxp/intel_pxp.h" >>>>> #include "i915_drv.h" >>>>> +#include "i915_gem_context.h" >>>> >>>> I can't spot that you are adding any code which would need this? >>>> I915_GTT_PAGE_SIZE_4K? It is in intel_gtt.h. >>> >>> This include should have been added in a later patch for calling >>> i915_gem_vm_lookup(). But got added here while patch refactoring. >>> Will fix. >>> >>>> >>>>> #include "i915_gem_create.h" >>>>> #include "i915_trace.h" >>>>> #include "i915_user_extensions.h" >>>>> -static u32 object_max_page_size(struct intel_memory_region >>>>> **placements, >>>>> - unsigned int n_placements) >>>>> +/** >>>>> + * i915_gem_object_max_page_size() - max of min_page_size of the >>>>> regions >>>>> + * @placements: list of regions >>>>> + * @n_placements: number of the placements >>>>> + * >>>>> + * Calculates the max of the min_page_size of a list of placements >>>>> passed in. >>>>> + * >>>>> + * Return: max of the min_page_size >>>>> + */ >>>>> +u32 i915_gem_object_max_page_size(struct intel_memory_region >>>>> **placements, >>>>> + unsigned int n_placements) >>>>> { >>>>> - u32 max_page_size = 0; >>>>> + u32 max_page_size = I915_GTT_PAGE_SIZE_4K; >>>>> int i; >>>>> for (i = 0; i < n_placements; i++) { >>>>> @@ -28,7 +38,6 @@ static u32 object_max_page_size(struct >>>>> intel_memory_region **placements, >>>>> max_page_size = max_t(u32, max_page_size, mr->min_page_size); >>>>> } >>>>> - GEM_BUG_ON(!max_page_size); >>>>> return max_page_size; >>>>> } >>>>> @@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct >>>>> drm_i915_private *i915, u64 size, >>>>> i915_gem_flush_free_objects(i915); >>>>> - size = round_up(size, object_max_page_size(placements, >>>>> n_placements)); >>>>> + size = round_up(size, i915_gem_object_max_page_size(placements, >>>>> + n_placements)); >>>>> if (size == 0) >>>>> return ERR_PTR(-EINVAL); >>>> >>>> Because of the changes above this path is now unreachable. I suppose >>>> it was meant to tell the user "you have supplied no placements"? But >>>> then GEM_BUG_ON (which you remove) used to be wrong. >>>> >>> >>> Yah, looks like an existing problem. May be this "size == 0" check >>> should have been made before we do the round_up()? ie., check input >>> 'size' >>> paramter is not 0? >>> I think for now, I will remove this check as it was unreachable anyhow. >> >> Hm that's true as well. i915_gem_create_ext_ioctl ensures at least one >> placement and internal callers do as well. >> >> To be safe, instead of removing maybe move to before "size = " and >> change to "if (GEM_WARN_ON(n_placements == 0))"? Not sure.. Matt any >> thoughts here given the changes in this patch? > > The check is also to reject a zero sized object with args->size = 0, i.e > round_up(0, PAGE_SIZE) == 0. So for sure that is still needed here. Oh yeah sneaky round up.. Thanks, my bad. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 33673fe7ee0a..3b3ab4abb0a3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -11,14 +11,24 @@ #include "pxp/intel_pxp.h" #include "i915_drv.h" +#include "i915_gem_context.h" #include "i915_gem_create.h" #include "i915_trace.h" #include "i915_user_extensions.h" -static u32 object_max_page_size(struct intel_memory_region **placements, - unsigned int n_placements) +/** + * i915_gem_object_max_page_size() - max of min_page_size of the regions + * @placements: list of regions + * @n_placements: number of the placements + * + * Calculates the max of the min_page_size of a list of placements passed in. + * + * Return: max of the min_page_size + */ +u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, + unsigned int n_placements) { - u32 max_page_size = 0; + u32 max_page_size = I915_GTT_PAGE_SIZE_4K; int i; for (i = 0; i < n_placements; i++) { @@ -28,7 +38,6 @@ static u32 object_max_page_size(struct intel_memory_region **placements, max_page_size = max_t(u32, max_page_size, mr->min_page_size); } - GEM_BUG_ON(!max_page_size); return max_page_size; } @@ -99,7 +108,8 @@ __i915_gem_object_create_user_ext(struct drm_i915_private *i915, u64 size, i915_gem_flush_free_objects(i915); - size = round_up(size, object_max_page_size(placements, n_placements)); + size = round_up(size, i915_gem_object_max_page_size(placements, + n_placements)); if (size == 0) return ERR_PTR(-EINVAL); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 7317d4102955..8c97bddad921 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64 size) } void i915_gem_init__objects(struct drm_i915_private *i915); +u32 i915_gem_object_max_page_size(struct intel_memory_region **placements, + unsigned int n_placements); void i915_objects_module_exit(void); int i915_objects_module_init(void);