All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/vkms: Set preferred depth correctly
@ 2020-10-09 23:21 Daniel Vetter
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
                   ` (4 more replies)
  0 siblings, 5 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-09 23:21 UTC (permalink / raw)
  To: DRI Development
  Cc: Haneen Mohammed, Rodrigo Siqueira, Daniel Vetter, Melissa Wen,
	Daniel Vetter

The only thing we support is xrgb8888.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Melissa Wen <melissa.srw@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 726801ab44d4..eb4007443706 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	dev->mode_config.max_height = YRES_MAX;
 	dev->mode_config.cursor_width = 512;
 	dev->mode_config.cursor_height = 512;
-	dev->mode_config.preferred_depth = 24;
+	dev->mode_config.preferred_depth = 32;
 	dev->mode_config.helper_private = &vkms_mode_config_helpers;
 
 	return vkms_output_init(vkmsdev, 0);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* [PATCH 2/3] drm/vkms: Switch to shmem helpers
  2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
@ 2020-10-09 23:21 ` Daniel Vetter
  2020-10-12 10:59   ` Chris Wilson
                     ` (3 more replies)
  2020-10-09 23:21 ` [PATCH 3/3] drm/vkms: fbdev emulation support Daniel Vetter
                   ` (3 subsequent siblings)
  4 siblings, 4 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-09 23:21 UTC (permalink / raw)
  To: DRI Development
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, Daniel Vetter,
	Chris Wilson, Melissa Wen, Thomas Zimmermann, Daniel Vetter

Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
at the gem bo level, which we need for generic fbdev emulation.

Luckily shmem also tracks ->vaddr, so we just need to adjust the code
all over a bit to make this fit.

Also wire up handle_to_fd, dunno why that was missing.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Melissa Wen <melissa.srw@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/Kconfig               |   1 +
 drivers/gpu/drm/vkms/Makefile         |   1 -
 drivers/gpu/drm/vkms/vkms_composer.c  |  17 +-
 drivers/gpu/drm/vkms/vkms_drv.c       |  19 +-
 drivers/gpu/drm/vkms/vkms_drv.h       |  26 ---
 drivers/gpu/drm/vkms/vkms_gem.c       | 261 --------------------------
 drivers/gpu/drm/vkms/vkms_plane.c     |  13 +-
 drivers/gpu/drm/vkms/vkms_writeback.c |  17 +-
 8 files changed, 32 insertions(+), 323 deletions(-)
 delete mode 100644 drivers/gpu/drm/vkms/vkms_gem.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 9efb82caaa87..b796c118fc3b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -287,6 +287,7 @@ config DRM_VKMS
 	tristate "Virtual KMS (EXPERIMENTAL)"
 	depends on DRM
 	select DRM_KMS_HELPER
+	select DRM_GEM_SHMEM_HELPER
 	select CRC32
 	default n
 	help
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 333d3cead0e3..72f779cbfedd 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -4,7 +4,6 @@ vkms-y := \
 	vkms_plane.o \
 	vkms_output.o \
 	vkms_crtc.o \
-	vkms_gem.o \
 	vkms_composer.o \
 	vkms_writeback.o
 
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 33c031f27c2c..66c6842d70db 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -5,6 +5,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "vkms_drv.h"
@@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
 			   void *vaddr_out)
 {
 	struct drm_gem_object *cursor_obj;
-	struct vkms_gem_object *cursor_vkms_obj;
+	struct drm_gem_shmem_object *cursor_shmem_obj;
 
 	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
-	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
+	cursor_shmem_obj = to_drm_gem_shmem_obj(cursor_obj);
 
-	if (WARN_ON(!cursor_vkms_obj->vaddr))
+	if (WARN_ON(!cursor_shmem_obj->vaddr))
 		return;
 
-	blend(vaddr_out, cursor_vkms_obj->vaddr,
+	blend(vaddr_out, cursor_shmem_obj->vaddr,
 	      primary_composer, cursor_composer);
 }
 
@@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
 {
 	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
 
 	if (!*vaddr_out) {
-		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
+		*vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
 		if (!*vaddr_out) {
 			DRM_ERROR("Cannot allocate memory for output frame.");
 			return -ENOMEM;
 		}
 	}
 
-	if (WARN_ON(!vkms_obj->vaddr))
+	if (WARN_ON(!shmem_obj->vaddr))
 		return -EINVAL;
 
-	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
+	memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
 
 	if (cursor_composer)
 		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index eb4007443706..6221e5040264 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -23,6 +23,7 @@
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "vkms_drv.h"
@@ -39,17 +40,7 @@ bool enable_cursor = true;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
 
-static const struct file_operations vkms_driver_fops = {
-	.owner		= THIS_MODULE,
-	.open		= drm_open,
-	.mmap		= drm_gem_mmap,
-	.unlocked_ioctl	= drm_ioctl,
-	.compat_ioctl	= drm_compat_ioctl,
-	.poll		= drm_poll,
-	.read		= drm_read,
-	.llseek		= no_llseek,
-	.release	= drm_release,
-};
+DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
 static void vkms_release(struct drm_device *dev)
 {
@@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
 	.release		= vkms_release,
 	.fops			= &vkms_driver_fops,
-	.dumb_create		= vkms_dumb_create,
+	.dumb_create		= drm_gem_shmem_dumb_create,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
-	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
+	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
+	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
+	.gem_prime_mmap		= drm_gem_prime_mmap,
 
 	.name			= DRIVER_NAME,
 	.desc			= DRIVER_DESC,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 380a8f27e156..ef6482e3adea 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -88,14 +88,6 @@ struct vkms_device {
 	struct vkms_output output;
 };
 
-struct vkms_gem_object {
-	struct drm_gem_object gem;
-	struct mutex pages_lock; /* Page lock used in page fault handler */
-	struct page **pages;
-	unsigned int vmap_count;
-	void *vaddr;
-};
-
 #define drm_crtc_to_vkms_output(target) \
 	container_of(target, struct vkms_output, crtc)
 
@@ -120,24 +112,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index);
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 				  enum drm_plane_type type, int index);
 
-/* Gem stuff */
-vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
-
-int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
-		     struct drm_mode_create_dumb *args);
-
-void vkms_gem_free_object(struct drm_gem_object *obj);
-
-int vkms_gem_vmap(struct drm_gem_object *obj);
-
-void vkms_gem_vunmap(struct drm_gem_object *obj);
-
-/* Prime */
-struct drm_gem_object *
-vkms_prime_import_sg_table(struct drm_device *dev,
-			   struct dma_buf_attachment *attach,
-			   struct sg_table *sg);
-
 /* CRC Support */
 const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
 					size_t *count);
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
deleted file mode 100644
index 19a0e260a4df..000000000000
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ /dev/null
@@ -1,261 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-
-#include <linux/dma-buf.h>
-#include <linux/shmem_fs.h>
-#include <linux/vmalloc.h>
-#include <drm/drm_prime.h>
-
-#include "vkms_drv.h"
-
-static const struct vm_operations_struct vkms_gem_vm_ops = {
-	.fault = vkms_gem_fault,
-	.open = drm_gem_vm_open,
-	.close = drm_gem_vm_close,
-};
-
-static const struct drm_gem_object_funcs vkms_gem_object_funcs = {
-	.free = vkms_gem_free_object,
-	.vm_ops = &vkms_gem_vm_ops,
-};
-
-static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
-						 u64 size)
-{
-	struct vkms_gem_object *obj;
-	int ret;
-
-	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
-	if (!obj)
-		return ERR_PTR(-ENOMEM);
-
-	obj->gem.funcs = &vkms_gem_object_funcs;
-
-	size = roundup(size, PAGE_SIZE);
-	ret = drm_gem_object_init(dev, &obj->gem, size);
-	if (ret) {
-		kfree(obj);
-		return ERR_PTR(ret);
-	}
-
-	mutex_init(&obj->pages_lock);
-
-	return obj;
-}
-
-void vkms_gem_free_object(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
-						   gem);
-
-	WARN_ON(gem->pages);
-	WARN_ON(gem->vaddr);
-
-	mutex_destroy(&gem->pages_lock);
-	drm_gem_object_release(obj);
-	kfree(gem);
-}
-
-vm_fault_t vkms_gem_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct vkms_gem_object *obj = vma->vm_private_data;
-	unsigned long vaddr = vmf->address;
-	pgoff_t page_offset;
-	loff_t num_pages;
-	vm_fault_t ret = VM_FAULT_SIGBUS;
-
-	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
-	num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);
-
-	if (page_offset > num_pages)
-		return VM_FAULT_SIGBUS;
-
-	mutex_lock(&obj->pages_lock);
-	if (obj->pages) {
-		get_page(obj->pages[page_offset]);
-		vmf->page = obj->pages[page_offset];
-		ret = 0;
-	}
-	mutex_unlock(&obj->pages_lock);
-	if (ret) {
-		struct page *page;
-		struct address_space *mapping;
-
-		mapping = file_inode(obj->gem.filp)->i_mapping;
-		page = shmem_read_mapping_page(mapping, page_offset);
-
-		if (!IS_ERR(page)) {
-			vmf->page = page;
-			ret = 0;
-		} else {
-			switch (PTR_ERR(page)) {
-			case -ENOSPC:
-			case -ENOMEM:
-				ret = VM_FAULT_OOM;
-				break;
-			case -EBUSY:
-				ret = VM_FAULT_RETRY;
-				break;
-			case -EFAULT:
-			case -EINVAL:
-				ret = VM_FAULT_SIGBUS;
-				break;
-			default:
-				WARN_ON(PTR_ERR(page));
-				ret = VM_FAULT_SIGBUS;
-				break;
-			}
-		}
-	}
-	return ret;
-}
-
-static struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
-					      struct drm_file *file,
-					      u32 *handle,
-					      u64 size)
-{
-	struct vkms_gem_object *obj;
-	int ret;
-
-	if (!file || !dev || !handle)
-		return ERR_PTR(-EINVAL);
-
-	obj = __vkms_gem_create(dev, size);
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	ret = drm_gem_handle_create(file, &obj->gem, handle);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return &obj->gem;
-}
-
-int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
-		     struct drm_mode_create_dumb *args)
-{
-	struct drm_gem_object *gem_obj;
-	u64 pitch, size;
-
-	if (!args || !dev || !file)
-		return -EINVAL;
-
-	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
-	size = pitch * args->height;
-
-	if (!size)
-		return -EINVAL;
-
-	gem_obj = vkms_gem_create(dev, file, &args->handle, size);
-	if (IS_ERR(gem_obj))
-		return PTR_ERR(gem_obj);
-
-	args->size = gem_obj->size;
-	args->pitch = pitch;
-
-	drm_gem_object_put(gem_obj);
-
-	DRM_DEBUG_DRIVER("Created object of size %lld\n", size);
-
-	return 0;
-}
-
-static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
-{
-	struct drm_gem_object *gem_obj = &vkms_obj->gem;
-
-	if (!vkms_obj->pages) {
-		struct page **pages = drm_gem_get_pages(gem_obj);
-
-		if (IS_ERR(pages))
-			return pages;
-
-		if (cmpxchg(&vkms_obj->pages, NULL, pages))
-			drm_gem_put_pages(gem_obj, pages, false, true);
-	}
-
-	return vkms_obj->pages;
-}
-
-void vkms_gem_vunmap(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
-
-	mutex_lock(&vkms_obj->pages_lock);
-	if (vkms_obj->vmap_count < 1) {
-		WARN_ON(vkms_obj->vaddr);
-		WARN_ON(vkms_obj->pages);
-		mutex_unlock(&vkms_obj->pages_lock);
-		return;
-	}
-
-	vkms_obj->vmap_count--;
-
-	if (vkms_obj->vmap_count == 0) {
-		vunmap(vkms_obj->vaddr);
-		vkms_obj->vaddr = NULL;
-		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
-		vkms_obj->pages = NULL;
-	}
-
-	mutex_unlock(&vkms_obj->pages_lock);
-}
-
-int vkms_gem_vmap(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
-	int ret = 0;
-
-	mutex_lock(&vkms_obj->pages_lock);
-
-	if (!vkms_obj->vaddr) {
-		unsigned int n_pages = obj->size >> PAGE_SHIFT;
-		struct page **pages = _get_pages(vkms_obj);
-
-		if (IS_ERR(pages)) {
-			ret = PTR_ERR(pages);
-			goto out;
-		}
-
-		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
-		if (!vkms_obj->vaddr)
-			goto err_vmap;
-	}
-
-	vkms_obj->vmap_count++;
-	goto out;
-
-err_vmap:
-	ret = -ENOMEM;
-	drm_gem_put_pages(obj, vkms_obj->pages, false, true);
-	vkms_obj->pages = NULL;
-out:
-	mutex_unlock(&vkms_obj->pages_lock);
-	return ret;
-}
-
-struct drm_gem_object *
-vkms_prime_import_sg_table(struct drm_device *dev,
-			   struct dma_buf_attachment *attach,
-			   struct sg_table *sg)
-{
-	struct vkms_gem_object *obj;
-	int npages;
-
-	obj = __vkms_gem_create(dev, attach->dmabuf->size);
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
-	DRM_DEBUG_PRIME("Importing %d pages\n", npages);
-
-	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
-	if (!obj->pages) {
-		vkms_gem_free_object(&obj->gem);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL, npages);
-	return &obj->gem;
-}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 6d31265a2ab7..9890137bcb8d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -5,6 +5,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 
 #include "vkms_drv.h"
 
