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

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

[Nirmoy Das]

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

[Intel-gfx] [PATCH 2/3] drm/i915: Add a helper func for gem obj mmap

[Nirmoy Das]

Move gem obj mmap code to i915_gem_object_mmap() so that
this can be used by others.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 20 ++---------------
 drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 25 ++++++++++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  1 +
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index fd556a076d05..831dd8ebf819 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -12,6 +12,7 @@
 #include <asm/smp.h>
 
 #include "gem/i915_gem_dmabuf.h"
+#include "gem/i915_gem_mman.h"
 #include "i915_drv.h"
 #include "i915_gem_object.h"
 #include "i915_scatterlist.h"
@@ -94,27 +95,10 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf,
 static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	int ret;
 
 	dma_resv_assert_held(dma_buf->resv);
 
-	if (obj->base.size < vma->vm_end - vma->vm_start)
-		return -EINVAL;
-
-	if (HAS_LMEM(i915))
-		return drm_gem_prime_mmap(&obj->base, vma);
-
-	if (!obj->base.filp)
-		return -ENODEV;
-
-	ret = call_mmap(obj->base.filp, vma);
-	if (ret)
-		return ret;
-
-	vma_set_file(vma, obj->base.filp);
-
-	return 0;
+	return i915_gem_object_mmap(obj, vma);
 }
 
 static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 2aac6bf78740..d378720ca626 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -11,6 +11,8 @@
 
 #include <drm/drm_cache.h>
 
+#include "gem/i915_gem_lmem.h"
+
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_requests.h"
 
@@ -1043,6 +1045,29 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	return 0;
 }
 
+int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	int ret;
+
+	if (obj->base.size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	if (HAS_LMEM(i915))
+		return drm_gem_prime_mmap(&obj->base, vma);
+
+	if (obj->base.filp) {
+		ret = call_mmap(obj->base.filp, vma);
+		if (ret)
+			return ret;
+
+		vma_set_file(vma, obj->base.filp);
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/i915_gem_mman.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
index 1fa91b3033b3..303e81ddc5ba 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
@@ -30,4 +30,5 @@ void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
 void i915_gem_object_runtime_pm_release_mmap_offset(struct drm_i915_gem_object *obj);
 void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj);
 
+int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma);
 #endif
-- 
2.39.0

[Intel-gfx] [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function

[Nirmoy Das]

If stolen memory allocation fails for fbdev, the driver will
fallback to system memory. Calculation of smem_start is wrong
for such framebuffer objs if the platform comes with no gmadr or
no aperture. Solve this by adding fb_mmap callback which also gives
driver more control.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 98ae3a3a986a..ed0f9e2af3ed 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -40,8 +40,10 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 #include "gem/i915_gem_lmem.h"
+#include "gem/i915_gem_mman.h"
 
 #include "i915_drv.h"
 #include "intel_display_types.h"
@@ -120,6 +122,23 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
 	return ret;
 }
 
+#define to_intel_fbdev(x) container_of(x, struct intel_fbdev, helper)
+static int intel_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	struct intel_fbdev *fbdev = to_intel_fbdev(info->par);
+	struct drm_gem_object *bo = drm_gem_fb_get_obj(&fbdev->fb->base, 0);
+	struct drm_i915_gem_object *obj = to_intel_bo(bo);
+	struct drm_device *dev = fbdev->helper.dev;
+
+	vma->vm_page_prot =
+			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+
+	if (obj->stolen)
+		return vm_iomap_memory(vma, info->fix.smem_start,
+				       info->fix.smem_len);
+
+	return i915_gem_object_mmap(obj, vma);
+}
 static const struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	DRM_FB_HELPER_DEFAULT_OPS,
@@ -131,6 +150,7 @@ static const struct fb_ops intelfb_ops = {
 	.fb_imageblit = drm_fb_helper_cfb_imageblit,
 	.fb_pan_display = intel_fbdev_pan_display,
 	.fb_blank = intel_fbdev_blank,
+	.fb_mmap = intel_fbdev_mmap,
 };
 
 static int intelfb_alloc(struct drm_fb_helper *helper,
-- 
2.39.0

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer
URL   : https://patchwork.freedesktop.org/series/114693/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_fbdev.o
drivers/gpu/drm/i915/display/intel_fbdev.c: In function ‘intel_fbdev_mmap’:
drivers/gpu/drm/i915/display/intel_fbdev.c:131:21: error: unused variable ‘dev’ [-Werror=unused-variable]
  131 |  struct drm_device *dev = fbdev->helper.dev;
      |                     ^~~
cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:252: drivers/gpu/drm/i915/display/intel_fbdev.o] Error 1
make[4]: *** [scripts/Makefile.build:494: drivers/gpu/drm/i915] Error 2
make[3]: *** [scripts/Makefile.build:494: drivers/gpu/drm] Error 2
make[2]: *** [scripts/Makefile.build:494: drivers/gpu] Error 2
make[1]: *** [scripts/Makefile.build:494: drivers] Error 2
make: *** [Makefile:2028: .] Error 2

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

