dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj
@ 2023-03-20 10:09 Nirmoy Das
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Nirmoy Das @ 2023-03-20 10:09 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, dri-devel, Matthew Auld, Andi Shyti, Nirmoy Das

Implement i915_gem_fb_mmap() to enable fb_ops.fb_mmap()
callback for i915's framebuffer objects.

v2: add a comment why i915_gem_object_get() needed(Andi).

Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Andi Shyti <andi.shyti@linux.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>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 127 +++++++++++++++--------
 drivers/gpu/drm/i915/gem/i915_gem_mman.h |   2 +-
 2 files changed, 83 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index d3c1dee16af2..341e952d3510 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -927,53 +927,15 @@ static struct file *mmap_singleton(struct drm_i915_private *i915)
 	return file;
 }
 
-/*
- * This overcomes the limitation in drm_gem_mmap's assignment of a
- * drm_gem_object as the vma->vm_private_data. Since we need to
- * be able to resolve multiple mmap offsets which could be tied
- * to a single gem object.
- */
-int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
+static int
+i915_gem_object_mmap(struct drm_i915_gem_object *obj,
+		     struct i915_mmap_offset *mmo,
+		     struct vm_area_struct *vma)
 {
-	struct drm_vma_offset_node *node;
-	struct drm_file *priv = filp->private_data;
-	struct drm_device *dev = priv->minor->dev;
-	struct drm_i915_gem_object *obj = NULL;
-	struct i915_mmap_offset *mmo = NULL;
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct drm_device *dev = &i915->drm;
 	struct file *anon;
 
-	if (drm_dev_is_unplugged(dev))
-		return -ENODEV;
-
-	rcu_read_lock();
-	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
-	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
-						  vma->vm_pgoff,
-						  vma_pages(vma));
-	if (node && drm_vma_node_is_allowed(node, priv)) {
-		/*
-		 * Skip 0-refcnted objects as it is in the process of being
-		 * destroyed and will be invalid when the vma manager lock
-		 * is released.
-		 */
-		if (!node->driver_private) {
-			mmo = container_of(node, struct i915_mmap_offset, vma_node);
-			obj = i915_gem_object_get_rcu(mmo->obj);
-
-			GEM_BUG_ON(obj && obj->ops->mmap_ops);
-		} else {
-			obj = i915_gem_object_get_rcu
-				(container_of(node, struct drm_i915_gem_object,
-					      base.vma_node));
-
-			GEM_BUG_ON(obj && !obj->ops->mmap_ops);
-		}
-	}
-	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
-	rcu_read_unlock();
-	if (!obj)
-		return node ? -EACCES : -EINVAL;
-
 	if (i915_gem_object_is_readonly(obj)) {
 		if (vma->vm_flags & VM_WRITE) {
 			i915_gem_object_put(obj);
@@ -1005,7 +967,7 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	if (obj->ops->mmap_ops) {
 		vma->vm_page_prot = pgprot_decrypted(vm_get_page_prot(vma->vm_flags));
 		vma->vm_ops = obj->ops->mmap_ops;
-		vma->vm_private_data = node->driver_private;
+		vma->vm_private_data = obj->base.vma_node.driver_private;
 		return 0;
 	}
 
@@ -1043,6 +1005,81 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	return 0;
 }
 
+/*
+ * This overcomes the limitation in drm_gem_mmap's assignment of a
+ * drm_gem_object as the vma->vm_private_data. Since we need to
+ * be able to resolve multiple mmap offsets which could be tied
+ * to a single gem object.
+ */
+int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct drm_vma_offset_node *node;
+	struct drm_file *priv = filp->private_data;
+	struct drm_device *dev = priv->minor->dev;
+	struct drm_i915_gem_object *obj = NULL;
+	struct i915_mmap_offset *mmo = NULL;
+
+	if (drm_dev_is_unplugged(dev))
+		return -ENODEV;
+
+	rcu_read_lock();
+	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
+	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
+						  vma->vm_pgoff,
+						  vma_pages(vma));
+	if (node && drm_vma_node_is_allowed(node, priv)) {
+		/*
+		 * Skip 0-refcnted objects as it is in the process of being
+		 * destroyed and will be invalid when the vma manager lock
+		 * is released.
+		 */
+		if (!node->driver_private) {
+			mmo = container_of(node, struct i915_mmap_offset, vma_node);
+			obj = i915_gem_object_get_rcu(mmo->obj);
+
+			GEM_BUG_ON(obj && obj->ops->mmap_ops);
+		} else {
+			obj = i915_gem_object_get_rcu
+				(container_of(node, struct drm_i915_gem_object,
+					      base.vma_node));
+
+			GEM_BUG_ON(obj && !obj->ops->mmap_ops);
+		}
+	}
+	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
+	rcu_read_unlock();
+	if (!obj)
+		return node ? -EACCES : -EINVAL;
+
+	return i915_gem_object_mmap(obj, mmo, vma);
+}
+
+int i915_gem_fb_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct drm_device *dev = &i915->drm;
+	struct i915_mmap_offset *mmo = NULL;
+	enum i915_mmap_type mmap_type;
+	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
+
+	if (drm_dev_is_unplugged(dev))
+		return -ENODEV;
+
+	mmap_type = i915_ggtt_has_aperture(ggtt) ? I915_MMAP_TYPE_GTT : I915_MMAP_TYPE_WC;
+	mmo = mmap_offset_attach(obj, mmap_type, NULL);
+	if (!mmo)
+		return -ENODEV;
+
+	/*
+	 * When we install vm_ops for mmap we are too late for
+	 * the vm_ops->open() which increases the ref_count of
+	 * this obj and then it gets decreased by the vm_ops->close().
+	 * To balance this increase the obj ref_count here.
+	 */
+	obj = i915_gem_object_get(mmo->obj);
+	return i915_gem_object_mmap(obj, mmo, vma);
+}
+
 #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..196417fd0f5c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
