| Message ID | 20230404143100.10452-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/ttm: Add I915_BO_PREALLOC | expand |
On 04.04.2023 16:30, Nirmoy Das wrote: > Add a mechanism to keep existing data when creating > a ttm object with I915_BO_ALLOC_USER flag. > > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++++++++++---- > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 5 +++-- > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > index 5dcbbef31d44..830c11431ee8 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -328,6 +328,12 @@ struct drm_i915_gem_object { > */ > #define I915_BO_ALLOC_GPU_ONLY BIT(6) > #define I915_BO_ALLOC_CCS_AUX BIT(7) > +/* > + * Object is allowed to retain its initial data and will not be cleared on first > + * access if used along with I915_BO_ALLOC_USER. This is mainly to keep > + * preallocated framebuffer data intact while transitioning it to i915drmfb. > + */ > +#define I915_BO_PREALLOC BIT(8) > #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ > I915_BO_ALLOC_VOLATILE | \ > I915_BO_ALLOC_CPU_CLEAR | \ > @@ -335,10 +341,11 @@ struct drm_i915_gem_object { > I915_BO_ALLOC_PM_VOLATILE | \ > I915_BO_ALLOC_PM_EARLY | \ > I915_BO_ALLOC_GPU_ONLY | \ > - I915_BO_ALLOC_CCS_AUX) > -#define I915_BO_READONLY BIT(8) > -#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not release! */ > -#define I915_BO_PROTECTED BIT(10) > + I915_BO_ALLOC_CCS_AUX | \ > + I915_BO_PREALLOC) > +#define I915_BO_READONLY BIT(9) > +#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not release! */ > +#define I915_BO_PROTECTED BIT(11) > /** > * @mem_flags - Mutable placement-related flags > * > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > index dd188dfcc423..69eb20ed4d47 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > @@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, > struct dma_fence *migration_fence = NULL; > struct ttm_tt *ttm = bo->ttm; > struct i915_refct_sgt *dst_rsgt; > - bool clear; > + bool clear, prealloc_bo; > int ret; > > if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) { > @@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, > return PTR_ERR(dst_rsgt); > > clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm || !ttm_tt_is_populated(ttm)); > - if (!(clear && ttm && !(ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC))) { > + prealloc_bo = obj->flags & I915_BO_PREALLOC; > + if (!(clear && ttm && !((ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) && !prealloc_bo))) { This looks like school exercise for complicated usage of logical operators, and I have problem with understanding this :) Couldn't this be somehow simplified? Anyway as the patch just reuses existing code: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > struct i915_deps deps; > > i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
Hi Nirmoy, On Tue, Apr 04, 2023 at 04:30:56PM +0200, Nirmoy Das wrote: > Add a mechanism to keep existing data when creating > a ttm object with I915_BO_ALLOC_USER flag. why do we need this mechanism? What was the logic behind? These are all questions people might have when checking this commit. Please be a bit more explicative. > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
On 4/4/2023 5:30 PM, Andrzej Hajda wrote: > > > On 04.04.2023 16:30, Nirmoy Das wrote: >> Add a mechanism to keep existing data when creating >> a ttm object with I915_BO_ALLOC_USER flag. >> >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> Cc: Andrzej Hajda <andrzej.hajda@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++++++++++---- >> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 5 +++-- >> 2 files changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> index 5dcbbef31d44..830c11431ee8 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> @@ -328,6 +328,12 @@ struct drm_i915_gem_object { >> */ >> #define I915_BO_ALLOC_GPU_ONLY BIT(6) >> #define I915_BO_ALLOC_CCS_AUX BIT(7) >> +/* >> + * Object is allowed to retain its initial data and will not be >> cleared on first >> + * access if used along with I915_BO_ALLOC_USER. This is mainly to keep >> + * preallocated framebuffer data intact while transitioning it to >> i915drmfb. >> + */ >> +#define I915_BO_PREALLOC BIT(8) >> #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ >> I915_BO_ALLOC_VOLATILE | \ >> I915_BO_ALLOC_CPU_CLEAR | \ >> @@ -335,10 +341,11 @@ struct drm_i915_gem_object { >> I915_BO_ALLOC_PM_VOLATILE | \ >> I915_BO_ALLOC_PM_EARLY | \ >> I915_BO_ALLOC_GPU_ONLY | \ >> - I915_BO_ALLOC_CCS_AUX) >> -#define I915_BO_READONLY BIT(8) >> -#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not >> release! */ >> -#define I915_BO_PROTECTED BIT(10) >> + I915_BO_ALLOC_CCS_AUX | \ >> + I915_BO_PREALLOC) >> +#define I915_BO_READONLY BIT(9) >> +#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not >> release! */ >> +#define I915_BO_PROTECTED BIT(11) >> /** >> * @mem_flags - Mutable placement-related flags >> * >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> index dd188dfcc423..69eb20ed4d47 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> @@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, >> bool evict, >> struct dma_fence *migration_fence = NULL; >> struct ttm_tt *ttm = bo->ttm; >> struct i915_refct_sgt *dst_rsgt; >> - bool clear; >> + bool clear, prealloc_bo; >> int ret; >> if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) { >> @@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo, >> bool evict, >> return PTR_ERR(dst_rsgt); >> clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm || >> !ttm_tt_is_populated(ttm)); >> - if (!(clear && ttm && !(ttm->page_flags & >> TTM_TT_FLAG_ZERO_ALLOC))) { >> + prealloc_bo = obj->flags & I915_BO_PREALLOC; >> + if (!(clear && ttm && !((ttm->page_flags & >> TTM_TT_FLAG_ZERO_ALLOC) && !prealloc_bo))) { > > This looks like school exercise for complicated usage of logical > operators, and I have problem with understanding this :) > Couldn't this be somehow simplified? (I thought I sent this email yesterday but was stuck in oAuth pop up sign-in) Yes, this can be improved I think, took me while too. > > Anyway as the patch just reuses existing code: > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Thanks Andrzej, Nirmoy > > Regards > Andrzej > > >> struct i915_deps deps; >> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | >> __GFP_NOWARN); >
On 4/4/2023 6:23 PM, Andi Shyti wrote: > Hi Nirmoy, > > On Tue, Apr 04, 2023 at 04:30:56PM +0200, Nirmoy Das wrote: >> Add a mechanism to keep existing data when creating >> a ttm object with I915_BO_ALLOC_USER flag. > why do we need this mechanism? What was the logic behind? These > are all questions people might have when checking this commit. > Please be a bit more explicative. Agree, the commit message is bit short. I will add more content in next revision. > >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> Cc: Andrzej Hajda <andrzej.hajda@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Nirmoy > > Thanks, > Andi
Hi Nirmoy, > > > Add a mechanism to keep existing data when creating > > > a ttm object with I915_BO_ALLOC_USER flag. > > why do we need this mechanism? What was the logic behind? These > > are all questions people might have when checking this commit. > > Please be a bit more explicative. > > > Agree, the commit message is bit short. I will add more content in next > revision. you don't need to send a new version just for this commit log. You could just propose a new commit log in the reply and if it's OK, add it before pushing it. As you wish. Andi > > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > > Thanks, > > Nirmoy > > > > > Thanks, > > Andi
Hi Andi, On 4/5/2023 1:53 PM, Andi Shyti wrote: > Hi Nirmoy, > >>>> Add a mechanism to keep existing data when creating >>>> a ttm object with I915_BO_ALLOC_USER flag. >>> why do we need this mechanism? What was the logic behind? These >>> are all questions people might have when checking this commit. >>> Please be a bit more explicative. >> >> Agree, the commit message is bit short. I will add more content in next >> revision. > you don't need to send a new version just for this commit log. > > You could just propose a new commit log in the reply and if it's > OK, add it before pushing it. Let me know what do you think about: Add a mechanism to preserve existing data when creating a TTM object with the I915_BO_ALLOC_USER flag. This will be used in the subsequent patch where the I915_BO_ALLOC_USER flag will be applied to the framebuffer object. For a pre-allocated framebuffer without the I915_BO_PREALLOC flag, TTM would clear the content, which is not desirable. Thanks, Nirmoy > > As you wish. > > Andi > >>>> Cc: Matthew Auld<matthew.auld@intel.com> >>>> Cc: Andi Shyti<andi.shyti@linux.intel.com> >>>> Cc: Andrzej Hajda<andrzej.hajda@intel.com> >>>> Cc: Ville Syrjälä<ville.syrjala@linux.intel.com> >>>> Cc: Jani Nikula<jani.nikula@intel.com> >>>> Cc: Imre Deak<imre.deak@intel.com> >>>> Signed-off-by: Nirmoy Das<nirmoy.das@intel.com> >>> Reviewed-by: Andi Shyti<andi.shyti@linux.intel.com> >> >> Thanks, >> >> Nirmoy >> >>> Thanks, >>> Andi
> Add a mechanism to preserve existing data when creating a TTM > object with the I915_BO_ALLOC_USER flag. This will be used in the subsequent > patch where the I915_BO_ALLOC_USER flag will be applied to the framebuffer > object. For a pre-allocated framebuffer without the I915_BO_PREALLOC flag, > TTM would clear the content, which is not desirable. ack! Andi
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 5dcbbef31d44..830c11431ee8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -328,6 +328,12 @@ struct drm_i915_gem_object { */ #define I915_BO_ALLOC_GPU_ONLY BIT(6) #define I915_BO_ALLOC_CCS_AUX BIT(7) +/* + * Object is allowed to retain its initial data and will not be cleared on first + * access if used along with I915_BO_ALLOC_USER. This is mainly to keep + * preallocated framebuffer data intact while transitioning it to i915drmfb. + */ +#define I915_BO_PREALLOC BIT(8) #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ I915_BO_ALLOC_VOLATILE | \ I915_BO_ALLOC_CPU_CLEAR | \ @@ -335,10 +341,11 @@ struct drm_i915_gem_object { I915_BO_ALLOC_PM_VOLATILE | \ I915_BO_ALLOC_PM_EARLY | \ I915_BO_ALLOC_GPU_ONLY | \ - I915_BO_ALLOC_CCS_AUX) -#define I915_BO_READONLY BIT(8) -#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not release! */ -#define I915_BO_PROTECTED BIT(10) + I915_BO_ALLOC_CCS_AUX | \ + I915_BO_PREALLOC) +#define I915_BO_READONLY BIT(9) +#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not release! */ +#define I915_BO_PROTECTED BIT(11) /** * @mem_flags - Mutable placement-related flags * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c index dd188dfcc423..69eb20ed4d47 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c @@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, struct dma_fence *migration_fence = NULL; struct ttm_tt *ttm = bo->ttm; struct i915_refct_sgt *dst_rsgt; - bool clear; + bool clear, prealloc_bo; int ret; if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) { @@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, return PTR_ERR(dst_rsgt); clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm || !ttm_tt_is_populated(ttm)); - if (!(clear && ttm && !(ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC))) { + prealloc_bo = obj->flags & I915_BO_PREALLOC; + if (!(clear && ttm && !((ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) && !prealloc_bo))) { struct i915_deps deps; i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
Add a mechanism to keep existing data when creating a ttm object with I915_BO_ALLOC_USER flag. Cc: Matthew Auld <matthew.auld@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++++++++++---- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 5 +++-- 2 files changed, 14 insertions(+), 6 deletions(-)