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

[Matthew Auld <matthew.auld@xxxxxxxxx>]

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@xxxxxxxxx>
> > > ---
> > >   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?

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