| Message ID | 20230306120732.28656-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer | expand |
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? > 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. > if (IS_ERR(obj)) > return NULL; >
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(). Regards, Nirmoy > >> if (IS_ERR(obj)) >> return NULL;
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? > > > Regards, > > Nirmoy > > >> >>> if (IS_ERR(obj)) >>> return NULL;
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;
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); 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); if (IS_ERR(obj)) return NULL;
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(-)