@@ -145,15 +146,15 @@ static int vkms_prepare_fb(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
 	struct drm_gem_object *gem_obj;
-	int ret;
+	void *vaddr;
 
 	if (!state->fb)
 		return 0;
 
 	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
-	ret = vkms_gem_vmap(gem_obj);
-	if (ret)
-		DRM_ERROR("vmap failed: %d\n", ret);
+	vaddr = drm_gem_shmem_vmap(gem_obj);
+	if (IS_ERR(vaddr))
+		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
 
 	return drm_gem_fb_prepare_fb(plane, state);
 }
@@ -162,12 +163,14 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
 			    struct drm_plane_state *old_state)
 {
 	struct drm_gem_object *gem_obj;
+	struct drm_gem_shmem_object *shmem_obj;
 
 	if (!old_state->fb)
 		return;
 
 	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
-	vkms_gem_vunmap(gem_obj);
+	shmem_obj = to_drm_gem_shmem_obj(drm_gem_fb_get_obj(old_state->fb, 0));
+	drm_gem_shmem_vunmap(gem_obj, shmem_obj->vaddr);
 }
 
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 094fa4aa061d..26b903926872 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -6,6 +6,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 
 static const u32 vkms_wb_formats[] = {
 	DRM_FORMAT_XRGB8888,
@@ -63,22 +64,20 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
 static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 			       struct drm_writeback_job *job)
 {
-	struct vkms_gem_object *vkms_obj;
 	struct drm_gem_object *gem_obj;
-	int ret;
+	void *vaddr;
 
 	if (!job->fb)
 		return 0;
 
 	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	ret = vkms_gem_vmap(gem_obj);
-	if (ret) {
-		DRM_ERROR("vmap failed: %d\n", ret);
-		return ret;
+	vaddr = drm_gem_shmem_vmap(gem_obj);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
+		return PTR_ERR(vaddr);
 	}
 
-	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
-	job->priv = vkms_obj->vaddr;
+	job->priv = vaddr;
 
 	return 0;
 }
@@ -93,7 +92,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 		return;
 
 	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	vkms_gem_vunmap(gem_obj);
+	drm_gem_shmem_vunmap(gem_obj, job->priv);
 
 	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
 	vkms_set_composer(&vkmsdev->output, false);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
@ 2020-10-09 23:21 ` Daniel Vetter
  2020-10-12 11:24   ` Thomas Zimmermann
  2020-10-16 10:10   ` Melissa Wen
  2020-10-12 11:08 ` [PATCH 1/3] drm/vkms: Set preferred depth correctly Thomas Zimmermann
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-09 23:21 UTC (permalink / raw)
  To: DRI Development
  Cc: Haneen Mohammed, Rodrigo Siqueira, Daniel Vetter, Melissa Wen,
	Daniel Vetter

Hooray for generic fbdev support, making this a oneliner. We just
needed to fix preferred_depth fixed and the vmap support added first.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Melissa Wen <melissa.srw@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 6221e5040264..cc09e2df5cb1 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -169,6 +169,8 @@ static int __init vkms_init(void)
 	if (ret)
 		goto out_devres;
 
+	drm_fbdev_generic_setup(&vkms_device->drm, 0);
+
 	return 0;
 
 out_devres:
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 2/3] drm/vkms: Switch to shmem helpers
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
@ 2020-10-12 10:59   ` Chris Wilson
  2020-10-12 11:20     ` Thomas Zimmermann
  2020-10-12 11:22   ` Thomas Zimmermann
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2020-10-12 10:59 UTC (permalink / raw)
  To: DRI Development, Daniel Vetter
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, Daniel Vetter,
	Melissa Wen, Thomas Zimmermann, Daniel Vetter

Quoting Daniel Vetter (2020-10-10 00:21:55)
> Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
> at the gem bo level, which we need for generic fbdev emulation.
> 
> Luckily shmem also tracks ->vaddr, so we just need to adjust the code
> all over a bit to make this fit.
> 
> Also wire up handle_to_fd, dunno why that was missing.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 33c031f27c2c..66c6842d70db 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>

>  static void vkms_release(struct drm_device *dev)
>  {
> @@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
>         .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
>         .release                = vkms_release,
>         .fops                   = &vkms_driver_fops,
> -       .dumb_create            = vkms_dumb_create,
> +       .dumb_create            = drm_gem_shmem_dumb_create,

Something worth pointing out is that will create an uncached (well WC)
buffer, but since it is being exported with prime, that is probably for
the better. It might be worth using a memcpy_from_wc() for writeback/CRC
calculations. E.g.

> @@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
>                            void *vaddr_out)
>  {
> +       blend(vaddr_out, cursor_shmem_obj->vaddr,
>               primary_composer, cursor_composer);
>  }
>  
> @@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
>  {
> +       memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);

would benefit from WC accessors.

On the other hand, if the load is so small no one notices, it can wait.

Do we have anything that uses vkms in CI?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
  2020-10-09 23:21 ` [PATCH 3/3] drm/vkms: fbdev emulation support Daniel Vetter
@ 2020-10-12 11:08 ` Thomas Zimmermann
  2020-10-12 12:59 ` Melissa Wen
  2020-10-16 10:38 ` Simon Ser
  4 siblings, 0 replies; 29+ messages in thread
From: Thomas Zimmermann @ 2020-10-12 11:08 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Melissa Wen, Haneen Mohammed, Rodrigo Siqueira, DRI Development,
	Daniel Vetter

On Sat, 10 Oct 2020 01:21:54 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The only thing we support is xrgb8888.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 726801ab44d4..eb4007443706 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.max_height = YRES_MAX;
>  	dev->mode_config.cursor_width = 512;
>  	dev->mode_config.cursor_height = 512;
> -	dev->mode_config.preferred_depth = 24;
> +	dev->mode_config.preferred_depth = 32;
>  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
>  
>  	return vkms_output_init(vkmsdev, 0);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 2/3] drm/vkms: Switch to shmem helpers
  2020-10-12 10:59   ` Chris Wilson
@ 2020-10-12 11:20     ` Thomas Zimmermann
  0 siblings, 0 replies; 29+ messages in thread
From: Thomas Zimmermann @ 2020-10-12 11:20 UTC (permalink / raw)
  To: Chris Wilson
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, Daniel Vetter,
	Melissa Wen, DRI Development, Daniel Vetter

On Mon, 12 Oct 2020 11:59:03 +0100 Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> Quoting Daniel Vetter (2020-10-10 00:21:55)
> > Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
> > at the gem bo level, which we need for generic fbdev emulation.
> > 
> > Luckily shmem also tracks ->vaddr, so we just need to adjust the code
> > all over a bit to make this fit.
> > 
> > Also wire up handle_to_fd, dunno why that was missing.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > ---
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 33c031f27c2c..66c6842d70db 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -5,6 +5,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_gem_shmem_helper.h>
> >  #include <drm/drm_vblank.h>
> 
> >  static void vkms_release(struct drm_device *dev)
> >  {
> > @@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
> >         .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> >         .release                = vkms_release,
> >         .fops                   = &vkms_driver_fops,
> > -       .dumb_create            = vkms_dumb_create,
> > +       .dumb_create            = drm_gem_shmem_dumb_create,
> 
> Something worth pointing out is that will create an uncached (well WC)
> buffer, but since it is being exported with prime, that is probably for

I'd rather set .gem_create_object to drm_gem_shmem_create_object_cached() to
get cached memory. We already have other drivers that do this.

Best regards
Thomas


> the better. It might be worth using a memcpy_from_wc() for writeback/CRC
> calculations. E.g.
> 
> > @@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
> >                            void *vaddr_out)
> >  {
> > +       blend(vaddr_out, cursor_shmem_obj->vaddr,
> >               primary_composer, cursor_composer);
> >  }
> >  
> > @@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
> >  {
> > +       memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
> 
> would benefit from WC accessors.
> 
> On the other hand, if the load is so small no one notices, it can wait.
> 
> Do we have anything that uses vkms in CI?
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 2/3] drm/vkms: Switch to shmem helpers
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
  2020-10-12 10:59   ` Chris Wilson
