Re: [Intel-gfx] [PATCH 1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer

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

On 3/6/2023 2:49 PM, Matthew Auld wrote:
> On 06/03/2023 13:31, Das, Nirmoy wrote:
>> Hi Matt,
>>
>> On 3/6/2023 1:25 PM, Matthew Auld wrote:
>>> On 06/03/2023 12:07, Nirmoy Das wrote:
>>>> Framebuffer is exposed to userspace so set I915_BO_ALLOC_USER
>>>> flag for it. This also make sure that ttm allocates offset
>>>> for lmem objects.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/display/intel_dpt.c           | 4 +++-
>>>>   drivers/gpu/drm/i915/display/intel_fbdev.c         | 3 ++-
>>>>   drivers/gpu/drm/i915/display/intel_plane_initial.c | 3 ++-
>>>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c 
>>>> b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>> index ad1a37b515fb..2e6238881860 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>> @@ -254,7 +254,9 @@ intel_dpt_create(struct intel_framebuffer *fb)
>>>>         size = round_up(size * sizeof(gen8_pte_t), 
>>>> I915_GTT_PAGE_SIZE);
>>>>   -    dpt_obj = i915_gem_object_create_lmem(i915, size, 
>>>> I915_BO_ALLOC_CONTIGUOUS);
>>>> +    dpt_obj = i915_gem_object_create_lmem(i915, size,
>>>> +                          I915_BO_ALLOC_CONTIGUOUS |
>>>> +                          I915_BO_ALLOC_USER);
>>>
>>> AFAICT this is just some driver internal stuff for display 
>>> page-table, which gets mapped through GGTT or something, and is not 
>>> the actual fb. Is it really exposed to the user?
>>
>>
>> I misunderstood this for something else. I will remove this.
>>
>>>
>>>>       if (IS_ERR(dpt_obj) && 
>>>> i915_ggtt_has_aperture(to_gt(i915)->ggtt))
>>>>           dpt_obj = i915_gem_object_create_stolen(i915, size);
>>>>       if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c 
>>>> b/drivers/gpu/drm/i915/display/intel_fbdev.c
>>>> index 3659350061a7..98ae3a3a986a 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>>>> @@ -163,7 +163,8 @@ static int intelfb_alloc(struct drm_fb_helper 
>>>> *helper,
>>>>       obj = ERR_PTR(-ENODEV);
>>>>       if (HAS_LMEM(dev_priv)) {
>>>>           obj = i915_gem_object_create_lmem(dev_priv, size,
>>>> -                          I915_BO_ALLOC_CONTIGUOUS);
>>>> +                          I915_BO_ALLOC_CONTIGUOUS |
>>>> +                          I915_BO_ALLOC_USER);
>>>>       } else {
>>>>           /*
>>>>            * If the FB is too big, just don't use it since fbdev is 
>>>> not very
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c 
>>>> b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>>> index bb6ea7de5c61..4a3680f6a3f5 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>>> @@ -110,7 +110,8 @@ initial_plane_vma(struct drm_i915_private *i915,
>>>>           size * 2 > i915->dsm.usable_size)
>>>>           return NULL;
>>>>   -    obj = i915_gem_object_create_region_at(mem, phys_base, size, 
>>>> 0);
>>>> +    obj = i915_gem_object_create_region_at(mem, phys_base, size,
>>>> +                           I915_BO_ALLOC_USER);
>>>
>>> ALLOC_USER has the side effect of also zeroing the memory 
>>> underneath, IIRC. However this here is the pre-allocated fb (will 
>>> have some boot logo stuff), so we shouldn't ever clear it.
>>
>>
>> This was my concern.  I wonder if there is any other better way than 
>> to use a temp buffer to copy the pre-allocated content and put it 
>> back after getting i915_gem_object_create_region_at().
>
> If we need ALLOC_USER for this buffer then maybe just a new flag like 
> BO_PREALLOCATED which skips all the clearing?


Sounds good, I will look into that.


Thanks,

Nirmoy

>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>
>>>
>>>>       if (IS_ERR(obj))
>>>>           return NULL;