Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC

["Das, Nirmoy" <nirmoy.das@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> > Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> > Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxx>


Thanks Andrzej,

Nirmoy

> Regards
> Andrzej
>
>
> >           struct i915_deps deps;
> >             i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | 
> > __GFP_NOWARN);