@ 2020-10-12 11:22   ` Thomas Zimmermann
  2020-10-12 12:41   ` Melissa Wen
  2020-10-13 11:10   ` [PATCH] " Daniel Vetter
  3 siblings, 0 replies; 29+ messages in thread
From: Thomas Zimmermann @ 2020-10-12 11:22 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, DRI Development,
	Chris Wilson, Melissa Wen, Daniel Vetter

On Sat, 10 Oct 2020 01:21:55 +0200 Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
> at the gem bo level, which we need for generic fbdev emulation.
> 
> Luckily shmem also tracks ->vaddr, so we just need to adjust the code
> all over a bit to make this fit.
> 
> Also wire up handle_to_fd, dunno why that was missing.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
>  drivers/gpu/drm/Kconfig               |   1 +
>  drivers/gpu/drm/vkms/Makefile         |   1 -
>  drivers/gpu/drm/vkms/vkms_composer.c  |  17 +-
>  drivers/gpu/drm/vkms/vkms_drv.c       |  19 +-
>  drivers/gpu/drm/vkms/vkms_drv.h       |  26 ---
>  drivers/gpu/drm/vkms/vkms_gem.c       | 261 --------------------------
>  drivers/gpu/drm/vkms/vkms_plane.c     |  13 +-
>  drivers/gpu/drm/vkms/vkms_writeback.c |  17 +-
>  8 files changed, 32 insertions(+), 323 deletions(-)

Nice :)

>  delete mode 100644 drivers/gpu/drm/vkms/vkms_gem.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9efb82caaa87..b796c118fc3b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -287,6 +287,7 @@ config DRM_VKMS
>  	tristate "Virtual KMS (EXPERIMENTAL)"
>  	depends on DRM
>  	select DRM_KMS_HELPER
> +	select DRM_GEM_SHMEM_HELPER
>  	select CRC32
>  	default n
>  	help
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 333d3cead0e3..72f779cbfedd 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -4,7 +4,6 @@ vkms-y := \
>  	vkms_plane.o \
>  	vkms_output.o \
>  	vkms_crtc.o \
> -	vkms_gem.o \
>  	vkms_composer.o \
>  	vkms_writeback.o
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 33c031f27c2c..66c6842d70db 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "vkms_drv.h"
> @@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
>  			   void *vaddr_out)
>  {
>  	struct drm_gem_object *cursor_obj;
> -	struct vkms_gem_object *cursor_vkms_obj;
> +	struct drm_gem_shmem_object *cursor_shmem_obj;
>  
>  	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
> -	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> +	cursor_shmem_obj = to_drm_gem_shmem_obj(cursor_obj);
>  
> -	if (WARN_ON(!cursor_vkms_obj->vaddr))
> +	if (WARN_ON(!cursor_shmem_obj->vaddr))
>  		return;
>  
> -	blend(vaddr_out, cursor_vkms_obj->vaddr,
> +	blend(vaddr_out, cursor_shmem_obj->vaddr,
>  	      primary_composer, cursor_composer);
>  }
>  
> @@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
>  {
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
>  
>  	if (!*vaddr_out) {
> -		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> +		*vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
>  		if (!*vaddr_out) {
>  			DRM_ERROR("Cannot allocate memory for output frame.");
>  			return -ENOMEM;
>  		}
>  	}
>  
> -	if (WARN_ON(!vkms_obj->vaddr))
> +	if (WARN_ON(!shmem_obj->vaddr))
>  		return -EINVAL;
>  
> -	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> +	memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
>  
>  	if (cursor_composer)
>  		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index eb4007443706..6221e5040264 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_ioctl.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "vkms_drv.h"
> @@ -39,17 +40,7 @@ bool enable_cursor = true;
>  module_param_named(enable_cursor, enable_cursor, bool, 0444);
>  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>  
> -static const struct file_operations vkms_driver_fops = {
> -	.owner		= THIS_MODULE,
> -	.open		= drm_open,
> -	.mmap		= drm_gem_mmap,
> -	.unlocked_ioctl	= drm_ioctl,
> -	.compat_ioctl	= drm_compat_ioctl,
> -	.poll		= drm_poll,
> -	.read		= drm_read,
> -	.llseek		= no_llseek,
> -	.release	= drm_release,
> -};
> +DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  
>  static void vkms_release(struct drm_device *dev)
>  {
> @@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
>  	.release		= vkms_release,
>  	.fops			= &vkms_driver_fops,
> -	.dumb_create		= vkms_dumb_create,
> +	.dumb_create		= drm_gem_shmem_dumb_create,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> -	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
> +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> +	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> +	.gem_prime_mmap		= drm_gem_prime_mmap,

Please see my comment on using drm_gem_shmem_create_object_cached() In any
case.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

>   
>  	.name			= DRIVER_NAME,
>  	.desc			= DRIVER_DESC,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 380a8f27e156..ef6482e3adea 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -88,14 +88,6 @@ struct vkms_device {
>  	struct vkms_output output;
>  };
>  
> -struct vkms_gem_object {
> -	struct drm_gem_object gem;
> -	struct mutex pages_lock; /* Page lock used in page fault handler */
> -	struct page **pages;
> -	unsigned int vmap_count;
> -	void *vaddr;
> -};
> -
>  #define drm_crtc_to_vkms_output(target) \
>  	container_of(target, struct vkms_output, crtc)
>  
> @@ -120,24 +112,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index);
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  				  enum drm_plane_type type, int index);
>  
> -/* Gem stuff */
> -vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
> -
> -int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> -		     struct drm_mode_create_dumb *args);
> -
> -void vkms_gem_free_object(struct drm_gem_object *obj);
> -
> -int vkms_gem_vmap(struct drm_gem_object *obj);
> -
> -void vkms_gem_vunmap(struct drm_gem_object *obj);
> -
> -/* Prime */
> -struct drm_gem_object *
> -vkms_prime_import_sg_table(struct drm_device *dev,
> -			   struct dma_buf_attachment *attach,
> -			   struct sg_table *sg);
> -
>  /* CRC Support */
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
>  					size_t *count);
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> deleted file mode 100644
> index 19a0e260a4df..000000000000
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ /dev/null
> @@ -1,261 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -
> -#include <linux/dma-buf.h>
> -#include <linux/shmem_fs.h>
> -#include <linux/vmalloc.h>
> -#include <drm/drm_prime.h>
> -
> -#include "vkms_drv.h"
> -
> -static const struct vm_operations_struct vkms_gem_vm_ops = {
> -	.fault = vkms_gem_fault,
> -	.open = drm_gem_vm_open,
> -	.close = drm_gem_vm_close,
> -};
> -
> -static const struct drm_gem_object_funcs vkms_gem_object_funcs = {
> -	.free = vkms_gem_free_object,
> -	.vm_ops = &vkms_gem_vm_ops,
> -};
> -
> -static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
> -						 u64 size)
> -{
> -	struct vkms_gem_object *obj;
> -	int ret;
> -
> -	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> -	if (!obj)
> -		return ERR_PTR(-ENOMEM);
> -
> -	obj->gem.funcs = &vkms_gem_object_funcs;
> -
> -	size = roundup(size, PAGE_SIZE);
> -	ret = drm_gem_object_init(dev, &obj->gem, size);
> -	if (ret) {
> -		kfree(obj);
> -		return ERR_PTR(ret);
> -	}
> -
> -	mutex_init(&obj->pages_lock);
> -
> -	return obj;
> -}
> -
> -void vkms_gem_free_object(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
> -						   gem);
> -
> -	WARN_ON(gem->pages);
> -	WARN_ON(gem->vaddr);
> -
> -	mutex_destroy(&gem->pages_lock);
> -	drm_gem_object_release(obj);
> -	kfree(gem);
> -}
> -
> -vm_fault_t vkms_gem_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct vkms_gem_object *obj = vma->vm_private_data;
> -	unsigned long vaddr = vmf->address;
> -	pgoff_t page_offset;
> -	loff_t num_pages;
> -	vm_fault_t ret = VM_FAULT_SIGBUS;
> -
> -	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
> -	num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);
> -
> -	if (page_offset > num_pages)
> -		return VM_FAULT_SIGBUS;
> -
> -	mutex_lock(&obj->pages_lock);
> -	if (obj->pages) {
> -		get_page(obj->pages[page_offset]);
> -		vmf->page = obj->pages[page_offset];
> -		ret = 0;
> -	}
> -	mutex_unlock(&obj->pages_lock);
> -	if (ret) {
> -		struct page *page;
> -		struct address_space *mapping;
> -
> -		mapping = file_inode(obj->gem.filp)->i_mapping;
> -		page = shmem_read_mapping_page(mapping, page_offset);
> -
> -		if (!IS_ERR(page)) {
> -			vmf->page = page;
> -			ret = 0;
> -		} else {
> -			switch (PTR_ERR(page)) {
> -			case -ENOSPC:
> -			case -ENOMEM:
> -				ret = VM_FAULT_OOM;
> -				break;
> -			case -EBUSY:
> -				ret = VM_FAULT_RETRY;
> -				break;
> -			case -EFAULT:
> -			case -EINVAL:
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> -			default:
> -				WARN_ON(PTR_ERR(page));
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> -			}
> -		}
> -	}
> -	return ret;
> -}
> -
> -static struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> -					      struct drm_file *file,
> -					      u32 *handle,
> -					      u64 size)
> -{
> -	struct vkms_gem_object *obj;
> -	int ret;
> -
> -	if (!file || !dev || !handle)
> -		return ERR_PTR(-EINVAL);
> -
> -	obj = __vkms_gem_create(dev, size);
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	ret = drm_gem_handle_create(file, &obj->gem, handle);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
> -	return &obj->gem;
> -}
> -
> -int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> -		     struct drm_mode_create_dumb *args)
> -{
> -	struct drm_gem_object *gem_obj;
> -	u64 pitch, size;
> -
> -	if (!args || !dev || !file)
> -		return -EINVAL;
> -
> -	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> -	size = pitch * args->height;
> -
> -	if (!size)
> -		return -EINVAL;
> -
> -	gem_obj = vkms_gem_create(dev, file, &args->handle, size);
> -	if (IS_ERR(gem_obj))
> -		return PTR_ERR(gem_obj);
> -
> -	args->size = gem_obj->size;
> -	args->pitch = pitch;
> -
> -	drm_gem_object_put(gem_obj);
> -
> -	DRM_DEBUG_DRIVER("Created object of size %lld\n", size);
> -
> -	return 0;
> -}
> -
> -static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
> -{
> -	struct drm_gem_object *gem_obj = &vkms_obj->gem;
> -
> -	if (!vkms_obj->pages) {
> -		struct page **pages = drm_gem_get_pages(gem_obj);
> -
> -		if (IS_ERR(pages))
> -			return pages;
> -
> -		if (cmpxchg(&vkms_obj->pages, NULL, pages))
> -			drm_gem_put_pages(gem_obj, pages, false, true);
> -	}
> -
> -	return vkms_obj->pages;
> -}
> -
> -void vkms_gem_vunmap(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> -
> -	mutex_lock(&vkms_obj->pages_lock);
> -	if (vkms_obj->vmap_count < 1) {
> -		WARN_ON(vkms_obj->vaddr);
> -		WARN_ON(vkms_obj->pages);
> -		mutex_unlock(&vkms_obj->pages_lock);
> -		return;
> -	}
> -
> -	vkms_obj->vmap_count--;
> -
> -	if (vkms_obj->vmap_count == 0) {
> -		vunmap(vkms_obj->vaddr);
> -		vkms_obj->vaddr = NULL;
> -		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> -		vkms_obj->pages = NULL;
> -	}
> -
> -	mutex_unlock(&vkms_obj->pages_lock);
> -}
> -
> -int vkms_gem_vmap(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> -	int ret = 0;
> -
> -	mutex_lock(&vkms_obj->pages_lock);
> -
> -	if (!vkms_obj->vaddr) {
> -		unsigned int n_pages = obj->size >> PAGE_SHIFT;
> -		struct page **pages = _get_pages(vkms_obj);
> -
> -		if (IS_ERR(pages)) {
> -			ret = PTR_ERR(pages);
> -			goto out;
> -		}
> -
> -		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
> -		if (!vkms_obj->vaddr)
> -			goto err_vmap;
> -	}
> -
> -	vkms_obj->vmap_count++;
> -	goto out;
> -
> -err_vmap:
> -	ret = -ENOMEM;
> -	drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> -	vkms_obj->pages = NULL;
> -out:
> -	mutex_unlock(&vkms_obj->pages_lock);
> -	return ret;
> -}
> -
> -struct drm_gem_object *
> -vkms_prime_import_sg_table(struct drm_device *dev,
> -			   struct dma_buf_attachment *attach,
> -			   struct sg_table *sg)
> -{
> -	struct vkms_gem_object *obj;
> -	int npages;
> -
> -	obj = __vkms_gem_create(dev, attach->dmabuf->size);
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> -	DRM_DEBUG_PRIME("Importing %d pages\n", npages);
> -
> -	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> -	if (!obj->pages) {
> -		vkms_gem_free_object(&obj->gem);
> -		return ERR_PTR(-ENOMEM);
> -	}
> -
> -	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL, npages);
> -	return &obj->gem;
> -}
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 6d31265a2ab7..9890137bcb8d 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  
>  #include "vkms_drv.h"
>  
> @@ -145,15 +146,15 @@ static int vkms_prepare_fb(struct drm_plane *plane,
>  			   struct drm_plane_state *state)
>  {
>  	struct drm_gem_object *gem_obj;
> -	int ret;
> +	void *vaddr;
>  
>  	if (!state->fb)
>  		return 0;
>  
>  	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
> -	ret = vkms_gem_vmap(gem_obj);
> -	if (ret)
> -		DRM_ERROR("vmap failed: %d\n", ret);
> +	vaddr = drm_gem_shmem_vmap(gem_obj);
> +	if (IS_ERR(vaddr))
> +		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
>  
>  	return drm_gem_fb_prepare_fb(plane, state);
>  }
> @@ -162,12 +163,14 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
>  			    struct drm_plane_state *old_state)
>  {
>  	struct drm_gem_object *gem_obj;
> +	struct drm_gem_shmem_object *shmem_obj;
>  
>  	if (!old_state->fb)
>  		return;
>  
>  	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
> -	vkms_gem_vunmap(gem_obj);
> +	shmem_obj = to_drm_gem_shmem_obj(drm_gem_fb_get_obj(old_state->fb, 0));
> +	drm_gem_shmem_vunmap(gem_obj, shmem_obj->vaddr);
>  }
>  
>  static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 094fa4aa061d..26b903926872 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -6,6 +6,7 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  
>  static const u32 vkms_wb_formats[] = {
>  	DRM_FORMAT_XRGB8888,
> @@ -63,22 +64,20 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
>  static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  			       struct drm_writeback_job *job)
>  {
> -	struct vkms_gem_object *vkms_obj;
>  	struct drm_gem_object *gem_obj;
> -	int ret;
> +	void *vaddr;
>  
>  	if (!job->fb)
>  		return 0;
>  
>  	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	ret = vkms_gem_vmap(gem_obj);
> -	if (ret) {
> -		DRM_ERROR("vmap failed: %d\n", ret);
> -		return ret;
> +	vaddr = drm_gem_shmem_vmap(gem_obj);
> +	if (IS_ERR(vaddr)) {
> +		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
> +		return PTR_ERR(vaddr);
>  	}
>  
> -	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> -	job->priv = vkms_obj->vaddr;
> +	job->priv = vaddr;
>  
>  	return 0;
>  }
> @@ -93,7 +92,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  		return;
>  
>  	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	vkms_gem_vunmap(gem_obj);
> +	drm_gem_shmem_vunmap(gem_obj, job->priv);
>  
>  	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
>  	vkms_set_composer(&vkmsdev->output, false);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-09 23:21 ` [PATCH 3/3] drm/vkms: fbdev emulation support Daniel Vetter
@ 2020-10-12 11:24   ` Thomas Zimmermann
  2020-10-12 12:40     ` Neil Armstrong
  2020-10-16 10:10   ` Melissa Wen
  1 sibling, 1 reply; 29+ messages in thread
From: Thomas Zimmermann @ 2020-10-12 11:24 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Melissa Wen, Haneen Mohammed, Rodrigo Siqueira, DRI Development,
	Daniel Vetter

Hi

On Sat, 10 Oct 2020 01:21:56 +0200 Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Hooray for generic fbdev support, making this a oneliner. We just
> needed to fix preferred_depth fixed and the vmap support added first.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 6221e5040264..cc09e2df5cb1 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -169,6 +169,8 @@ static int __init vkms_init(void)
>  	if (ret)
>  		goto out_devres;
>  
> +	drm_fbdev_generic_setup(&vkms_device->drm, 0);
> +

It feels strange to have console support in a driver for non-interactive
systems. But OK, why not. I guess it helps with testing?

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

>  	return 0;
>  
>  out_devres:

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-12 11:24   ` Thomas Zimmermann
@ 2020-10-12 12:40     ` Neil Armstrong
  2020-10-12 14:23       ` Daniel Vetter
  0 siblings, 1 reply; 29+ messages in thread
From: Neil Armstrong @ 2020-10-12 12:40 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Melissa Wen, Haneen Mohammed, DRI Development, Rodrigo Siqueira,
	Daniel Vetter

Hi,

On 12/10/2020 13:24, Thomas Zimmermann wrote:
> Hi
> 
> On Sat, 10 Oct 2020 01:21:56 +0200 Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
>> Hooray for generic fbdev support, making this a oneliner. We just
>> needed to fix preferred_depth fixed and the vmap support added first.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>> Cc: Melissa Wen <melissa.srw@gmail.com>
>> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> ---
>>  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>> index 6221e5040264..cc09e2df5cb1 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>> @@ -169,6 +169,8 @@ static int __init vkms_init(void)
>>  	if (ret)
>>  		goto out_devres;
>>  
>> +	drm_fbdev_generic_setup(&vkms_device->drm, 0);
>> +
> 
> It feels strange to have console support in a driver for non-interactive
> systems. But OK, why not. I guess it helps with testing?

It's weird because it the kernel is misconfigured and no console is specified on the cmdline
this console could become the main console...

It's a great feature, but couldn't this be a module parameter ?

Neil

> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Best regards
> Thomas
> 
>>  	return 0;
>>  
>>  out_devres:
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 2/3] drm/vkms: Switch to shmem helpers
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
  2020-10-12 10:59   ` Chris Wilson
  2020-10-12 11:22   ` Thomas Zimmermann
@ 2020-10-12 12:41   ` Melissa Wen
  2020-10-13 11:10   ` [PATCH] " Daniel Vetter
  3 siblings, 0 replies; 29+ messages in thread
From: Melissa Wen @ 2020-10-12 12:41 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, Chris Wilson,
	DRI Development, Thomas Zimmermann, Daniel Vetter

Hi Daniel,

Thanks for this patch.

It took me a while to understand the update.
I run some tests and it looks good to me.