[Nirmoy Das]

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

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

[Matthew Auld]

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

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

[Das, Nirmoy]

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;

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

[Matthew Auld]

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;

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

[Das, Nirmoy]

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;

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

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 11:28:48AM +0100, 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.

I have no idea what that means.

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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add a helper func for gem obj mmap

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 11:28:49AM +0100, Nirmoy Das wrote:
> Move gem obj mmap code to i915_gem_object_mmap() so that
> this can be used by others.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 20 ++---------------
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 25 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  1 +
>  3 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index fd556a076d05..831dd8ebf819 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -12,6 +12,7 @@
>  #include <asm/smp.h>
>  
>  #include "gem/i915_gem_dmabuf.h"
> +#include "gem/i915_gem_mman.h"
>  #include "i915_drv.h"
>  #include "i915_gem_object.h"
>  #include "i915_scatterlist.h"
> @@ -94,27 +95,10 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf,
>  static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  {
>  	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> -	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> -	int ret;
>  
>  	dma_resv_assert_held(dma_buf->resv);
>  
> -	if (obj->base.size < vma->vm_end - vma->vm_start)
> -		return -EINVAL;
> -
> -	if (HAS_LMEM(i915))
> -		return drm_gem_prime_mmap(&obj->base, vma);
> -
> -	if (!obj->base.filp)
> -		return -ENODEV;
> -
> -	ret = call_mmap(obj->base.filp, vma);
> -	if (ret)
> -		return ret;
> -
> -	vma_set_file(vma, obj->base.filp);
> -
> -	return 0;
> +	return i915_gem_object_mmap(obj, vma);
>  }
>  
>  static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 2aac6bf78740..d378720ca626 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -11,6 +11,8 @@
>  
>  #include <drm/drm_cache.h>
>  
> +#include "gem/i915_gem_lmem.h"
> +
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_requests.h"
>  
> @@ -1043,6 +1045,29 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  	return 0;
>  }
>  
> +int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	int ret;
> +
> +	if (obj->base.size < vma->vm_end - vma->vm_start)
> +		return -EINVAL;
> +
> +	if (HAS_LMEM(i915))
> +		return drm_gem_prime_mmap(&obj->base, vma);

Calling some prime stuff here doesn't smell right.

> +
> +	if (obj->base.filp) {
> +		ret = call_mmap(obj->base.filp, vma);
> +		if (ret)
> +			return ret;
> +
> +		vma_set_file(vma, obj->base.filp);
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/i915_gem_mman.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> index 1fa91b3033b3..303e81ddc5ba 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> @@ -30,4 +30,5 @@ void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>  void i915_gem_object_runtime_pm_release_mmap_offset(struct drm_i915_gem_object *obj);
>  void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj);
>  
> +int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma);
>  #endif
> -- 
> 2.39.0

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 11:28:50AM +0100, Nirmoy Das wrote:
> If stolen memory allocation fails for fbdev, the driver will
> fallback to system memory. Calculation of smem_start is wrong
> for such framebuffer objs if the platform comes with no gmadr or
> no aperture. Solve this by adding fb_mmap callback which also gives
> driver more control.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 98ae3a3a986a..ed0f9e2af3ed 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -40,8 +40,10 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  
>  #include "gem/i915_gem_lmem.h"
> +#include "gem/i915_gem_mman.h"
>  
>  #include "i915_drv.h"
>  #include "intel_display_types.h"
> @@ -120,6 +122,23 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>  	return ret;
>  }
>  
> +#define to_intel_fbdev(x) container_of(x, struct intel_fbdev, helper)
> +static int intel_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +{
> +	struct intel_fbdev *fbdev = to_intel_fbdev(info->par);
> +	struct drm_gem_object *bo = drm_gem_fb_get_obj(&fbdev->fb->base, 0);
> +	struct drm_i915_gem_object *obj = to_intel_bo(bo);
> +	struct drm_device *dev = fbdev->helper.dev;

You seem to be missing the fb vs. mmio handling here entirely.

> +
> +	vma->vm_page_prot =
> +			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));

Does that do something sane on eg. !PAT?