@@ -29,5 +29,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_fb_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma);
 #endif
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper
  2023-03-20 10:09 [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Nirmoy Das
@ 2023-03-20 10:09 ` Nirmoy Das
  2023-03-20 10:39   ` Jani Nikula
                     ` (2 more replies)
  2023-03-20 10:09 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
  2023-03-20 14:02 ` [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Andrzej Hajda
  2 siblings, 3 replies; 12+ messages in thread
From: Nirmoy Das @ 2023-03-20 10:09 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, dri-devel, Matthew Auld, Andi Shyti, Nirmoy Das

Add a helper function to retrieve struct intel_fbdev from
struct drm_fb_helper.

Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Andi Shyti <andi.shyti@linux.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/display/intel_fbdev.c | 23 ++++++++++------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 673bcdfb7ff6..8c3b3c3fd0e0 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -67,6 +67,11 @@ struct intel_fbdev {
 	struct mutex hpd_lock;
 };
 
+static struct intel_fbdev *to_intel_fbdev(struct drm_fb_helper *fb_helper)
+{
+	return container_of(fb_helper, struct intel_fbdev, helper);
+}
+
 static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
 {
 	return ifbdev->fb->frontbuffer;
@@ -79,9 +84,7 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
 
 static int intel_fbdev_set_par(struct fb_info *info)
 {
-	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
+	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
 	int ret;
 
 	ret = drm_fb_helper_set_par(info);
@@ -93,9 +96,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
 
 static int intel_fbdev_blank(int blank, struct fb_info *info)
 {
-	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
+	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
 	int ret;
 
 	ret = drm_fb_helper_blank(blank, info);
@@ -108,9 +109,7 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
 static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
 				   struct fb_info *info)
 {
-	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
+	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
 	int ret;
 
 	ret = drm_fb_helper_pan_display(var, info);
@@ -136,8 +135,7 @@ static const struct fb_ops intelfb_ops = {
 static int intelfb_alloc(struct drm_fb_helper *helper,
 			 struct drm_fb_helper_surface_size *sizes)
 {
-	struct intel_fbdev *ifbdev =
-		container_of(helper, struct intel_fbdev, helper);
+	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
 	struct drm_framebuffer *fb;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -193,8 +191,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
-	struct intel_fbdev *ifbdev =
-		container_of(helper, struct intel_fbdev, helper);
+	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
 	struct intel_framebuffer *intel_fb = ifbdev->fb;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function
  2023-03-20 10:09 [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Nirmoy Das
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
@ 2023-03-20 10:09 ` Nirmoy Das
  2023-03-20 14:00   ` [Intel-gfx] " Andrzej Hajda
  2023-03-20 14:02 ` [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Andrzej Hajda
  2 siblings, 1 reply; 12+ messages in thread
From: Nirmoy Das @ 2023-03-20 10:09 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, dri-devel, Matthew Auld, Andi Shyti, 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 will
use GTT if aperture is available otherwise will use cpu to access
the framebuffer.

v2: Use to_intel_fbdev() function(Jani)

Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Andi Shyti <andi.shyti@linux.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>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 8c3b3c3fd0e0..5e52bef868a0 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"
@@ -119,6 +121,15 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
 	return ret;
 }
 
+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);
+
+	return i915_gem_fb_mmap(obj, vma);
+}
+
 static const struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	DRM_FB_HELPER_DEFAULT_OPS,
@@ -130,6 +141,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
@ 2023-03-20 10:39   ` Jani Nikula
  2023-03-20 10:49   ` Andi Shyti
  2023-03-20 13:58   ` [Intel-gfx] " Andrzej Hajda
  2 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2023-03-20 10:39 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: dri-devel, Matthew Auld, Andi Shyti, Nirmoy Das

On Mon, 20 Mar 2023, Nirmoy Das <nirmoy.das@intel.com> wrote:
> Add a helper function to retrieve struct intel_fbdev from
> struct drm_fb_helper.
>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.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>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 23 ++++++++++------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 673bcdfb7ff6..8c3b3c3fd0e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -67,6 +67,11 @@ struct intel_fbdev {
>  	struct mutex hpd_lock;
>  };
>  
> +static struct intel_fbdev *to_intel_fbdev(struct drm_fb_helper *fb_helper)
> +{
> +	return container_of(fb_helper, struct intel_fbdev, helper);
> +}
> +
>  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>  {
>  	return ifbdev->fb->frontbuffer;
> @@ -79,9 +84,7 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
>  
>  static int intel_fbdev_set_par(struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_set_par(info);
> @@ -93,9 +96,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
>  
>  static int intel_fbdev_blank(int blank, struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_blank(blank, info);
> @@ -108,9 +109,7 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
>  static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>  				   struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_pan_display(var, info);
> @@ -136,8 +135,7 @@ static const struct fb_ops intelfb_ops = {
>  static int intelfb_alloc(struct drm_fb_helper *helper,
>  			 struct drm_fb_helper_surface_size *sizes)
>  {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>  	struct drm_framebuffer *fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -193,8 +191,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>  	struct intel_framebuffer *intel_fb = ifbdev->fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
  2023-03-20 10:39   ` Jani Nikula
@ 2023-03-20 10:49   ` Andi Shyti
  2023-03-20 13:58   ` [Intel-gfx] " Andrzej Hajda
  2 siblings, 0 replies; 12+ messages in thread
From: Andi Shyti @ 2023-03-20 10:49 UTC (permalink / raw)
  To: Nirmoy Das; +Cc: Jani Nikula, intel-gfx, Matthew Auld, dri-devel, Andi Shyti

On Mon, Mar 20, 2023 at 11:09:02AM +0100, Nirmoy Das wrote:
> Add a helper function to retrieve struct intel_fbdev from
> struct drm_fb_helper.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.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>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

> ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 23 ++++++++++------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 673bcdfb7ff6..8c3b3c3fd0e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -67,6 +67,11 @@ struct intel_fbdev {
>  	struct mutex hpd_lock;
>  };
>  
> +static struct intel_fbdev *to_intel_fbdev(struct drm_fb_helper *fb_helper)
> +{
> +	return container_of(fb_helper, struct intel_fbdev, helper);
> +}
> +
>  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>  {
>  	return ifbdev->fb->frontbuffer;
> @@ -79,9 +84,7 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
>  
>  static int intel_fbdev_set_par(struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_set_par(info);
> @@ -93,9 +96,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
>  
>  static int intel_fbdev_blank(int blank, struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_blank(blank, info);
> @@ -108,9 +109,7 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
>  static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>  				   struct fb_info *info)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>  	int ret;
>  
>  	ret = drm_fb_helper_pan_display(var, info);
> @@ -136,8 +135,7 @@ static const struct fb_ops intelfb_ops = {
>  static int intelfb_alloc(struct drm_fb_helper *helper,
>  			 struct drm_fb_helper_surface_size *sizes)
>  {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>  	struct drm_framebuffer *fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -193,8 +191,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>  	struct intel_framebuffer *intel_fb = ifbdev->fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Intel-gfx] [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
  2023-03-20 10:39   ` Jani Nikula
  2023-03-20 10:49   ` Andi Shyti
@ 2023-03-20 13:58   ` Andrzej Hajda
  2 siblings, 0 replies; 12+ messages in thread
From: Andrzej Hajda @ 2023-03-20 13:58 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: Jani Nikula, Matthew Auld, dri-devel

On 20.03.2023 11:09, Nirmoy Das wrote:
> Add a helper function to retrieve struct intel_fbdev from
> struct drm_fb_helper.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.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>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/display/intel_fbdev.c | 23 ++++++++++------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 673bcdfb7ff6..8c3b3c3fd0e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -67,6 +67,11 @@ struct intel_fbdev {
>   	struct mutex hpd_lock;
>   };
>   
> +static struct intel_fbdev *to_intel_fbdev(struct drm_fb_helper *fb_helper)
> +{
> +	return container_of(fb_helper, struct intel_fbdev, helper);
> +}
> +
>   static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>   {
>   	return ifbdev->fb->frontbuffer;
> @@ -79,9 +84,7 @@ static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
>   
>   static int intel_fbdev_set_par(struct fb_info *info)
>   {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>   	int ret;
>   
>   	ret = drm_fb_helper_set_par(info);
> @@ -93,9 +96,7 @@ static int intel_fbdev_set_par(struct fb_info *info)
>   
>   static int intel_fbdev_blank(int blank, struct fb_info *info)
>   {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>   	int ret;
>   
>   	ret = drm_fb_helper_blank(blank, info);
> @@ -108,9 +109,7 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
>   static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>   				   struct fb_info *info)
>   {
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct intel_fbdev *ifbdev =
> -		container_of(fb_helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(info->par);
>   	int ret;
>   
>   	ret = drm_fb_helper_pan_display(var, info);
> @@ -136,8 +135,7 @@ static const struct fb_ops intelfb_ops = {
>   static int intelfb_alloc(struct drm_fb_helper *helper,
>   			 struct drm_fb_helper_surface_size *sizes)
>   {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>   	struct drm_framebuffer *fb;
>   	struct drm_device *dev = helper->dev;
>   	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -193,8 +191,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>   static int intelfb_create(struct drm_fb_helper *helper,
>   			  struct drm_fb_helper_surface_size *sizes)
>   {
> -	struct intel_fbdev *ifbdev =
> -		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
>   	struct intel_framebuffer *intel_fb = ifbdev->fb;
>   	struct drm_device *dev = helper->dev;
>   	struct drm_i915_private *dev_priv = to_i915(dev);


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Intel-gfx] [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function
  2023-03-20 10:09 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
@ 2023-03-20 14:00   ` Andrzej Hajda
  0 siblings, 0 replies; 12+ messages in thread
From: Andrzej Hajda @ 2023-03-20 14:00 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: Jani Nikula, Matthew Auld, dri-devel

On 20.03.2023 11:09, 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 will
> use GTT if aperture is available otherwise will use cpu to access
> the framebuffer.
> 
> v2: Use to_intel_fbdev() function(Jani)
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.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>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/display/intel_fbdev.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 8c3b3c3fd0e0..5e52bef868a0 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"
> @@ -119,6 +121,15 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>   	return ret;
>   }
>   
> +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);
> +
> +	return i915_gem_fb_mmap(obj, vma);
> +}
> +
>   static const struct fb_ops intelfb_ops = {
>   	.owner = THIS_MODULE,
>   	DRM_FB_HELPER_DEFAULT_OPS,
> @@ -130,6 +141,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,


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj
  2023-03-20 10:09 [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Nirmoy Das
  2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
  2023-03-20 10:09 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
@ 2023-03-20 14:02 ` Andrzej Hajda
  2023-03-23  8:00   ` Das, Nirmoy
  2 siblings, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2023-03-20 14:02 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: Jani Nikula, Matthew Auld, dri-devel

On 20.03.2023 11:09, Nirmoy Das wrote:
> Implement i915_gem_fb_mmap() to enable fb_ops.fb_mmap()
> callback for i915's framebuffer objects.
> 
> v2: add a comment why i915_gem_object_get() needed(Andi).
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.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>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 127 +++++++++++++++--------
>   drivers/gpu/drm/i915/gem/i915_gem_mman.h |   2 +-
>   2 files changed, 83 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index d3c1dee16af2..341e952d3510 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -927,53 +927,15 @@ static struct file *mmap_singleton(struct drm_i915_private *i915)
>   	return file;
>   }
>   
> -/*
> - * This overcomes the limitation in drm_gem_mmap's assignment of a
> - * drm_gem_object as the vma->vm_private_data. Since we need to
> - * be able to resolve multiple mmap offsets which could be tied
> - * to a single gem object.
> - */
> -int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +static int
> +i915_gem_object_mmap(struct drm_i915_gem_object *obj,
> +		     struct i915_mmap_offset *mmo,
> +		     struct vm_area_struct *vma)
>   {
> -	struct drm_vma_offset_node *node;
> -	struct drm_file *priv = filp->private_data;
> -	struct drm_device *dev = priv->minor->dev;
> -	struct drm_i915_gem_object *obj = NULL;
> -	struct i915_mmap_offset *mmo = NULL;
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct drm_device *dev = &i915->drm;
>   	struct file *anon;
>   
> -	if (drm_dev_is_unplugged(dev))
> -		return -ENODEV;
> -
> -	rcu_read_lock();
> -	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> -	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> -						  vma->vm_pgoff,
> -						  vma_pages(vma));
> -	if (node && drm_vma_node_is_allowed(node, priv)) {
> -		/*
> -		 * Skip 0-refcnted objects as it is in the process of being
> -		 * destroyed and will be invalid when the vma manager lock
> -		 * is released.
> -		 */
> -		if (!node->driver_private) {
> -			mmo = container_of(node, struct i915_mmap_offset, vma_node);
> -			obj = i915_gem_object_get_rcu(mmo->obj);
> -
> -			GEM_BUG_ON(obj && obj->ops->mmap_ops);
> -		} else {
> -			obj = i915_gem_object_get_rcu
> -				(container_of(node, struct drm_i915_gem_object,
> -					      base.vma_node));
> -
> -			GEM_BUG_ON(obj && !obj->ops->mmap_ops);
> -		}
> -	}
> -	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> -	rcu_read_unlock();
> -	if (!obj)
> -		return node ? -EACCES : -EINVAL;
> -
>   	if (i915_gem_object_is_readonly(obj)) {
>   		if (vma->vm_flags & VM_WRITE) {
>   			i915_gem_object_put(obj);
> @@ -1005,7 +967,7 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>   	if (obj->ops->mmap_ops) {
>   		vma->vm_page_prot = pgprot_decrypted(vm_get_page_prot(vma->vm_flags));
>   		vma->vm_ops = obj->ops->mmap_ops;
> -		vma->vm_private_data = node->driver_private;
> +		vma->vm_private_data = obj->base.vma_node.driver_private;
>   		return 0;
>   	}
>   
> @@ -1043,6 +1005,81 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>   	return 0;
>   }
>   
> +/*
> + * This overcomes the limitation in drm_gem_mmap's assignment of a
> + * drm_gem_object as the vma->vm_private_data. Since we need to
> + * be able to resolve multiple mmap offsets which could be tied
> + * to a single gem object.
> + */
> +int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct drm_vma_offset_node *node;
> +	struct drm_file *priv = filp->private_data;
> +	struct drm_device *dev = priv->minor->dev;
> +	struct drm_i915_gem_object *obj = NULL;
> +	struct i915_mmap_offset *mmo = NULL;
> +
> +	if (drm_dev_is_unplugged(dev))
> +		return -ENODEV;
> +
> +	rcu_read_lock();
> +	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> +	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> +						  vma->vm_pgoff,
> +						  vma_pages(vma));
> +	if (node && drm_vma_node_is_allowed(node, priv)) {
> +		/*
> +		 * Skip 0-refcnted objects as it is in the process of being
> +		 * destroyed and will be invalid when the vma manager lock
> +		 * is released.
> +		 */
> +		if (!node->driver_private) {
> +			mmo = container_of(node, struct i915_mmap_offset, vma_node);
> +			obj = i915_gem_object_get_rcu(mmo->obj);
> +
> +			GEM_BUG_ON(obj && obj->ops->mmap_ops);
> +		} else {
> +			obj = i915_gem_object_get_rcu
> +				(container_of(node, struct drm_i915_gem_object,
> +					      base.vma_node));
> +
> +			GEM_BUG_ON(obj && !obj->ops->mmap_ops);
> +		}
> +	}
> +	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> +	rcu_read_unlock();
> +	if (!obj)
> +		return node ? -EACCES : -EINVAL;
> +
> +	return i915_gem_object_mmap(obj, mmo, vma);
> +}
> +
> +int i915_gem_fb_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct drm_device *dev = &i915->drm;
> +	struct i915_mmap_offset *mmo = NULL;
> +	enum i915_mmap_type mmap_type;
> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> +
> +	if (drm_dev_is_unplugged(dev))
> +		return -ENODEV;
> +
> +	mmap_type = i915_ggtt_has_aperture(ggtt) ? I915_MMAP_TYPE_GTT : I915_MMAP_TYPE_WC;
> +	mmo = mmap_offset_attach(obj, mmap_type, NULL);
> +	if (!mmo)
> +		return -ENODEV;
> +
> +	/*
> +	 * When we install vm_ops for mmap we are too late for
> +	 * the vm_ops->open() which increases the ref_count of
> +	 * this obj and then it gets decreased by the vm_ops->close().
> +	 * To balance this increase the obj ref_count here.
> +	 */
> +	obj = i915_gem_object_get(mmo->obj);
> +	return i915_gem_object_mmap(obj, mmo, vma);
> +}
> +
>   #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..196417fd0f5c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> @@ -29,5 +29,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_fb_mmap(struct drm_i915_gem_object *obj, struct vm_area_struct *vma);
>   #endif


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj
  2023-03-20 14:02 ` [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Andrzej Hajda
@ 2023-03-23  8:00   ` Das, Nirmoy
  0 siblings, 0 replies; 12+ messages in thread
From: Das, Nirmoy @ 2023-03-23  8:00 UTC (permalink / raw)
  To: Andrzej Hajda, Nirmoy Das, intel-gfx; +Cc: Jani Nikula, Matthew Auld, dri-devel


On 3/20/2023 3:02 PM, Andrzej Hajda wrote:
> On 20.03.2023 11:09, Nirmoy Das wrote:
>> Implement i915_gem_fb_mmap() to enable fb_ops.fb_mmap()
>> callback for i915's framebuffer objects.
>>
>> v2: add a comment why i915_gem_object_get() needed(Andi).
>>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.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>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>


Thanks, Andrzej.


Going to resend it without RFC now as there are two r-bs and no one 
complained.


Regards,

Nirmoy

>
> Regards
> Andrzej
>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 127 +++++++++++++++--------
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.h |   2 +-
>>   2 files changed, 83 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index d3c1dee16af2..341e952d3510 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -927,53 +927,15 @@ static struct file *mmap_singleton(struct 
>> drm_i915_private *i915)
>>       return file;
>>   }
>>   -/*
>> - * This overcomes the limitation in drm_gem_mmap's assignment of a
>> - * drm_gem_object as the vma->vm_private_data. Since we need to
>> - * be able to resolve multiple mmap offsets which could be tied
>> - * to a single gem object.
>> - */
>> -int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> +static int
>> +i915_gem_object_mmap(struct drm_i915_gem_object *obj,
>> +             struct i915_mmap_offset *mmo,
>> +             struct vm_area_struct *vma)
>>   {
>> -    struct drm_vma_offset_node *node;
>> -    struct drm_file *priv = filp->private_data;
>> -    struct drm_device *dev = priv->minor->dev;
>> -    struct drm_i915_gem_object *obj = NULL;
>> -    struct i915_mmap_offset *mmo = NULL;
>> +    struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> +    struct drm_device *dev = &i915->drm;
>>       struct file *anon;
>>   -    if (drm_dev_is_unplugged(dev))
>> -        return -ENODEV;
>> -
>> -    rcu_read_lock();
>> -    drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>> -    node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>> -                          vma->vm_pgoff,
>> -                          vma_pages(vma));
>> -    if (node && drm_vma_node_is_allowed(node, priv)) {
>> -        /*
>> -         * Skip 0-refcnted objects as it is in the process of being
>> -         * destroyed and will be invalid when the vma manager lock
>> -         * is released.
>> -         */
>> -        if (!node->driver_private) {
>> -            mmo = container_of(node, struct i915_mmap_offset, 
>> vma_node);
>> -            obj = i915_gem_object_get_rcu(mmo->obj);
>> -
>> -            GEM_BUG_ON(obj && obj->ops->mmap_ops);
>> -        } else {
>> -            obj = i915_gem_object_get_rcu
>> -                (container_of(node, struct drm_i915_gem_object,
>> -                          base.vma_node));
>> -
>> -            GEM_BUG_ON(obj && !obj->ops->mmap_ops);
>> -        }
>> -    }
>> -    drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>> -    rcu_read_unlock();
>> -    if (!obj)
>> -        return node ? -EACCES : -EINVAL;
>> -
>>       if (i915_gem_object_is_readonly(obj)) {
>>           if (vma->vm_flags & VM_WRITE) {
>>               i915_gem_object_put(obj);
>> @@ -1005,7 +967,7 @@ int i915_gem_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>       if (obj->ops->mmap_ops) {
>>           vma->vm_page_prot = 
>> pgprot_decrypted(vm_get_page_prot(vma->vm_flags));
>>           vma->vm_ops = obj->ops->mmap_ops;
>> -        vma->vm_private_data = node->driver_private;
>> +        vma->vm_private_data = obj->base.vma_node.driver_private;
>>           return 0;
>>       }
>>   @@ -1043,6 +1005,81 @@ int i915_gem_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>       return 0;
>>   }
>>   +/*
>> + * This overcomes the limitation in drm_gem_mmap's assignment of a
>> + * drm_gem_object as the vma->vm_private_data. Since we need to
>> + * be able to resolve multiple mmap offsets which could be tied
>> + * to a single gem object.
>> + */
>> +int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> +    struct drm_vma_offset_node *node;
>> +    struct drm_file *priv = filp->private_data;
>> +    struct drm_device *dev = priv->minor->dev;
>> +    struct drm_i915_gem_object *obj = NULL;
>> +    struct i915_mmap_offset *mmo = NULL;
>> +
>> +    if (drm_dev_is_unplugged(dev))
>> +        return -ENODEV;
>> +
>> +    rcu_read_lock();
>> +    drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>> +    node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>> +                          vma->vm_pgoff,
>> +                          vma_pages(vma));
>> +    if (node && drm_vma_node_is_allowed(node, priv)) {
>> +        /*
>> +         * Skip 0-refcnted objects as it is in the process of being
>> +         * destroyed and will be invalid when the vma manager lock
>> +         * is released.
>> +         */
>> +        if (!node->driver_private) {
>> +            mmo = container_of(node, struct i915_mmap_offset, 
>> vma_node);
>> +            obj = i915_gem_object_get_rcu(mmo->obj);
>> +
>> +            GEM_BUG_ON(obj && obj->ops->mmap_ops);
>> +        } else {
>> +            obj = i915_gem_object_get_rcu
>> +                (container_of(node, struct drm_i915_gem_object,
>> +                          base.vma_node));
>> +
>> +            GEM_BUG_ON(obj && !obj->ops->mmap_ops);
>> +        }
>> +    }
>> +    drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>> +    rcu_read_unlock();
>> +    if (!obj)
>> +        return node ? -EACCES : -EINVAL;
>> +
>> +    return i915_gem_object_mmap(obj, mmo, vma);
>> +}
>> +
>> +int i915_gem_fb_mmap(struct drm_i915_gem_object *obj, struct 
>> vm_area_struct *vma)
>> +{
>> +    struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> +    struct drm_device *dev = &i915->drm;
>> +    struct i915_mmap_offset *mmo = NULL;
>> +    enum i915_mmap_type mmap_type;
>> +    struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
>> +
>> +    if (drm_dev_is_unplugged(dev))
>> +        return -ENODEV;
>> +
>> +    mmap_type = i915_ggtt_has_aperture(ggtt) ? I915_MMAP_TYPE_GTT : 
>> I915_MMAP_TYPE_WC;
>> +    mmo = mmap_offset_attach(obj, mmap_type, NULL);
>> +    if (!mmo)
>> +        return -ENODEV;
>> +
>> +    /*
>> +     * When we install vm_ops for mmap we are too late for
>> +     * the vm_ops->open() which increases the ref_count of
>> +     * this obj and then it gets decreased by the vm_ops->close().
>> +     * To balance this increase the obj ref_count here.
>> +     */
>> +    obj = i915_gem_object_get(mmo->obj);
>> +    return i915_gem_object_mmap(obj, mmo, vma);
>> +}
>> +
>>   #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..196417fd0f5c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
>> @@ -29,5 +29,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_fb_mmap(struct drm_i915_gem_object *obj, struct 
>> vm_area_struct *vma);
>>   #endif
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function
  2023-03-06 14:32   ` Ville Syrjälä
@ 2023-03-07 14:50     ` Das, Nirmoy
  0 siblings, 0 replies; 12+ messages in thread
From: Das, Nirmoy @ 2023-03-07 14:50 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, dri-devel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function
  2023-03-06 10:28 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
@ 2023-03-06 14:32   ` Ville Syrjälä
  2023-03-07 14:50     ` Das, Nirmoy
  0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2023-03-06 14:32 UTC (permalink / raw)
  To: Nirmoy Das; +Cc: intel-gfx, dri-devel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function
  2023-03-06 10:28 [PATCH 1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer Nirmoy Das
@ 2023-03-06 10:28 ` Nirmoy Das
  2023-03-06 14:32   ` Ville Syrjälä
  0 siblings, 1 reply; 12+ messages in thread
From: Nirmoy Das @ 2023-03-06 10:28 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-03-23  8:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 10:09 [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Nirmoy Das
2023-03-20 10:09 ` [PATCH 2/3] drm/i915/display: Add helper func to get intel_fbdev from drm_fb_helper Nirmoy Das
2023-03-20 10:39   ` Jani Nikula
2023-03-20 10:49   ` Andi Shyti
2023-03-20 13:58   ` [Intel-gfx] " Andrzej Hajda
2023-03-20 10:09 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
2023-03-20 14:00   ` [Intel-gfx] " Andrzej Hajda
2023-03-20 14:02 ` [Intel-gfx] [PATCH v2: 1/3] drm/i915: Add a function to mmap framebuffer obj Andrzej Hajda
2023-03-23  8:00   ` Das, Nirmoy
  -- strict thread matches above, loose matches on Subject: below --
2023-03-06 10:28 [PATCH 1/3] drm/i915: Set I915_BO_ALLOC_USER for framebuffer Nirmoy Das
2023-03-06 10:28 ` [PATCH RFC 3/3] drm/i915/display: Implement fb_mmap callback function Nirmoy Das
2023-03-06 14:32   ` Ville Syrjälä
2023-03-07 14:50     ` Das, Nirmoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).