On 10/10, Daniel Vetter wrote:
> Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
> at the gem bo level, which we need for generic fbdev emulation.
> 
> Luckily shmem also tracks ->vaddr, so we just need to adjust the code
> all over a bit to make this fit.
> 
> Also wire up handle_to_fd, dunno why that was missing.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
>  drivers/gpu/drm/Kconfig               |   1 +
>  drivers/gpu/drm/vkms/Makefile         |   1 -
>  drivers/gpu/drm/vkms/vkms_composer.c  |  17 +-
>  drivers/gpu/drm/vkms/vkms_drv.c       |  19 +-
>  drivers/gpu/drm/vkms/vkms_drv.h       |  26 ---
>  drivers/gpu/drm/vkms/vkms_gem.c       | 261 --------------------------
>  drivers/gpu/drm/vkms/vkms_plane.c     |  13 +-
>  drivers/gpu/drm/vkms/vkms_writeback.c |  17 +-
>  8 files changed, 32 insertions(+), 323 deletions(-)
>  delete mode 100644 drivers/gpu/drm/vkms/vkms_gem.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9efb82caaa87..b796c118fc3b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -287,6 +287,7 @@ config DRM_VKMS
>  	tristate "Virtual KMS (EXPERIMENTAL)"
>  	depends on DRM
>  	select DRM_KMS_HELPER
> +	select DRM_GEM_SHMEM_HELPER
>  	select CRC32
>  	default n
>  	help
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 333d3cead0e3..72f779cbfedd 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -4,7 +4,6 @@ vkms-y := \
>  	vkms_plane.o \
>  	vkms_output.o \
>  	vkms_crtc.o \
> -	vkms_gem.o \
>  	vkms_composer.o \
>  	vkms_writeback.o
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 33c031f27c2c..66c6842d70db 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "vkms_drv.h"
> @@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
>  			   void *vaddr_out)
>  {
>  	struct drm_gem_object *cursor_obj;
> -	struct vkms_gem_object *cursor_vkms_obj;
> +	struct drm_gem_shmem_object *cursor_shmem_obj;
>  
>  	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
> -	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> +	cursor_shmem_obj = to_drm_gem_shmem_obj(cursor_obj);
>  
> -	if (WARN_ON(!cursor_vkms_obj->vaddr))
> +	if (WARN_ON(!cursor_shmem_obj->vaddr))
>  		return;
>  
> -	blend(vaddr_out, cursor_vkms_obj->vaddr,
> +	blend(vaddr_out, cursor_shmem_obj->vaddr,
>  	      primary_composer, cursor_composer);
>  }
>  
> @@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
>  {
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
>  
>  	if (!*vaddr_out) {
> -		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> +		*vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
>  		if (!*vaddr_out) {
>  			DRM_ERROR("Cannot allocate memory for output frame.");
>  			return -ENOMEM;
>  		}
>  	}
>  
> -	if (WARN_ON(!vkms_obj->vaddr))
> +	if (WARN_ON(!shmem_obj->vaddr))
>  		return -EINVAL;
>  
> -	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> +	memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
>  
>  	if (cursor_composer)
>  		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index eb4007443706..6221e5040264 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_ioctl.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "vkms_drv.h"
> @@ -39,17 +40,7 @@ bool enable_cursor = true;
>  module_param_named(enable_cursor, enable_cursor, bool, 0444);
>  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>  
> -static const struct file_operations vkms_driver_fops = {
> -	.owner		= THIS_MODULE,
> -	.open		= drm_open,
> -	.mmap		= drm_gem_mmap,
> -	.unlocked_ioctl	= drm_ioctl,
> -	.compat_ioctl	= drm_compat_ioctl,
> -	.poll		= drm_poll,
> -	.read		= drm_read,
> -	.llseek		= no_llseek,
> -	.release	= drm_release,
> -};
> +DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  
>  static void vkms_release(struct drm_device *dev)
>  {
> @@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
>  	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
>  	.release		= vkms_release,
>  	.fops			= &vkms_driver_fops,
> -	.dumb_create		= vkms_dumb_create,
> +	.dumb_create		= drm_gem_shmem_dumb_create,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> -	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
> +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> +	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> +	.gem_prime_mmap		= drm_gem_prime_mmap,
>  
>  	.name			= DRIVER_NAME,
>  	.desc			= DRIVER_DESC,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 380a8f27e156..ef6482e3adea 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -88,14 +88,6 @@ struct vkms_device {
>  	struct vkms_output output;
>  };
>  
> -struct vkms_gem_object {
> -	struct drm_gem_object gem;
> -	struct mutex pages_lock; /* Page lock used in page fault handler */
> -	struct page **pages;
> -	unsigned int vmap_count;
> -	void *vaddr;
> -};
> -

Around here, there is a "#define drm_gem_to_vkms_gem" that I think we
can also get rid of it.

>  #define drm_crtc_to_vkms_output(target) \
>  	container_of(target, struct vkms_output, crtc)
>  
> @@ -120,24 +112,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index);
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  				  enum drm_plane_type type, int index);
>  
> -/* Gem stuff */
> -vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
> -
> -int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> -		     struct drm_mode_create_dumb *args);
> -
> -void vkms_gem_free_object(struct drm_gem_object *obj);
> -
> -int vkms_gem_vmap(struct drm_gem_object *obj);
> -
> -void vkms_gem_vunmap(struct drm_gem_object *obj);
> -
> -/* Prime */
> -struct drm_gem_object *
> -vkms_prime_import_sg_table(struct drm_device *dev,
> -			   struct dma_buf_attachment *attach,
> -			   struct sg_table *sg);
> -
>  /* CRC Support */
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
>  					size_t *count);
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> deleted file mode 100644
> index 19a0e260a4df..000000000000
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ /dev/null
> @@ -1,261 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -
> -#include <linux/dma-buf.h>
> -#include <linux/shmem_fs.h>
> -#include <linux/vmalloc.h>
> -#include <drm/drm_prime.h>
> -
> -#include "vkms_drv.h"
> -
> -static const struct vm_operations_struct vkms_gem_vm_ops = {
> -	.fault = vkms_gem_fault,
> -	.open = drm_gem_vm_open,
> -	.close = drm_gem_vm_close,
> -};
> -
> -static const struct drm_gem_object_funcs vkms_gem_object_funcs = {
> -	.free = vkms_gem_free_object,
> -	.vm_ops = &vkms_gem_vm_ops,
> -};
> -
> -static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
> -						 u64 size)
> -{
> -	struct vkms_gem_object *obj;
> -	int ret;
> -
> -	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> -	if (!obj)
> -		return ERR_PTR(-ENOMEM);
> -
> -	obj->gem.funcs = &vkms_gem_object_funcs;
> -
> -	size = roundup(size, PAGE_SIZE);
> -	ret = drm_gem_object_init(dev, &obj->gem, size);
> -	if (ret) {
> -		kfree(obj);
> -		return ERR_PTR(ret);
> -	}
> -
> -	mutex_init(&obj->pages_lock);
> -
> -	return obj;
> -}
> -
> -void vkms_gem_free_object(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
> -						   gem);
> -
> -	WARN_ON(gem->pages);
> -	WARN_ON(gem->vaddr);
> -
> -	mutex_destroy(&gem->pages_lock);
> -	drm_gem_object_release(obj);
> -	kfree(gem);
> -}
> -
> -vm_fault_t vkms_gem_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct vkms_gem_object *obj = vma->vm_private_data;
> -	unsigned long vaddr = vmf->address;
> -	pgoff_t page_offset;
> -	loff_t num_pages;
> -	vm_fault_t ret = VM_FAULT_SIGBUS;
> -
> -	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
> -	num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);
> -
> -	if (page_offset > num_pages)
> -		return VM_FAULT_SIGBUS;
> -
> -	mutex_lock(&obj->pages_lock);
> -	if (obj->pages) {
> -		get_page(obj->pages[page_offset]);
> -		vmf->page = obj->pages[page_offset];
> -		ret = 0;
> -	}
> -	mutex_unlock(&obj->pages_lock);
> -	if (ret) {
> -		struct page *page;
> -		struct address_space *mapping;
> -
> -		mapping = file_inode(obj->gem.filp)->i_mapping;
> -		page = shmem_read_mapping_page(mapping, page_offset);
> -
> -		if (!IS_ERR(page)) {
> -			vmf->page = page;
> -			ret = 0;
> -		} else {
> -			switch (PTR_ERR(page)) {
> -			case -ENOSPC:
> -			case -ENOMEM:
> -				ret = VM_FAULT_OOM;
> -				break;
> -			case -EBUSY:
> -				ret = VM_FAULT_RETRY;
> -				break;
> -			case -EFAULT:
> -			case -EINVAL:
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> -			default:
> -				WARN_ON(PTR_ERR(page));
> -				ret = VM_FAULT_SIGBUS;
> -				break;
> -			}
> -		}
> -	}
> -	return ret;
> -}
> -
> -static struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> -					      struct drm_file *file,
> -					      u32 *handle,
> -					      u64 size)
> -{
> -	struct vkms_gem_object *obj;
> -	int ret;
> -
> -	if (!file || !dev || !handle)
> -		return ERR_PTR(-EINVAL);
> -
> -	obj = __vkms_gem_create(dev, size);
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	ret = drm_gem_handle_create(file, &obj->gem, handle);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
> -	return &obj->gem;
> -}
> -
> -int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> -		     struct drm_mode_create_dumb *args)
> -{
> -	struct drm_gem_object *gem_obj;
> -	u64 pitch, size;
> -
> -	if (!args || !dev || !file)
> -		return -EINVAL;
> -
> -	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> -	size = pitch * args->height;
> -
> -	if (!size)
> -		return -EINVAL;
> -
> -	gem_obj = vkms_gem_create(dev, file, &args->handle, size);
> -	if (IS_ERR(gem_obj))
> -		return PTR_ERR(gem_obj);
> -
> -	args->size = gem_obj->size;
> -	args->pitch = pitch;
> -
> -	drm_gem_object_put(gem_obj);
> -
> -	DRM_DEBUG_DRIVER("Created object of size %lld\n", size);
> -
> -	return 0;
> -}
> -
> -static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
> -{
> -	struct drm_gem_object *gem_obj = &vkms_obj->gem;
> -
> -	if (!vkms_obj->pages) {
> -		struct page **pages = drm_gem_get_pages(gem_obj);
> -
> -		if (IS_ERR(pages))
> -			return pages;
> -
> -		if (cmpxchg(&vkms_obj->pages, NULL, pages))
> -			drm_gem_put_pages(gem_obj, pages, false, true);
> -	}
> -
> -	return vkms_obj->pages;
> -}
> -
> -void vkms_gem_vunmap(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> -
> -	mutex_lock(&vkms_obj->pages_lock);
> -	if (vkms_obj->vmap_count < 1) {
> -		WARN_ON(vkms_obj->vaddr);
> -		WARN_ON(vkms_obj->pages);
> -		mutex_unlock(&vkms_obj->pages_lock);
> -		return;
> -	}
> -
> -	vkms_obj->vmap_count--;
> -
> -	if (vkms_obj->vmap_count == 0) {
> -		vunmap(vkms_obj->vaddr);
> -		vkms_obj->vaddr = NULL;
> -		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> -		vkms_obj->pages = NULL;
> -	}
> -
> -	mutex_unlock(&vkms_obj->pages_lock);
> -}
> -
> -int vkms_gem_vmap(struct drm_gem_object *obj)
> -{
> -	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> -	int ret = 0;
> -
> -	mutex_lock(&vkms_obj->pages_lock);
> -
> -	if (!vkms_obj->vaddr) {
> -		unsigned int n_pages = obj->size >> PAGE_SHIFT;
> -		struct page **pages = _get_pages(vkms_obj);
> -
> -		if (IS_ERR(pages)) {
> -			ret = PTR_ERR(pages);
> -			goto out;
> -		}
> -
> -		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
> -		if (!vkms_obj->vaddr)
> -			goto err_vmap;
> -	}
> -
> -	vkms_obj->vmap_count++;
> -	goto out;
> -
> -err_vmap:
> -	ret = -ENOMEM;
> -	drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> -	vkms_obj->pages = NULL;
> -out:
> -	mutex_unlock(&vkms_obj->pages_lock);
> -	return ret;
> -}
> -
> -struct drm_gem_object *
> -vkms_prime_import_sg_table(struct drm_device *dev,
> -			   struct dma_buf_attachment *attach,
> -			   struct sg_table *sg)
> -{
> -	struct vkms_gem_object *obj;
> -	int npages;
> -
> -	obj = __vkms_gem_create(dev, attach->dmabuf->size);
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> -	DRM_DEBUG_PRIME("Importing %d pages\n", npages);
> -
> -	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> -	if (!obj->pages) {
> -		vkms_gem_free_object(&obj->gem);
> -		return ERR_PTR(-ENOMEM);
> -	}
> -
> -	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL, npages);
> -	return &obj->gem;
> -}
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 6d31265a2ab7..9890137bcb8d 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  
>  #include "vkms_drv.h"
>  
> @@ -145,15 +146,15 @@ static int vkms_prepare_fb(struct drm_plane *plane,
>  			   struct drm_plane_state *state)
>  {
>  	struct drm_gem_object *gem_obj;
> -	int ret;
> +	void *vaddr;
>  
>  	if (!state->fb)
>  		return 0;
>  
>  	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
> -	ret = vkms_gem_vmap(gem_obj);
> -	if (ret)
> -		DRM_ERROR("vmap failed: %d\n", ret);
> +	vaddr = drm_gem_shmem_vmap(gem_obj);
> +	if (IS_ERR(vaddr))
> +		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
>  
>  	return drm_gem_fb_prepare_fb(plane, state);
>  }
> @@ -162,12 +163,14 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
>  			    struct drm_plane_state *old_state)
>  {
>  	struct drm_gem_object *gem_obj;
> +	struct drm_gem_shmem_object *shmem_obj;
>  
>  	if (!old_state->fb)
>  		return;
>  
>  	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
> -	vkms_gem_vunmap(gem_obj);
> +	shmem_obj = to_drm_gem_shmem_obj(drm_gem_fb_get_obj(old_state->fb, 0));
> +	drm_gem_shmem_vunmap(gem_obj, shmem_obj->vaddr);
>  }
>  
>  static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 094fa4aa061d..26b903926872 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -6,6 +6,7 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
>  
>  static const u32 vkms_wb_formats[] = {
>  	DRM_FORMAT_XRGB8888,
> @@ -63,22 +64,20 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
>  static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  			       struct drm_writeback_job *job)
>  {
> -	struct vkms_gem_object *vkms_obj;
>  	struct drm_gem_object *gem_obj;
> -	int ret;
> +	void *vaddr;
>  
>  	if (!job->fb)
>  		return 0;
>  
>  	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	ret = vkms_gem_vmap(gem_obj);
> -	if (ret) {
> -		DRM_ERROR("vmap failed: %d\n", ret);
> -		return ret;
> +	vaddr = drm_gem_shmem_vmap(gem_obj);
> +	if (IS_ERR(vaddr)) {
> +		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
> +		return PTR_ERR(vaddr);
>  	}
>  
> -	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> -	job->priv = vkms_obj->vaddr;
> +	job->priv = vaddr;
>  
>  	return 0;
>  }
> @@ -93,7 +92,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  		return;
>  
>  	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	vkms_gem_vunmap(gem_obj);
> +	drm_gem_shmem_vunmap(gem_obj, job->priv);
>  
>  	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
>  	vkms_set_composer(&vkmsdev->output, false);
> -- 
> 2.28.0
>
Reviewed-by: Melissa Wen <melissa.srw@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
                   ` (2 preceding siblings ...)
  2020-10-12 11:08 ` [PATCH 1/3] drm/vkms: Set preferred depth correctly Thomas Zimmermann
@ 2020-10-12 12:59 ` Melissa Wen
  2020-10-15 21:39   ` Daniel Vetter
  2020-10-16 10:38 ` Simon Ser
  4 siblings, 1 reply; 29+ messages in thread
From: Melissa Wen @ 2020-10-12 12:59 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

On 10/10, Daniel Vetter wrote:
> The only thing we support is xrgb8888.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 726801ab44d4..eb4007443706 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.max_height = YRES_MAX;
>  	dev->mode_config.cursor_width = 512;
>  	dev->mode_config.cursor_height = 512;
> -	dev->mode_config.preferred_depth = 24;
> +	dev->mode_config.preferred_depth = 32;
nice catch!

>  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
>  
>  	return vkms_output_init(vkmsdev, 0);
> -- 
> 2.28.0
>
Thanks, 

Reviewed-by: Melissa Wen <melissa.srw@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-12 12:40     ` Neil Armstrong
@ 2020-10-12 14:23       ` Daniel Vetter
  2020-10-13  6:14         ` Pekka Paalanen
  0 siblings, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2020-10-12 14:23 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Haneen Mohammed, Rodrigo Siqueira, Daniel Vetter,
	DRI Development, Melissa Wen, Thomas Zimmermann, Daniel Vetter

On Mon, Oct 12, 2020 at 02:40:58PM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 12/10/2020 13:24, Thomas Zimmermann wrote:
> > Hi
> > 
> > On Sat, 10 Oct 2020 01:21:56 +0200 Daniel Vetter <daniel.vetter@ffwll.ch>
> > wrote:
> > 
> >> Hooray for generic fbdev support, making this a oneliner. We just
> >> needed to fix preferred_depth fixed and the vmap support added first.
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >> Cc: Melissa Wen <melissa.srw@gmail.com>
> >> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> ---
> >>  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> >> index 6221e5040264..cc09e2df5cb1 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> >> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> >> @@ -169,6 +169,8 @@ static int __init vkms_init(void)
> >>  	if (ret)
> >>  		goto out_devres;
> >>  
> >> +	drm_fbdev_generic_setup(&vkms_device->drm, 0);
> >> +
> > 
> > It feels strange to have console support in a driver for non-interactive
> > systems. But OK, why not. I guess it helps with testing?

Yeah I want to polish the igt fbdev testcase a bit, need a victim for fb
device selction. vkms was the quickest way to get two fbdev instances into
one box :-)

