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

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

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);
>