> +
> +	if (obj->stolen)
> +		return vm_iomap_memory(vma, info->fix.smem_start,
> +				       info->fix.smem_len);

Why doesn't i915_gem_object_mmap() know how to handle stolen?

> +
> +	return i915_gem_object_mmap(obj, vma);
> +}
>  static const struct fb_ops intelfb_ops = {
>  	.owner = THIS_MODULE,
>  	DRM_FB_HELPER_DEFAULT_OPS,
> @@ -131,6 +150,7 @@ static const struct fb_ops intelfb_ops = {
>  	.fb_imageblit = drm_fb_helper_cfb_imageblit,
>  	.fb_pan_display = intel_fbdev_pan_display,
>  	.fb_blank = intel_fbdev_blank,
> +	.fb_mmap = intel_fbdev_mmap,
>  };
>  
>  static int intelfb_alloc(struct drm_fb_helper *helper,
> -- 
> 2.39.0

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Add a helper func for gem obj mmap

[Das, Nirmoy]

On 3/6/2023 3:26 PM, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 11:28:49AM +0100, Nirmoy Das wrote:
>> Move gem obj mmap code to i915_gem_object_mmap() so that
>> this can be used by others.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 20 ++---------------
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 25 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  1 +
>>   3 files changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> index fd556a076d05..831dd8ebf819 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> @@ -12,6 +12,7 @@
>>   #include <asm/smp.h>
>>   
>>   #include "gem/i915_gem_dmabuf.h"
>> +#include "gem/i915_gem_mman.h"
>>   #include "i915_drv.h"
>>   #include "i915_gem_object.h"
>>   #include "i915_scatterlist.h"
>> @@ -94,27 +95,10 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf,
>>   static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>>   {
>>   	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>> -	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> -	int ret;
>>   
>>   	dma_resv_assert_held(dma_buf->resv);
>>   
>> -	if (obj->base.size < vma->vm_end - vma->vm_start)
>> -		return -EINVAL;
>> -
>> -	if (HAS_LMEM(i915))
>> -		return drm_gem_prime_mmap(&obj->base, vma);
>> -
>> -	if (!obj->base.filp)
>> -		return -ENODEV;
>> -
>> -	ret = call_mmap(obj->base.filp, vma);
>> -	if (ret)
>> -		return ret;
>> -
>> -	vma_set_file(vma, obj->base.filp);
>> -
>> -	return 0;
>> +	return i915_gem_object_mmap(obj, vma);
>>   }
>>   
>>   static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 2aac6bf78740..d378720ca626 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -11,6 +11,8 @@
>>   
>>   #include <drm/drm_cache.h>
>>   
>> +#include "gem/i915_gem_lmem.h"
>> +
>>   #include "gt/intel_gt.h"
>>   #include "gt/intel_gt_requests.h"
>>   
>> @@ -1043,6 +1045,29 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>   	return 0;
>>   }
>>   
>> +int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma)
>> +{
>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> +	int ret;
>> +
>> +	if (obj->base.size < vma->vm_end - vma->vm_start)
>> +		return -EINVAL;
>> +
>> +	if (HAS_LMEM(i915))
>> +		return drm_gem_prime_mmap(&obj->base, vma);
> Calling some prime stuff here doesn't smell right.

Yes, I should use drm_gem_mmap_obj() here.


>
>> +
>> +	if (obj->base.filp) {
>> +		ret = call_mmap(obj->base.filp, vma);
>> +		if (ret)
>> +			return ret;
>> +
>> +		vma_set_file(vma, obj->base.filp);
>> +		return 0;
>> +	}
>> +
>> +	return -ENODEV;
>> +}
>> +
>>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>>   #include "selftests/i915_gem_mman.c"
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
>> index 1fa91b3033b3..303e81ddc5ba 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
>> @@ -30,4 +30,5 @@ void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>>   void i915_gem_object_runtime_pm_release_mmap_offset(struct drm_i915_gem_object *obj);
>>   void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj);
>>   
>> +int i915_gem_object_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma);
>>   #endif
>> -- 
>> 2.39.0

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

[Das, Nirmoy]

On 3/6/2023 3:21 PM, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 11:28:48AM +0100, 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.
> I have no idea what that means.

Sorry for poor explanation.

Without I915_BO_ALLOC_USER, ttm will assume the obj as kernel buffer and 
will not allocate fake offset which I needed for fb_mmap callback to work.