Plus more code we could test using igt & vkms in gitlab CI, whenever we
get around to that ...

> It's weird because it the kernel is misconfigured and no console is specified on the cmdline
> this console could become the main console...
> 
> It's a great feature, but couldn't this be a module parameter ?

If you have vkms enabled in a distro, you're doing it wrong.

Also, if you accidentally load vkms, then Xorg will happily bind to it
(maybe in preference to the real fbdev driver that's there as a simplefb).
So imo this problem isn't really new.
-Daniel

> 
> Neil
> 
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > Best regards
> > Thomas
> > 
> >>  	return 0;
> >>  
> >>  out_devres:
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-12 14:23       ` Daniel Vetter
@ 2020-10-13  6:14         ` Pekka Paalanen
  2020-10-13  7:53           ` Daniel Vetter
  0 siblings, 1 reply; 29+ messages in thread
From: Pekka Paalanen @ 2020-10-13  6:14 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Haneen Mohammed, Rodrigo Siqueira, Neil Armstrong, Daniel Vetter,
	DRI Development, Melissa Wen, Thomas Zimmermann, Daniel Vetter


[-- Attachment #1.1: Type: text/plain, Size: 1619 bytes --]

On Mon, 12 Oct 2020 16:23:35 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Oct 12, 2020 at 02:40:58PM +0200, Neil Armstrong wrote:
> > Hi,
> > 

...

> > It's weird because it the kernel is misconfigured and no console is specified on the cmdline
> > this console could become the main console...
> > 
> > It's a great feature, but couldn't this be a module parameter ?  
> 
> If you have vkms enabled in a distro, you're doing it wrong.

That's really not a great position to take. I would prefer that
if a random contributor writes a Weston patch and runs 'meson test', it
will use VKMS to run Weston's DRM-backend tests on his machine
automatically, maybe save for some seat and device node access
permissions bits which distributions could be delivering as well.

Just put the VKMS device node into a non-default seat, and Xorg etc.
will happily ignore it.

For the fbdev device node, I don't know. Maybe a module parameter
really is a good choice there, defaulting to off. I have no interest in
testing anything against fbdev, but other people might disagree of
course.

Why? Gitlab CI is still not running tests for every commit, just per
MR, and it might even be infeasible too.

I am also hoping for a future where I don't have to build my own kernel
just to be able to run Weston DRM tests with VKMS. That means I want to
be able to run my machine with VKMS loaded and active at all times,
without affecting the normal desktop. I already have such a setup with
an extra AMD card, but you can't run most KMS tests against real
hardware drivers.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-13  6:14         ` Pekka Paalanen
@ 2020-10-13  7:53           ` Daniel Vetter
  2020-10-13 10:19             ` Pekka Paalanen
  0 siblings, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2020-10-13  7:53 UTC (permalink / raw)
  To: Pekka Paalanen
  Cc: Haneen Mohammed, Rodrigo Siqueira, Neil Armstrong,
	DRI Development, Melissa Wen, Thomas Zimmermann, Daniel Vetter

On Tue, Oct 13, 2020 at 8:14 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 12 Oct 2020 16:23:35 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Mon, Oct 12, 2020 at 02:40:58PM +0200, Neil Armstrong wrote:
> > > Hi,
> > >
>
> ...
>
> > > It's weird because it the kernel is misconfigured and no console is specified on the cmdline
> > > this console could become the main console...
> > >
> > > It's a great feature, but couldn't this be a module parameter ?
> >
> > If you have vkms enabled in a distro, you're doing it wrong.
>
> That's really not a great position to take. I would prefer that
> if a random contributor writes a Weston patch and runs 'meson test', it
> will use VKMS to run Weston's DRM-backend tests on his machine
> automatically, maybe save for some seat and device node access
> permissions bits which distributions could be delivering as well.
>
> Just put the VKMS device node into a non-default seat, and Xorg etc.
> will happily ignore it.
>
> For the fbdev device node, I don't know. Maybe a module parameter
> really is a good choice there, defaulting to off. I have no interest in
> testing anything against fbdev, but other people might disagree of
> course.
>
> Why? Gitlab CI is still not running tests for every commit, just per
> MR, and it might even be infeasible too.
>
> I am also hoping for a future where I don't have to build my own kernel
> just to be able to run Weston DRM tests with VKMS. That means I want to
> be able to run my machine with VKMS loaded and active at all times,
> without affecting the normal desktop. I already have such a setup with
> an extra AMD card, but you can't run most KMS tests against real
> hardware drivers.

I just realized that building vkms is no problem, since it doesn't
auto-load. And if our Grand Plans with configurability come true, then
your test-harness will want to do that loading and setup itself
anyway. With that there also shouldn't be any problems with fbcon,
since presumably you already have that bound to the real gpu.

So I think we're all fine here, for everyone.

Now if you built-in vkms, that's a different thing. And for that I
really think a "don't do that" is the right choice.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-13  7:53           ` Daniel Vetter
@ 2020-10-13 10:19             ` Pekka Paalanen
  2020-10-13 11:03               ` Daniel Vetter
  0 siblings, 1 reply; 29+ messages in thread
From: Pekka Paalanen @ 2020-10-13 10:19 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Haneen Mohammed, Rodrigo Siqueira, Neil Armstrong,
	DRI Development, Melissa Wen, Thomas Zimmermann, Daniel Vetter


[-- Attachment #1.1: Type: text/plain, Size: 2913 bytes --]

On Tue, 13 Oct 2020 09:53:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Oct 13, 2020 at 8:14 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Mon, 12 Oct 2020 16:23:35 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >  
> > > On Mon, Oct 12, 2020 at 02:40:58PM +0200, Neil Armstrong wrote:  
> > > > Hi,
> > > >  
> >
> > ...
> >  
> > > > It's weird because it the kernel is misconfigured and no console is specified on the cmdline
> > > > this console could become the main console...
> > > >
> > > > It's a great feature, but couldn't this be a module parameter ?  
> > >
> > > If you have vkms enabled in a distro, you're doing it wrong.  
> >
> > That's really not a great position to take. I would prefer that
> > if a random contributor writes a Weston patch and runs 'meson test', it
> > will use VKMS to run Weston's DRM-backend tests on his machine
> > automatically, maybe save for some seat and device node access
> > permissions bits which distributions could be delivering as well.
> >
> > Just put the VKMS device node into a non-default seat, and Xorg etc.
> > will happily ignore it.
> >
> > For the fbdev device node, I don't know. Maybe a module parameter
> > really is a good choice there, defaulting to off. I have no interest in
> > testing anything against fbdev, but other people might disagree of
> > course.
> >
> > Why? Gitlab CI is still not running tests for every commit, just per
> > MR, and it might even be infeasible too.
> >
> > I am also hoping for a future where I don't have to build my own kernel
> > just to be able to run Weston DRM tests with VKMS. That means I want to
> > be able to run my machine with VKMS loaded and active at all times,
> > without affecting the normal desktop. I already have such a setup with
> > an extra AMD card, but you can't run most KMS tests against real
> > hardware drivers.  
> 
> I just realized that building vkms is no problem, since it doesn't
> auto-load. And if our Grand Plans with configurability come true, then
> your test-harness will want to do that loading and setup itself
> anyway. With that there also shouldn't be any problems with fbcon,
> since presumably you already have that bound to the real gpu.
> 
> So I think we're all fine here, for everyone.
> 
> Now if you built-in vkms, that's a different thing. And for that I
> really think a "don't do that" is the right choice.

Very good.

My remaining wish is that VKMS would be fully configurable and usable
by an ordinary user, but I suppose that should be solved with a
privileged userspace daemon somewhat similar to logind that hands out
VKMS "sessions" somehow.

Not sure configfs is the best choice for VKMS configuration, unless
maybe unprivileged userspace could ask for a VKMS instance with its own
configfs tree it can access without CAP_ADMIN...


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-13 10:19             ` Pekka Paalanen
@ 2020-10-13 11:03               ` Daniel Vetter
  0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-13 11:03 UTC (permalink / raw)
  To: Pekka Paalanen
  Cc: Haneen Mohammed, Thomas Zimmermann, Rodrigo Siqueira,
	Neil Armstrong, DRI Development, Melissa Wen, Daniel Vetter

On Tue, Oct 13, 2020 at 01:19:38PM +0300, Pekka Paalanen wrote:
> On Tue, 13 Oct 2020 09:53:44 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Oct 13, 2020 at 8:14 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >
> > > On Mon, 12 Oct 2020 16:23:35 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >  
> > > > On Mon, Oct 12, 2020 at 02:40:58PM +0200, Neil Armstrong wrote:  
> > > > > Hi,
> > > > >  
> > >
> > > ...
> > >  
> > > > > It's weird because it the kernel is misconfigured and no console is specified on the cmdline
> > > > > this console could become the main console...
> > > > >
> > > > > It's a great feature, but couldn't this be a module parameter ?  
> > > >
> > > > If you have vkms enabled in a distro, you're doing it wrong.  
> > >
> > > That's really not a great position to take. I would prefer that
> > > if a random contributor writes a Weston patch and runs 'meson test', it
> > > will use VKMS to run Weston's DRM-backend tests on his machine
> > > automatically, maybe save for some seat and device node access
> > > permissions bits which distributions could be delivering as well.
> > >
> > > Just put the VKMS device node into a non-default seat, and Xorg etc.
> > > will happily ignore it.
> > >
> > > For the fbdev device node, I don't know. Maybe a module parameter
> > > really is a good choice there, defaulting to off. I have no interest in
> > > testing anything against fbdev, but other people might disagree of
> > > course.
> > >
> > > Why? Gitlab CI is still not running tests for every commit, just per
> > > MR, and it might even be infeasible too.
> > >
> > > I am also hoping for a future where I don't have to build my own kernel
> > > just to be able to run Weston DRM tests with VKMS. That means I want to
> > > be able to run my machine with VKMS loaded and active at all times,
> > > without affecting the normal desktop. I already have such a setup with
> > > an extra AMD card, but you can't run most KMS tests against real
> > > hardware drivers.  
> > 
> > I just realized that building vkms is no problem, since it doesn't
> > auto-load. And if our Grand Plans with configurability come true, then
> > your test-harness will want to do that loading and setup itself
> > anyway. With that there also shouldn't be any problems with fbcon,
> > since presumably you already have that bound to the real gpu.
> > 
> > So I think we're all fine here, for everyone.
> > 
> > Now if you built-in vkms, that's a different thing. And for that I
> > really think a "don't do that" is the right choice.
> 
> Very good.
> 
> My remaining wish is that VKMS would be fully configurable and usable
> by an ordinary user, but I suppose that should be solved with a
> privileged userspace daemon somewhat similar to logind that hands out
> VKMS "sessions" somehow.
> 
> Not sure configfs is the best choice for VKMS configuration, unless
> maybe unprivileged userspace could ask for a VKMS instance with its own
> configfs tree it can access without CAP_ADMIN...

My idea for testing would be to build a gitlab docker image and run that
under usermode linux, with vkms built-in. Given how few people work on
this, I don't think you get a fancy logind configuration session thing for
it anytime soon.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* [PATCH] drm/vkms: Switch to shmem helpers
  2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
                     ` (2 preceding siblings ...)
  2020-10-12 12:41   ` Melissa Wen
@ 2020-10-13 11:10   ` Daniel Vetter
  3 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-13 11:10 UTC (permalink / raw)
  To: DRI Development
  Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, Daniel Vetter,
	Chris Wilson, Melissa Wen, Thomas Zimmermann, Daniel Vetter

Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
at the gem bo level, which we need for generic fbdev emulation.

Luckily shmem also tracks ->vaddr, so we just need to adjust the code
all over a bit to make this fit.

Also wire up handle_to_fd, dunno why that was missing.

v2:
- Drop now unused container_of #define (Melissa)
- Make sure we keep creating cached objects, this is for testing
  (Thomas)

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Melissa Wen <melissa.srw@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Melissa Wen <melissa.srw@gmail.com>
---
 drivers/gpu/drm/Kconfig               |   1 +
 drivers/gpu/drm/vkms/Makefile         |   1 -
 drivers/gpu/drm/vkms/vkms_composer.c  |  17 +-
 drivers/gpu/drm/vkms/vkms_drv.c       |  18 +-
 drivers/gpu/drm/vkms/vkms_drv.h       |  29 ---
 drivers/gpu/drm/vkms/vkms_gem.c       | 261 --------------------------
 drivers/gpu/drm/vkms/vkms_plane.c     |  13 +-
 drivers/gpu/drm/vkms/vkms_writeback.c |  17 +-
 8 files changed, 30 insertions(+), 327 deletions(-)
 delete mode 100644 drivers/gpu/drm/vkms/vkms_gem.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 9efb82caaa87..b796c118fc3b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -287,6 +287,7 @@ config DRM_VKMS
 	tristate "Virtual KMS (EXPERIMENTAL)"
 	depends on DRM
 	select DRM_KMS_HELPER
+	select DRM_GEM_SHMEM_HELPER
 	select CRC32
 	default n
 	help
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 333d3cead0e3..72f779cbfedd 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -4,7 +4,6 @@ vkms-y := \
 	vkms_plane.o \
 	vkms_output.o \
 	vkms_crtc.o \
-	vkms_gem.o \
 	vkms_composer.o \
 	vkms_writeback.o
 
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 33c031f27c2c..66c6842d70db 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -5,6 +5,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "vkms_drv.h"
@@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
 			   void *vaddr_out)
 {
 	struct drm_gem_object *cursor_obj;
-	struct vkms_gem_object *cursor_vkms_obj;
+	struct drm_gem_shmem_object *cursor_shmem_obj;
 
 	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
-	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
+	cursor_shmem_obj = to_drm_gem_shmem_obj(cursor_obj);
 
-	if (WARN_ON(!cursor_vkms_obj->vaddr))
+	if (WARN_ON(!cursor_shmem_obj->vaddr))
 		return;
 
-	blend(vaddr_out, cursor_vkms_obj->vaddr,
+	blend(vaddr_out, cursor_shmem_obj->vaddr,
 	      primary_composer, cursor_composer);
 }
 
@@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
 {
 	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
 
 	if (!*vaddr_out) {
-		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
+		*vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
 		if (!*vaddr_out) {
 			DRM_ERROR("Cannot allocate memory for output frame.");
 			return -ENOMEM;
 		}
 	}
 
-	if (WARN_ON(!vkms_obj->vaddr))
+	if (WARN_ON(!shmem_obj->vaddr))
 		return -EINVAL;
 
-	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
+	memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
 
 	if (cursor_composer)
 		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index eb4007443706..198a8941d6ea 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -23,6 +23,7 @@
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "vkms_drv.h"
@@ -39,17 +40,7 @@ bool enable_cursor = true;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
 
-static const struct file_operations vkms_driver_fops = {
-	.owner		= THIS_MODULE,
-	.open		= drm_open,
-	.mmap		= drm_gem_mmap,
-	.unlocked_ioctl	= drm_ioctl,
-	.compat_ioctl	= drm_compat_ioctl,
-	.poll		= drm_poll,
-	.read		= drm_read,
-	.llseek		= no_llseek,
-	.release	= drm_release,
-};
+DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
 static void vkms_release(struct drm_device *dev)
 {
@@ -91,9 +82,8 @@ static struct drm_driver vkms_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
 	.release		= vkms_release,
 	.fops			= &vkms_driver_fops,
-	.dumb_create		= vkms_dumb_create,
-	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
-	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
+	.gem_create_object = drm_gem_shmem_create_object_cached,
+	DRM_GEM_SHMEM_DRIVER_OPS,
 
 	.name			= DRIVER_NAME,
 	.desc			= DRIVER_DESC,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 380a8f27e156..5ed91ff08cb3 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -88,23 +88,12 @@ struct vkms_device {
 	struct vkms_output output;
 };
 
-struct vkms_gem_object {
-	struct drm_gem_object gem;
-	struct mutex pages_lock; /* Page lock used in page fault handler */
-	struct page **pages;
-	unsigned int vmap_count;
-	void *vaddr;
-};
-
 #define drm_crtc_to_vkms_output(target) \
 	container_of(target, struct vkms_output, crtc)
 
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
-#define drm_gem_to_vkms_gem(target)\
-	container_of(target, struct vkms_gem_object, gem)
-
 #define to_vkms_crtc_state(target)\
 	container_of(target, struct vkms_crtc_state, base)
 
@@ -120,24 +109,6 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index);
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 				  enum drm_plane_type type, int index);
 
-/* Gem stuff */
-vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
-
-int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
-		     struct drm_mode_create_dumb *args);
-
-void vkms_gem_free_object(struct drm_gem_object *obj);
-
-int vkms_gem_vmap(struct drm_gem_object *obj);
-
-void vkms_gem_vunmap(struct drm_gem_object *obj);
-
-/* Prime */
-struct drm_gem_object *
-vkms_prime_import_sg_table(struct drm_device *dev,
-			   struct dma_buf_attachment *attach,
-			   struct sg_table *sg);
-
 /* CRC Support */
 const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
 					size_t *count);
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
deleted file mode 100644
index 19a0e260a4df..000000000000
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ /dev/null
@@ -1,261 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-
-#include <linux/dma-buf.h>
-#include <linux/shmem_fs.h>
-#include <linux/vmalloc.h>
-#include <drm/drm_prime.h>
-
-#include "vkms_drv.h"
-
-static const struct vm_operations_struct vkms_gem_vm_ops = {
-	.fault = vkms_gem_fault,
-	.open = drm_gem_vm_open,
-	.close = drm_gem_vm_close,
-};
-
-static const struct drm_gem_object_funcs vkms_gem_object_funcs = {
-	.free = vkms_gem_free_object,
-	.vm_ops = &vkms_gem_vm_ops,
-};
-
-static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
-						 u64 size)
-{
-	struct vkms_gem_object *obj;
-	int ret;
-
-	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
-	if (!obj)
-		return ERR_PTR(-ENOMEM);
-
-	obj->gem.funcs = &vkms_gem_object_funcs;
-
-	size = roundup(size, PAGE_SIZE);
-	ret = drm_gem_object_init(dev, &obj->gem, size);
-	if (ret) {
-		kfree(obj);
-		return ERR_PTR(ret);
-	}
-
-	mutex_init(&obj->pages_lock);
-
-	return obj;
-}
-
-void vkms_gem_free_object(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
-						   gem);
-
-	WARN_ON(gem->pages);
-	WARN_ON(gem->vaddr);
-
-	mutex_destroy(&gem->pages_lock);
-	drm_gem_object_release(obj);
-	kfree(gem);
-}
-
-vm_fault_t vkms_gem_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct vkms_gem_object *obj = vma->vm_private_data;
-	unsigned long vaddr = vmf->address;
-	pgoff_t page_offset;
-	loff_t num_pages;
-	vm_fault_t ret = VM_FAULT_SIGBUS;
-
-	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
-	num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);
-
-	if (page_offset > num_pages)
-		return VM_FAULT_SIGBUS;
-
-	mutex_lock(&obj->pages_lock);
-	if (obj->pages) {
-		get_page(obj->pages[page_offset]);
-		vmf->page = obj->pages[page_offset];
-		ret = 0;
-	}
-	mutex_unlock(&obj->pages_lock);
-	if (ret) {
-		struct page *page;
-		struct address_space *mapping;
-
-		mapping = file_inode(obj->gem.filp)->i_mapping;
-		page = shmem_read_mapping_page(mapping, page_offset);
-
-		if (!IS_ERR(page)) {
-			vmf->page = page;
-			ret = 0;
-		} else {
-			switch (PTR_ERR(page)) {
-			case -ENOSPC:
-			case -ENOMEM:
-				ret = VM_FAULT_OOM;
-				break;
-			case -EBUSY:
-				ret = VM_FAULT_RETRY;
-				break;
-			case -EFAULT:
-			case -EINVAL:
-				ret = VM_FAULT_SIGBUS;
-				break;
-			default:
-				WARN_ON(PTR_ERR(page));
-				ret = VM_FAULT_SIGBUS;
-				break;
-			}
-		}
-	}
-	return ret;
-}
-
-static struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
-					      struct drm_file *file,
-					      u32 *handle,
-					      u64 size)
-{
-	struct vkms_gem_object *obj;
-	int ret;
-
-	if (!file || !dev || !handle)
-		return ERR_PTR(-EINVAL);
-
-	obj = __vkms_gem_create(dev, size);
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	ret = drm_gem_handle_create(file, &obj->gem, handle);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return &obj->gem;
-}
-
-int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
-		     struct drm_mode_create_dumb *args)
-{
-	struct drm_gem_object *gem_obj;
-	u64 pitch, size;
-
-	if (!args || !dev || !file)
-		return -EINVAL;
-
-	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
-	size = pitch * args->height;
-
-	if (!size)
-		return -EINVAL;
-
-	gem_obj = vkms_gem_create(dev, file, &args->handle, size);
-	if (IS_ERR(gem_obj))
-		return PTR_ERR(gem_obj);
-
-	args->size = gem_obj->size;
-	args->pitch = pitch;
-
-	drm_gem_object_put(gem_obj);
-
-	DRM_DEBUG_DRIVER("Created object of size %lld\n", size);
-
-	return 0;
-}
-
-static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
-{
-	struct drm_gem_object *gem_obj = &vkms_obj->gem;
-
-	if (!vkms_obj->pages) {
-		struct page **pages = drm_gem_get_pages(gem_obj);
-
-		if (IS_ERR(pages))
-			return pages;
-
-		if (cmpxchg(&vkms_obj->pages, NULL, pages))
-			drm_gem_put_pages(gem_obj, pages, false, true);
-	}
-
-	return vkms_obj->pages;
-}
-
-void vkms_gem_vunmap(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
-
-	mutex_lock(&vkms_obj->pages_lock);
-	if (vkms_obj->vmap_count < 1) {
-		WARN_ON(vkms_obj->vaddr);
-		WARN_ON(vkms_obj->pages);
-		mutex_unlock(&vkms_obj->pages_lock);
-		return;
-	}
-
-	vkms_obj->vmap_count--;
-
-	if (vkms_obj->vmap_count == 0) {
-		vunmap(vkms_obj->vaddr);
-		vkms_obj->vaddr = NULL;
-		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
-		vkms_obj->pages = NULL;
-	}
-
-	mutex_unlock(&vkms_obj->pages_lock);
-}
-
-int vkms_gem_vmap(struct drm_gem_object *obj)
-{
-	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
-	int ret = 0;
-
-	mutex_lock(&vkms_obj->pages_lock);
-
-	if (!vkms_obj->vaddr) {
-		unsigned int n_pages = obj->size >> PAGE_SHIFT;
-		struct page **pages = _get_pages(vkms_obj);
-
-		if (IS_ERR(pages)) {
-			ret = PTR_ERR(pages);
-			goto out;
-		}
-
-		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
-		if (!vkms_obj->vaddr)
-			goto err_vmap;
-	}
-
-	vkms_obj->vmap_count++;
-	goto out;
-
-err_vmap:
-	ret = -ENOMEM;
-	drm_gem_put_pages(obj, vkms_obj->pages, false, true);
-	vkms_obj->pages = NULL;
-out:
-	mutex_unlock(&vkms_obj->pages_lock);
-	return ret;
-}
-
-struct drm_gem_object *
-vkms_prime_import_sg_table(struct drm_device *dev,
-			   struct dma_buf_attachment *attach,
-			   struct sg_table *sg)
-{
-	struct vkms_gem_object *obj;
-	int npages;
-
-	obj = __vkms_gem_create(dev, attach->dmabuf->size);
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
-	DRM_DEBUG_PRIME("Importing %d pages\n", npages);
-
-	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
-	if (!obj->pages) {
-		vkms_gem_free_object(&obj->gem);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL, npages);
-	return &obj->gem;
-}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 6d31265a2ab7..9890137bcb8d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -5,6 +5,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 
 #include "vkms_drv.h"
 
@@ -145,15 +146,15 @@ static int vkms_prepare_fb(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
 	struct drm_gem_object *gem_obj;
-	int ret;
+	void *vaddr;
 
 	if (!state->fb)
 		return 0;
 
 	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
-	ret = vkms_gem_vmap(gem_obj);
-	if (ret)
-		DRM_ERROR("vmap failed: %d\n", ret);
+	vaddr = drm_gem_shmem_vmap(gem_obj);
+	if (IS_ERR(vaddr))
+		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
 
 	return drm_gem_fb_prepare_fb(plane, state);
 }
@@ -162,12 +163,14 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
 			    struct drm_plane_state *old_state)
 {
 	struct drm_gem_object *gem_obj;
+	struct drm_gem_shmem_object *shmem_obj;
 
 	if (!old_state->fb)
 		return;
 
 	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
-	vkms_gem_vunmap(gem_obj);
+	shmem_obj = to_drm_gem_shmem_obj(drm_gem_fb_get_obj(old_state->fb, 0));
+	drm_gem_shmem_vunmap(gem_obj, shmem_obj->vaddr);
 }
 
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 094fa4aa061d..26b903926872 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -6,6 +6,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 
 static const u32 vkms_wb_formats[] = {
 	DRM_FORMAT_XRGB8888,
@@ -63,22 +64,20 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
 static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 			       struct drm_writeback_job *job)
 {
-	struct vkms_gem_object *vkms_obj;
 	struct drm_gem_object *gem_obj;
-	int ret;
+	void *vaddr;
 
 	if (!job->fb)
 		return 0;
 
 	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	ret = vkms_gem_vmap(gem_obj);
-	if (ret) {
-		DRM_ERROR("vmap failed: %d\n", ret);
-		return ret;
+	vaddr = drm_gem_shmem_vmap(gem_obj);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("vmap failed: %li\n", PTR_ERR(vaddr));
+		return PTR_ERR(vaddr);
 	}
 
-	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
-	job->priv = vkms_obj->vaddr;
+	job->priv = vaddr;
 
 	return 0;
 }