Regards,
Nirmoy

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

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

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 05:22:19PM +0100, Das, Nirmoy wrote:
> 
> On 3/6/2023 3:21 PM, Ville Syrjälä wrote:
> > On Mon, Mar 06, 2023 at 11:28:48AM +0100, 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.
> > I have no idea what that means.
> 
> Sorry for poor explanation.
> 
> Without I915_BO_ALLOC_USER, ttm will assume the obj as kernel buffer and 
> will not allocate fake offset which I needed for fb_mmap callback to work.

So that's the fake vm_pgoff thing? Doesn't that exist just so
mmap() through /dev/dri* can be passed a "gem handle"? 
With fbdev mmap we already know which BO we want to map so
why would any of that stuff even be needed?

> 
> Regards,
> Nirmoy
> 
> >
> >> 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);
> >>   	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;
> >>   
> >> -- 
> >> 2.39.0

-- 
Ville Syrjälä
Intel

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

[Das, Nirmoy]

On 3/6/2023 6:30 PM, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 05:22:19PM +0100, Das, Nirmoy wrote:
>> On 3/6/2023 3:21 PM, Ville Syrjälä wrote:
>>> On Mon, Mar 06, 2023 at 11:28:48AM +0100, 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.
>>> I have no idea what that means.
>> Sorry for poor explanation.
>>
>> Without I915_BO_ALLOC_USER, ttm will assume the obj as kernel buffer and
>> will not allocate fake offset which I needed for fb_mmap callback to work.
> So that's the fake vm_pgoff thing? Doesn't that exist just so
> mmap() through /dev/dri* can be passed a "gem handle"?
> With fbdev mmap we already know which BO we want to map so
> why would any of that stuff even be needed?


I was mainly concentrating on  using drm mmap API to achieve fb_mmap 
which eventually will call i915_gem_mmap()

and expects a  fake offset for the obj. I see your point: fb_mmap can be 
done without using drm mmap API which should be much simple . I will 
look into this and resend.


Thanks,

Nirmoy

>> Regards,
>> Nirmoy
>>
>>>> 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);
>>>>    	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;
>>>>    
>>>> -- 
>>>> 2.39.0

Re: [Intel-gfx] [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function

[Das, Nirmoy]

Hi Ville,

On 3/6/2023 3:32 PM, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 11:28:50AM +0100, Nirmoy Das wrote:
>> If stolen memory allocation fails for fbdev, the driver will
>> fallback to system memory. Calculation of smem_start is wrong
>> for such framebuffer objs if the platform comes with no gmadr or
>> no aperture. Solve this by adding fb_mmap callback which also gives
>> driver more control.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_fbdev.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index 98ae3a3a986a..ed0f9e2af3ed 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -40,8 +40,10 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_fb_helper.h>
>>   #include <drm/drm_fourcc.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>>   
>>   #include "gem/i915_gem_lmem.h"
>> +#include "gem/i915_gem_mman.h"
>>   
>>   #include "i915_drv.h"
>>   #include "intel_display_types.h"
>> @@ -120,6 +122,23 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>>   	return ret;
>>   }
>>   
>> +#define to_intel_fbdev(x) container_of(x, struct intel_fbdev, helper)
>> +static int intel_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> +{
>> +	struct intel_fbdev *fbdev = to_intel_fbdev(info->par);
>> +	struct drm_gem_object *bo = drm_gem_fb_get_obj(&fbdev->fb->base, 0);
>> +	struct drm_i915_gem_object *obj = to_intel_bo(bo);
>> +	struct drm_device *dev = fbdev->helper.dev;
> You seem to be missing the fb vs. mmio handling here entirely.


Could you please expand this more, I am not so familiar to fbdev code.


>
>> +
>> +	vma->vm_page_prot =
>> +			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> Does that do something sane on eg. !PAT?
>
>> +
>> +	if (obj->stolen)
>> +		return vm_iomap_memory(vma, info->fix.smem_start,
>> +				       info->fix.smem_len);
> Why doesn't i915_gem_object_mmap() know how to handle stolen?


Sent out another rfc series to address this.


Regards,

Nirmoy

>
>> +
>> +	return i915_gem_object_mmap(obj, vma);
>> +}
>>   static const struct fb_ops intelfb_ops = {
>>   	.owner = THIS_MODULE,
>>   	DRM_FB_HELPER_DEFAULT_OPS,
>> @@ -131,6 +150,7 @@ static const struct fb_ops intelfb_ops = {
>>   	.fb_imageblit = drm_fb_helper_cfb_imageblit,
>>   	.fb_pan_display = intel_fbdev_pan_display,
>>   	.fb_blank = intel_fbdev_blank,
>> +	.fb_mmap = intel_fbdev_mmap,
>>   };
>>   
>>   static int intelfb_alloc(struct drm_fb_helper *helper,
>> -- 
>> 2.39.0