@@ -93,7 +92,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 		return;
 
 	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	vkms_gem_vunmap(gem_obj);
+	drm_gem_shmem_vunmap(gem_obj, job->priv);
 
 	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
 	vkms_set_composer(&vkmsdev->output, false);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-12 12:59 ` Melissa Wen
@ 2020-10-15 21:39   ` Daniel Vetter
  2020-10-16 10:18     ` Melissa Wen
  0 siblings, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2020-10-15 21:39 UTC (permalink / raw)
  To: Melissa Wen
  Cc: Haneen Mohammed, Rodrigo Siqueira, Daniel Vetter,
	DRI Development, Daniel Vetter

On Mon, Oct 12, 2020 at 09:59:22AM -0300, Melissa Wen wrote:
> On 10/10, Daniel Vetter wrote:
> > The only thing we support is xrgb8888.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 726801ab44d4..eb4007443706 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >  	dev->mode_config.max_height = YRES_MAX;
> >  	dev->mode_config.cursor_width = 512;
> >  	dev->mode_config.cursor_height = 512;
> > -	dev->mode_config.preferred_depth = 24;
> > +	dev->mode_config.preferred_depth = 32;
> nice catch!
> 
> >  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
> >  
> >  	return vkms_output_init(vkmsdev, 0);
> > -- 
> > 2.28.0
> >
> Thanks, 
> 
> Reviewed-by: Melissa Wen <melissa.srw@gmail.com>

I merged the first 2 patches of this series, but noticed that you didn't
reply with a r-b tag on the 3rd patch. Is that intentional or just
oversight?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-09 23:21 ` [PATCH 3/3] drm/vkms: fbdev emulation support Daniel Vetter
  2020-10-12 11:24   ` Thomas Zimmermann
@ 2020-10-16 10:10   ` Melissa Wen
  2020-10-20  8:34     ` Daniel Vetter
  1 sibling, 1 reply; 29+ messages in thread
From: Melissa Wen @ 2020-10-16 10:10 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

Hi,

Thanks for this improvement.

I could see that it increased the IGT test coverage, including now the
fbdev test cases. 

On 10/10, Daniel Vetter wrote:
> Hooray for generic fbdev support, making this a oneliner. We just
> needed to fix preferred_depth fixed and the vmap support added first.

I consider that including in the msg that, with this patch, both fbdev
test cases [info and mmap] are passing would be interesting for future
debugs.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 6221e5040264..cc09e2df5cb1 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -169,6 +169,8 @@ static int __init vkms_init(void)
>  	if (ret)
>  		goto out_devres;
>  
> +	drm_fbdev_generic_setup(&vkms_device->drm, 0);
> +
>  	return 0;
>  
>  out_devres:
> -- 
> 2.28.0
>

Looks good to me,

Reviewed-by: Melissa Wen <melissa.srw@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-15 21:39   ` Daniel Vetter
@ 2020-10-16 10:18     ` Melissa Wen
  0 siblings, 0 replies; 29+ messages in thread
From: Melissa Wen @ 2020-10-16 10:18 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Haneen Mohammed, Rodrigo Siqueira,
	DRI Development, Daniel Vetter

On 10/15, Daniel Vetter wrote:
> On Mon, Oct 12, 2020 at 09:59:22AM -0300, Melissa Wen wrote:
> > On 10/10, Daniel Vetter wrote:
> > > The only thing we support is xrgb8888.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > Cc: Melissa Wen <melissa.srw@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 726801ab44d4..eb4007443706 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > >  	dev->mode_config.max_height = YRES_MAX;
> > >  	dev->mode_config.cursor_width = 512;
> > >  	dev->mode_config.cursor_height = 512;
> > > -	dev->mode_config.preferred_depth = 24;
> > > +	dev->mode_config.preferred_depth = 32;
> > nice catch!
> > 
> > >  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
> > >  
> > >  	return vkms_output_init(vkmsdev, 0);
> > > -- 
> > > 2.28.0
> > >
> > Thanks, 
> > 
> > Reviewed-by: Melissa Wen <melissa.srw@gmail.com>
> 
> I merged the first 2 patches of this series, but noticed that you didn't
> reply with a r-b tag on the 3rd patch. Is that intentional or just
> oversight?
Hi Daniel,

Initially, it was intentional because I was following the feedback and
still wanted to check it out better, then I forgot to come back to
comment.

My fault, but it's done now.

Thanks for the touch,

Melissa

> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
                   ` (3 preceding siblings ...)
  2020-10-12 12:59 ` Melissa Wen
@ 2020-10-16 10:38 ` Simon Ser
  2020-10-16 11:35   ` Daniel Vetter
  4 siblings, 1 reply; 29+ messages in thread
From: Simon Ser @ 2020-10-16 10:38 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Melissa Wen, Haneen Mohammed, Rodrigo Siqueira, DRI Development,
	Daniel Vetter

> The only thing we support is xrgb8888.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 726801ab44d4..eb4007443706 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.max_height = YRES_MAX;
>  	dev->mode_config.cursor_width = 512;
>  	dev->mode_config.cursor_height = 512;
> -	dev->mode_config.preferred_depth = 24;
> +	dev->mode_config.preferred_depth = 32;

Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
drivers set it to 24.

[1]: https://drmdb.emersion.fr/capabilities

>  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
>  	return vkms_output_init(vkmsdev, 0);
> --
> 2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-16 10:38 ` Simon Ser
@ 2020-10-16 11:35   ` Daniel Vetter
  2020-10-16 17:02     ` Melissa Wen
  2020-10-27 10:13     ` Thomas Zimmermann
  0 siblings, 2 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-16 11:35 UTC (permalink / raw)
  To: Simon Ser
  Cc: Melissa Wen, Haneen Mohammed, Rodrigo Siqueira, DRI Development,
	Daniel Vetter

On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
>
> > The only thing we support is xrgb8888.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 726801ab44d4..eb4007443706 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >       dev->mode_config.max_height = YRES_MAX;
> >       dev->mode_config.cursor_width = 512;
> >       dev->mode_config.cursor_height = 512;
> > -     dev->mode_config.preferred_depth = 24;
> > +     dev->mode_config.preferred_depth = 32;
>
> Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> drivers set it to 24.

Uh there's a problem I think. Depth should indeed stay at 24, the
problem is that fb helpers directly take that to be the bpp parameter,
which is a different thing. And if you look at how most drivers set up
that, they pick 32.

I guess I need to revert this here, and unconfuse the fb helpers about
depth vs bpp.

Maybe best would be to just switch over to preferred drm_fourcc format
code, or maybe just pick this up from the first format the primary
plane supports.

This is all getting slightly tricky and a lot more work :-/
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-16 11:35   ` Daniel Vetter
@ 2020-10-16 17:02     ` Melissa Wen
  2020-10-16 17:05       ` Daniel Vetter
  2020-10-27 10:13     ` Thomas Zimmermann
  1 sibling, 1 reply; 29+ messages in thread
From: Melissa Wen @ 2020-10-16 17:02 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

On 10/16, Daniel Vetter wrote:
> On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
> >
> > > The only thing we support is xrgb8888.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > Cc: Melissa Wen <melissa.srw@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 726801ab44d4..eb4007443706 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > >       dev->mode_config.max_height = YRES_MAX;
> > >       dev->mode_config.cursor_width = 512;
> > >       dev->mode_config.cursor_height = 512;
> > > -     dev->mode_config.preferred_depth = 24;
> > > +     dev->mode_config.preferred_depth = 32;
> >
> > Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> > drivers set it to 24.
> 
> Uh there's a problem I think. Depth should indeed stay at 24, the
> problem is that fb helpers directly take that to be the bpp parameter,
> which is a different thing. And if you look at how most drivers set up
> that, they pick 32.
> 
> I guess I need to revert this here, and unconfuse the fb helpers about
> depth vs bpp.

Hi guys,

Perhaps it deserves to be pointed out: the documentation says
"preferred_depth: preferred RBG(sic) pixel depth, used by fb helpers",
and looking to fb helpers, preferred_depth is only used by
generic_setup, as bits by pixel (if I didn't miss something there).

In fact, the alpha channel is not used for final display (perhaps in the
future); however, I saw another driver with a change similar to this
here and, possibly like me, following the same misunderstanding.

Melissa
> 
> Maybe best would be to just switch over to preferred drm_fourcc format
> code, or maybe just pick this up from the first format the primary
> plane supports.
> 
> This is all getting slightly tricky and a lot more work :-/
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-16 17:02     ` Melissa Wen
@ 2020-10-16 17:05       ` Daniel Vetter
  2020-10-17  8:39         ` Melissa Wen
  0 siblings, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2020-10-16 17:05 UTC (permalink / raw)
  To: Melissa Wen
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

On Fri, Oct 16, 2020 at 7:02 PM Melissa Wen <melissa.srw@gmail.com> wrote:
>
> On 10/16, Daniel Vetter wrote:
> > On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
> > >
> > > > The only thing we support is xrgb8888.
> > > >
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > Cc: Melissa Wen <melissa.srw@gmail.com>
> > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > index 726801ab44d4..eb4007443706 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > > >       dev->mode_config.max_height = YRES_MAX;
> > > >       dev->mode_config.cursor_width = 512;
> > > >       dev->mode_config.cursor_height = 512;
> > > > -     dev->mode_config.preferred_depth = 24;
> > > > +     dev->mode_config.preferred_depth = 32;
> > >
> > > Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> > > drivers set it to 24.
> >
> > Uh there's a problem I think. Depth should indeed stay at 24, the
> > problem is that fb helpers directly take that to be the bpp parameter,
> > which is a different thing. And if you look at how most drivers set up
> > that, they pick 32.
> >
> > I guess I need to revert this here, and unconfuse the fb helpers about
> > depth vs bpp.
>
> Hi guys,
>
> Perhaps it deserves to be pointed out: the documentation says
> "preferred_depth: preferred RBG(sic) pixel depth, used by fb helpers",
> and looking to fb helpers, preferred_depth is only used by
> generic_setup, as bits by pixel (if I didn't miss something there).
>
> In fact, the alpha channel is not used for final display (perhaps in the
> future); however, I saw another driver with a change similar to this
> here and, possibly like me, following the same misunderstanding.

Yeah the problem is that preferred_depth is depth, and that means 24
bit for XRGB8888. But bpp as used by fb helpers would be 32 bit for
XRGB8888.

I think the real fix here is to switch this entire mess over to using
drm_fourcc codes directly, at least for atomic drivers. Which nowadays
are most. Interim I'm not sure whether we should revert my patch (it
breaks fbdev) or switch preferred_depth to 0, which means we get the
default every, and that means both fbdev helpers and userspace will
pick XRGB8888.
-Daniel

> Melissa
> >
> > Maybe best would be to just switch over to preferred drm_fourcc format
> > code, or maybe just pick this up from the first format the primary
> > plane supports.
> >
> > This is all getting slightly tricky and a lot more work :-/
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-16 17:05       ` Daniel Vetter
@ 2020-10-17  8:39         ` Melissa Wen
  2020-10-17 15:23           ` Daniel Vetter
  0 siblings, 1 reply; 29+ messages in thread
From: Melissa Wen @ 2020-10-17  8:39 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

On 10/16, Daniel Vetter wrote:
> On Fri, Oct 16, 2020 at 7:02 PM Melissa Wen <melissa.srw@gmail.com> wrote:
> >
> > On 10/16, Daniel Vetter wrote:
> > > On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
> > > >
> > > > > The only thing we support is xrgb8888.
> > > > >
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > > Cc: Melissa Wen <melissa.srw@gmail.com>
> > > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > index 726801ab44d4..eb4007443706 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > > > >       dev->mode_config.max_height = YRES_MAX;
> > > > >       dev->mode_config.cursor_width = 512;
> > > > >       dev->mode_config.cursor_height = 512;
> > > > > -     dev->mode_config.preferred_depth = 24;
> > > > > +     dev->mode_config.preferred_depth = 32;
> > > >
> > > > Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> > > > drivers set it to 24.
> > >
> > > Uh there's a problem I think. Depth should indeed stay at 24, the
> > > problem is that fb helpers directly take that to be the bpp parameter,
> > > which is a different thing. And if you look at how most drivers set up
> > > that, they pick 32.
> > >
> > > I guess I need to revert this here, and unconfuse the fb helpers about
> > > depth vs bpp.
> >
> > Hi guys,
> >
> > Perhaps it deserves to be pointed out: the documentation says
> > "preferred_depth: preferred RBG(sic) pixel depth, used by fb helpers",
> > and looking to fb helpers, preferred_depth is only used by
> > generic_setup, as bits by pixel (if I didn't miss something there).
> >
> > In fact, the alpha channel is not used for final display (perhaps in the
> > future); however, I saw another driver with a change similar to this
> > here and, possibly like me, following the same misunderstanding.
> 
> Yeah the problem is that preferred_depth is depth, and that means 24
> bit for XRGB8888. But bpp as used by fb helpers would be 32 bit for
> XRGB8888.
> 
> I think the real fix here is to switch this entire mess over to using
> drm_fourcc codes directly, at least for atomic drivers. Which nowadays
> are most. Interim I'm not sure whether we should revert my patch (it
> breaks fbdev) or switch preferred_depth to 0, which means we get the
> default every, and that means both fbdev helpers and userspace will
> pick XRGB8888.

hmm... but why not keep preferred_depth = 24 and pass 32 as the
preferred_bpp parameter of fbdev_generic_setup?

> -Daniel
> 
> > Melissa
> > >
> > > Maybe best would be to just switch over to preferred drm_fourcc format
> > > code, or maybe just pick this up from the first format the primary
> > > plane supports.
> > >
> > > This is all getting slightly tricky and a lot more work :-/
> > > -Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-17  8:39         ` Melissa Wen
@ 2020-10-17 15:23           ` Daniel Vetter
  0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-17 15:23 UTC (permalink / raw)
  To: Melissa Wen
  Cc: Daniel Vetter, Rodrigo Siqueira, DRI Development, Haneen Mohammed

On Sat, Oct 17, 2020 at 10:39 AM Melissa Wen <melissa.srw@gmail.com> wrote:
>
> On 10/16, Daniel Vetter wrote:
> > On Fri, Oct 16, 2020 at 7:02 PM Melissa Wen <melissa.srw@gmail.com> wrote:
> > >
> > > On 10/16, Daniel Vetter wrote:
> > > > On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
> > > > >
> > > > > > The only thing we support is xrgb8888.
> > > > > >
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > > > Cc: Melissa Wen <melissa.srw@gmail.com>
> > > > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > ---
> > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > index 726801ab44d4..eb4007443706 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > > > > >       dev->mode_config.max_height = YRES_MAX;
> > > > > >       dev->mode_config.cursor_width = 512;
> > > > > >       dev->mode_config.cursor_height = 512;
> > > > > > -     dev->mode_config.preferred_depth = 24;
> > > > > > +     dev->mode_config.preferred_depth = 32;
> > > > >
> > > > > Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> > > > > drivers set it to 24.
> > > >
> > > > Uh there's a problem I think. Depth should indeed stay at 24, the
> > > > problem is that fb helpers directly take that to be the bpp parameter,
> > > > which is a different thing. And if you look at how most drivers set up
> > > > that, they pick 32.
> > > >
> > > > I guess I need to revert this here, and unconfuse the fb helpers about
> > > > depth vs bpp.
> > >
> > > Hi guys,
> > >
> > > Perhaps it deserves to be pointed out: the documentation says
> > > "preferred_depth: preferred RBG(sic) pixel depth, used by fb helpers",
> > > and looking to fb helpers, preferred_depth is only used by
> > > generic_setup, as bits by pixel (if I didn't miss something there).
> > >
> > > In fact, the alpha channel is not used for final display (perhaps in the
> > > future); however, I saw another driver with a change similar to this
> > > here and, possibly like me, following the same misunderstanding.
> >
> > Yeah the problem is that preferred_depth is depth, and that means 24
> > bit for XRGB8888. But bpp as used by fb helpers would be 32 bit for
> > XRGB8888.
> >
> > I think the real fix here is to switch this entire mess over to using
> > drm_fourcc codes directly, at least for atomic drivers. Which nowadays
> > are most. Interim I'm not sure whether we should revert my patch (it
> > breaks fbdev) or switch preferred_depth to 0, which means we get the
> > default every, and that means both fbdev helpers and userspace will
> > pick XRGB8888.
>
> hmm... but why not keep preferred_depth = 24 and pass 32 as the
> preferred_bpp parameter of fbdev_generic_setup?

The goal is to get rid of this parameter in fbdev_generic_setup. It
should be able to figure this out automatically, like any kms client
in userspace.

What does work is setting preferred_bpp = 0. Userspace will pick the
default (which is generally 24 depth, 32bpp), and fbcon do the same.

And then I guess a nice patch series to clean up this mess.
-Daniel

> > -Daniel
> >
> > > Melissa
> > > >
> > > > Maybe best would be to just switch over to preferred drm_fourcc format
> > > > code, or maybe just pick this up from the first format the primary
> > > > plane supports.
> > > >
> > > > This is all getting slightly tricky and a lot more work :-/
> > > > -Daniel
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 3/3] drm/vkms: fbdev emulation support
  2020-10-16 10:10   ` Melissa Wen
@ 2020-10-20  8:34     ` Daniel Vetter
  0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-20  8:34 UTC (permalink / raw)
  To: Melissa Wen
  Cc: Haneen Mohammed, Rodrigo Siqueira, Daniel Vetter,
	DRI Development, Daniel Vetter

On Fri, Oct 16, 2020 at 07:10:56AM -0300, Melissa Wen wrote:
> Hi,
> 
> Thanks for this improvement.
> 
> I could see that it increased the IGT test coverage, including now the
> fbdev test cases. 
> 
> On 10/10, Daniel Vetter wrote:
> > Hooray for generic fbdev support, making this a oneliner. We just
> > needed to fix preferred_depth fixed and the vmap support added first.
> 
> I consider that including in the msg that, with this patch, both fbdev
> test cases [info and mmap] are passing would be interesting for future
> debugs.

Done and patch applied, thanks for taking a look. I'll also follow up with
a quick patch to paper over the fbdev vs preferred_depth issue, until we
have a proper solution for all that.
-Daniel

> 
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 6221e5040264..cc09e2df5cb1 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -169,6 +169,8 @@ static int __init vkms_init(void)
> >  	if (ret)
> >  		goto out_devres;
> >  
> > +	drm_fbdev_generic_setup(&vkms_device->drm, 0);
> > +
> >  	return 0;
> >  
> >  out_devres:
> > -- 
> > 2.28.0
> >
> 
> Looks good to me,
> 
> Reviewed-by: Melissa Wen <melissa.srw@gmail.com>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-16 11:35   ` Daniel Vetter
  2020-10-16 17:02     ` Melissa Wen
@ 2020-10-27 10:13     ` Thomas Zimmermann
  2020-10-27 10:18       ` Daniel Vetter
  1 sibling, 1 reply; 29+ messages in thread
From: Thomas Zimmermann @ 2020-10-27 10:13 UTC (permalink / raw)
  To: Daniel Vetter, Simon Ser
  Cc: Melissa Wen, Haneen Mohammed, DRI Development, Rodrigo Siqueira,
	Daniel Vetter

Hi

Am 16.10.20 um 13:35 schrieb Daniel Vetter:
> On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
>>
>>> The only thing we support is xrgb8888.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>>> Cc: Melissa Wen <melissa.srw@gmail.com>
>>> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>>> index 726801ab44d4..eb4007443706 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>>> @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>>>       dev->mode_config.max_height = YRES_MAX;
>>>       dev->mode_config.cursor_width = 512;
>>>       dev->mode_config.cursor_height = 512;
>>> -     dev->mode_config.preferred_depth = 24;
>>> +     dev->mode_config.preferred_depth = 32;
>>
>> Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
>> drivers set it to 24.
> 
> Uh there's a problem I think. Depth should indeed stay at 24, the
> problem is that fb helpers directly take that to be the bpp parameter,
> which is a different thing. And if you look at how most drivers set up
> that, they pick 32.
> 
> I guess I need to revert this here, and unconfuse the fb helpers about
> depth vs bpp.

I just prepared the PR for drm-misc-next, but saw it at 32 still. Was
this supposed to be reverted?

Best regards
Thomas

> 
> Maybe best would be to just switch over to preferred drm_fourcc format
> code, or maybe just pick this up from the first format the primary
> plane supports.
> 
> This is all getting slightly tricky and a lot more work :-/
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [PATCH 1/3] drm/vkms: Set preferred depth correctly
  2020-10-27 10:13     ` Thomas Zimmermann
@ 2020-10-27 10:18       ` Daniel Vetter
  0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2020-10-27 10:18 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: Haneen Mohammed, Rodrigo Siqueira, DRI Development, Melissa Wen,
	Daniel Vetter

On Tue, Oct 27, 2020 at 11:13 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 16.10.20 um 13:35 schrieb Daniel Vetter:
> > On Fri, Oct 16, 2020 at 12:38 PM Simon Ser <contact@emersion.fr> wrote:
> >>
> >>> The only thing we support is xrgb8888.
> >>>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >>> Cc: Melissa Wen <melissa.srw@gmail.com>
> >>> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> >>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>> ---
> >>>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> index 726801ab44d4..eb4007443706 100644
> >>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> >>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> >>> @@ -124,7 +124,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >>>       dev->mode_config.max_height = YRES_MAX;
> >>>       dev->mode_config.cursor_width = 512;
> >>>       dev->mode_config.cursor_height = 512;
> >>> -     dev->mode_config.preferred_depth = 24;
> >>> +     dev->mode_config.preferred_depth = 32;
> >>
> >> Are you sure xrgb8888's depth is 32 and not 24? Looking at drmdb [1], *all*
> >> drivers set it to 24.
> >
> > Uh there's a problem I think. Depth should indeed stay at 24, the
> > problem is that fb helpers directly take that to be the bpp parameter,
> > which is a different thing. And if you look at how most drivers set up
> > that, they pick 32.
> >
> > I guess I need to revert this here, and unconfuse the fb helpers about
> > depth vs bpp.
>
> I just prepared the PR for drm-misc-next, but saw it at 32 still. Was
> this supposed to be reverted?

Revert makes fbdev go boom, only thing that would work is leaving it
at 0 so that everyone picks the default. I've tried to come up with a
patch series to clean up this mess, but it's rather horrible :-/
-Daniel

>
> Best regards
> Thomas
>
> >
> > Maybe best would be to just switch over to preferred drm_fourcc format
> > code, or maybe just pick this up from the first format the primary
> > plane supports.
> >
> > This is all getting slightly tricky and a lot more work :-/
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

end of thread, other threads:[~2020-10-27 10:18 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 23:21 [PATCH 1/3] drm/vkms: Set preferred depth correctly Daniel Vetter
2020-10-09 23:21 ` [PATCH 2/3] drm/vkms: Switch to shmem helpers Daniel Vetter
2020-10-12 10:59   ` Chris Wilson
2020-10-12 11:20     ` Thomas Zimmermann
2020-10-12 11:22   ` Thomas Zimmermann
2020-10-12 12:41   ` Melissa Wen
2020-10-13 11:10   ` [PATCH] " Daniel Vetter
2020-10-09 23:21 ` [PATCH 3/3] drm/vkms: fbdev emulation support Daniel Vetter
2020-10-12 11:24   ` Thomas Zimmermann
2020-10-12 12:40     ` Neil Armstrong
2020-10-12 14:23       ` Daniel Vetter
2020-10-13  6:14         ` Pekka Paalanen
2020-10-13  7:53           ` Daniel Vetter
2020-10-13 10:19             ` Pekka Paalanen
2020-10-13 11:03               ` Daniel Vetter
2020-10-16 10:10   ` Melissa Wen
2020-10-20  8:34     ` Daniel Vetter
2020-10-12 11:08 ` [PATCH 1/3] drm/vkms: Set preferred depth correctly Thomas Zimmermann
2020-10-12 12:59 ` Melissa Wen
2020-10-15 21:39   ` Daniel Vetter
2020-10-16 10:18     ` Melissa Wen
2020-10-16 10:38 ` Simon Ser
2020-10-16 11:35   ` Daniel Vetter
2020-10-16 17:02     ` Melissa Wen
2020-10-16 17:05       ` Daniel Vetter
2020-10-17  8:39         ` Melissa Wen
2020-10-17 15:23           ` Daniel Vetter
2020-10-27 10:13     ` Thomas Zimmermann
2020-10-27 10:18       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.