Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
3030 discussions

[PATCH 6.6 049/109] drm/framebuffer: Acquire internal references on GEM handles

by Greg Kroah-Hartman

6.6-stable review patch. If anyone has any objections, please let me know. ------------------ From: Thomas Zimmermann <tzimmermann(a)suse.de> commit f6bfc9afc7510cb5e6fbe0a17c507917b0120280 upstream. Acquire GEM handles in drm_framebuffer_init() and release them in the corresponding drm_framebuffer_cleanup(). Ties the handle's lifetime to the framebuffer. Not all GEM buffer objects have GEM handles. If not set, no refcounting takes place. This is the case for some fbdev emulation. This is not a problem as these GEM objects do not use dma-bufs and drivers will not release them while fbdev emulation is running. Framebuffer flags keep a bit per color plane of which the framebuffer holds a GEM handle reference. As all drivers use drm_framebuffer_init(), they will now all hold dma-buf references as fixed in commit 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers"). In the GEM framebuffer helpers, restore the original ref counting on buffer objects. As the helpers for handle refcounting are now no longer called from outside the DRM core, unexport the symbols. v3: - don't mix internal flags with mode flags (Christian) v2: - track framebuffer handle refs by flag - drop gma500 cleanup (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers") Reported-by: Bert Karwatzki <spasswolf(a)web.de> Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/ Tested-by: Bert Karwatzki <spasswolf(a)web.de> Tested-by: Mario Limonciello <superm1(a)kernel.org> Tested-by: Borislav Petkov (AMD) <bp(a)alien8.de> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://lore.kernel.org/r/20250707131224.249496-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_framebuffer.c | 31 ++++++++++++++++++++-- drivers/gpu/drm/drm_gem.c | 38 +++++++++++++++++---------- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++------- drivers/gpu/drm/drm_internal.h | 2 - include/drm/drm_framebuffer.h | 7 ++++ 5 files changed, 68 insertions(+), 26 deletions(-) --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -844,11 +844,23 @@ void drm_framebuffer_free(struct kref *k int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, const struct drm_framebuffer_funcs *funcs) { + unsigned int i; int ret; + bool exists; if (WARN_ON_ONCE(fb->dev != dev || !fb->format)) return -EINVAL; + for (i = 0; i < fb->format->num_planes; i++) { + if (drm_WARN_ON_ONCE(dev, fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i))) + fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + if (fb->obj[i]) { + exists = drm_gem_object_handle_get_if_exists_unlocked(fb->obj[i]); + if (exists) + fb->internal_flags |= DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + } + } + INIT_LIST_HEAD(&fb->filp_head); fb->funcs = funcs; @@ -857,7 +869,7 @@ int drm_framebuffer_init(struct drm_devi ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB, false, drm_framebuffer_free); if (ret) - goto out; + goto err; mutex_lock(&dev->mode_config.fb_lock); dev->mode_config.num_fb++; @@ -865,7 +877,16 @@ int drm_framebuffer_init(struct drm_devi mutex_unlock(&dev->mode_config.fb_lock); drm_mode_object_register(dev, &fb->base); -out: + + return 0; + +err: + for (i = 0; i < fb->format->num_planes; i++) { + if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) { + drm_gem_object_handle_put_unlocked(fb->obj[i]); + fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + } + } return ret; } EXPORT_SYMBOL(drm_framebuffer_init); @@ -942,6 +963,12 @@ EXPORT_SYMBOL(drm_framebuffer_unregister void drm_framebuffer_cleanup(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev; + unsigned int i; + + for (i = 0; i < fb->format->num_planes; i++) { + if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) + drm_gem_object_handle_put_unlocked(fb->obj[i]); + } mutex_lock(&dev->mode_config.fb_lock); list_del(&fb->head); --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -197,23 +197,34 @@ static void drm_gem_object_handle_get(st } /** - * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * drm_gem_object_handle_get_if_exists_unlocked - acquire reference on user-space handle, if any * @obj: GEM object * - * Acquires a reference on the GEM buffer object's handle. Required - * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() - * to release the reference. + * Acquires a reference on the GEM buffer object's handle. Required to keep + * the GEM object alive. Call drm_gem_object_handle_put_if_exists_unlocked() + * to release the reference. Does nothing if the buffer object has no handle. + * + * Returns: + * True if a handle exists, or false otherwise */ -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; guard(mutex)(&dev->object_name_lock); - drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */ + /* + * First ref taken during GEM object creation, if any. Some + * drivers set up internal framebuffers with GEM objects that + * do not have a GEM handle. Hence, this counter can be zero. + */ + if (!obj->handle_count) + return false; + drm_gem_object_handle_get(obj); + + return true; } -EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); /** * drm_gem_object_handle_free - release resources bound to userspace handles @@ -246,7 +257,7 @@ static void drm_gem_object_exported_dma_ } /** - * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * drm_gem_object_handle_put_unlocked - releases reference on user-space handle * @obj: GEM object * * Releases a reference on the GEM buffer object's handle. Possibly releases @@ -257,14 +268,14 @@ void drm_gem_object_handle_put_unlocked( struct drm_device *dev = obj->dev; bool final = false; - if (WARN_ON(READ_ONCE(obj->handle_count) == 0)) + if (drm_WARN_ON(dev, READ_ONCE(obj->handle_count) == 0)) return; /* - * Must bump handle count first as this may be the last - * ref, in which case the object would disappear before we - * checked for a name - */ + * Must bump handle count first as this may be the last + * ref, in which case the object would disappear before + * we checked for a name. + */ mutex_lock(&dev->object_name_lock); if (--obj->handle_count == 0) { @@ -277,7 +288,6 @@ void drm_gem_object_handle_put_unlocked( if (final) drm_gem_object_put(obj); } -EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -99,7 +99,7 @@ void drm_gem_fb_destroy(struct drm_frame unsigned int i; for (i = 0; i < fb->format->num_planes; i++) - drm_gem_object_handle_put_unlocked(fb->obj[i]); + drm_gem_object_put(fb->obj[i]); drm_framebuffer_cleanup(fb); kfree(fb); @@ -182,10 +182,8 @@ int drm_gem_fb_init_with_funcs(struct dr if (!objs[i]) { drm_dbg_kms(dev, "Failed to lookup GEM object\n"); ret = -ENOENT; - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; } - drm_gem_object_handle_get_unlocked(objs[i]); - drm_gem_object_put(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -195,22 +193,22 @@ int drm_gem_fb_init_with_funcs(struct dr drm_dbg_kms(dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); - drm_gem_object_handle_put_unlocked(objs[i]); + drm_gem_object_put(objs[i]); ret = -EINVAL; - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; } } ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs); if (ret) - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; return 0; -err_gem_object_handle_put_unlocked: +err_gem_object_put: while (i > 0) { --i; - drm_gem_object_handle_put_unlocked(objs[i]); + drm_gem_object_put(objs[i]); } return ret; } --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -155,7 +155,7 @@ void drm_sysfs_lease_event(struct drm_de /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj); void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -23,6 +23,7 @@ #ifndef __DRM_FRAMEBUFFER_H__ #define __DRM_FRAMEBUFFER_H__ +#include <linux/bits.h> #include <linux/ctype.h> #include <linux/list.h> #include <linux/sched.h> @@ -100,6 +101,8 @@ struct drm_framebuffer_funcs { unsigned num_clips); }; +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i) BIT(0u + (_i)) + /** * struct drm_framebuffer - frame buffer object * @@ -189,6 +192,10 @@ struct drm_framebuffer { */ int flags; /** + * @internal_flags: Framebuffer flags like DRM_FRAMEBUFFER_HAS_HANDLE_REF. + */ + unsigned int internal_flags; + /** * @hot_x: X coordinate of the cursor hotspot. Used by the legacy cursor * IOCTL when the driver supports cursor through a DRM_PLANE_TYPE_CURSOR * universal plane.

2 months

[PATCH 6.6 043/109] drm/gem: Acquire references on GEM handles for framebuffers

by Greg Kroah-Hartman

6.6-stable review patch. If anyone has any objections, please let me know. ------------------ From: Thomas Zimmermann <tzimmermann(a)suse.de> commit 5307dce878d4126e1b375587318955bd019c3741 upstream. A GEM handle can be released while the GEM buffer object is attached to a DRM framebuffer. This leads to the release of the dma-buf backing the buffer object, if any. [1] Trying to use the framebuffer in further mode-setting operations leads to a segmentation fault. Most easily happens with driver that use shadow planes for vmap-ing the dma-buf during a page flip. An example is shown below. [ 156.791968] ------------[ cut here ]------------ [ 156.796830] WARNING: CPU: 2 PID: 2255 at drivers/dma-buf/dma-buf.c:1527 dma_buf_vmap+0x224/0x430 [...] [ 156.942028] RIP: 0010:dma_buf_vmap+0x224/0x430 [ 157.043420] Call Trace: [ 157.045898] <TASK> [ 157.048030] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.052436] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.056836] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.061253] ? drm_gem_shmem_vmap+0x74/0x710 [ 157.065567] ? dma_buf_vmap+0x224/0x430 [ 157.069446] ? __warn.cold+0x58/0xe4 [ 157.073061] ? dma_buf_vmap+0x224/0x430 [ 157.077111] ? report_bug+0x1dd/0x390 [ 157.080842] ? handle_bug+0x5e/0xa0 [ 157.084389] ? exc_invalid_op+0x14/0x50 [ 157.088291] ? asm_exc_invalid_op+0x16/0x20 [ 157.092548] ? dma_buf_vmap+0x224/0x430 [ 157.096663] ? dma_resv_get_singleton+0x6d/0x230 [ 157.101341] ? __pfx_dma_buf_vmap+0x10/0x10 [ 157.105588] ? __pfx_dma_resv_get_singleton+0x10/0x10 [ 157.110697] drm_gem_shmem_vmap+0x74/0x710 [ 157.114866] drm_gem_vmap+0xa9/0x1b0 [ 157.118763] drm_gem_vmap_unlocked+0x46/0xa0 [ 157.123086] drm_gem_fb_vmap+0xab/0x300 [ 157.126979] drm_atomic_helper_prepare_planes.part.0+0x487/0xb10 [ 157.133032] ? lockdep_init_map_type+0x19d/0x880 [ 157.137701] drm_atomic_helper_commit+0x13d/0x2e0 [ 157.142671] ? drm_atomic_nonblocking_commit+0xa0/0x180 [ 157.147988] drm_mode_atomic_ioctl+0x766/0xe40 [...] [ 157.346424] ---[ end trace 0000000000000000 ]--- Acquiring GEM handles for the framebuffer's GEM buffer objects prevents this from happening. The framebuffer's cleanup later puts the handle references. Commit 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance") triggers the segmentation fault easily by using the dma-buf field more widely. The underlying issue with reference counting has been present before. v2: - acquire the handle instead of the BO (Christian) - fix comment style (Christian) - drop the Fixes tag (Christian) - rename err_ gotos - add missing Link tag Suggested-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Link: https://elixir.bootlin.com/linux/v6.15/source/drivers/gpu/drm/drm_gem.c#L241 # [1] Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://lore.kernel.org/r/20250630084001.293053-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 44 ++++++++++++++++++++++++--- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 +++++---- drivers/gpu/drm/drm_internal.h | 2 + 3 files changed, 51 insertions(+), 11 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -186,6 +186,35 @@ void drm_gem_private_object_fini(struct } EXPORT_SYMBOL(drm_gem_private_object_fini); +static void drm_gem_object_handle_get(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock)); + + if (obj->handle_count++ == 0) + drm_gem_object_get(obj); +} + +/** + * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * @obj: GEM object + * + * Acquires a reference on the GEM buffer object's handle. Required + * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() + * to release the reference. + */ +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + guard(mutex)(&dev->object_name_lock); + + drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */ + drm_gem_object_handle_get(obj); +} +EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); + /** * drm_gem_object_handle_free - release resources bound to userspace handles * @obj: GEM object to clean up. @@ -216,8 +245,14 @@ static void drm_gem_object_exported_dma_ } } -static void -drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) +/** + * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * @obj: GEM object + * + * Releases a reference on the GEM buffer object's handle. Possibly releases + * the GEM buffer object and associated dma-buf objects. + */ +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; bool final = false; @@ -242,6 +277,7 @@ drm_gem_object_handle_put_unlocked(struc if (final) drm_gem_object_put(obj); } +EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's @@ -363,8 +399,8 @@ drm_gem_handle_create_tail(struct drm_fi int ret; WARN_ON(!mutex_is_locked(&dev->object_name_lock)); - if (obj->handle_count++ == 0) - drm_gem_object_get(obj); + + drm_gem_object_handle_get(obj); /* * Get the user-visible handle using idr. Preload and perform --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -99,7 +99,7 @@ void drm_gem_fb_destroy(struct drm_frame unsigned int i; for (i = 0; i < fb->format->num_planes; i++) - drm_gem_object_put(fb->obj[i]); + drm_gem_object_handle_put_unlocked(fb->obj[i]); drm_framebuffer_cleanup(fb); kfree(fb); @@ -182,8 +182,10 @@ int drm_gem_fb_init_with_funcs(struct dr if (!objs[i]) { drm_dbg_kms(dev, "Failed to lookup GEM object\n"); ret = -ENOENT; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } + drm_gem_object_handle_get_unlocked(objs[i]); + drm_gem_object_put(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -193,22 +195,22 @@ int drm_gem_fb_init_with_funcs(struct dr drm_dbg_kms(dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); ret = -EINVAL; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } } ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs); if (ret) - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; return 0; -err_gem_object_put: +err_gem_object_handle_put_unlocked: while (i > 0) { --i; - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); } return ret; } --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -155,6 +155,8 @@ void drm_sysfs_lease_event(struct drm_de /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, u32 *handlep);

2 months

[PATCH 6.12 082/163] drm/framebuffer: Acquire internal references on GEM handles

by Greg Kroah-Hartman

6.12-stable review patch. If anyone has any objections, please let me know. ------------------ From: Thomas Zimmermann <tzimmermann(a)suse.de> commit f6bfc9afc7510cb5e6fbe0a17c507917b0120280 upstream. Acquire GEM handles in drm_framebuffer_init() and release them in the corresponding drm_framebuffer_cleanup(). Ties the handle's lifetime to the framebuffer. Not all GEM buffer objects have GEM handles. If not set, no refcounting takes place. This is the case for some fbdev emulation. This is not a problem as these GEM objects do not use dma-bufs and drivers will not release them while fbdev emulation is running. Framebuffer flags keep a bit per color plane of which the framebuffer holds a GEM handle reference. As all drivers use drm_framebuffer_init(), they will now all hold dma-buf references as fixed in commit 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers"). In the GEM framebuffer helpers, restore the original ref counting on buffer objects. As the helpers for handle refcounting are now no longer called from outside the DRM core, unexport the symbols. v3: - don't mix internal flags with mode flags (Christian) v2: - track framebuffer handle refs by flag - drop gma500 cleanup (Christian) Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers") Reported-by: Bert Karwatzki <spasswolf(a)web.de> Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/ Tested-by: Bert Karwatzki <spasswolf(a)web.de> Tested-by: Mario Limonciello <superm1(a)kernel.org> Tested-by: Borislav Petkov (AMD) <bp(a)alien8.de> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://lore.kernel.org/r/20250707131224.249496-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_framebuffer.c | 31 ++++++++++++++++++++-- drivers/gpu/drm/drm_gem.c | 38 +++++++++++++++++---------- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++------- drivers/gpu/drm/drm_internal.h | 2 - include/drm/drm_framebuffer.h | 7 ++++ 5 files changed, 68 insertions(+), 26 deletions(-) --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -860,11 +860,23 @@ void drm_framebuffer_free(struct kref *k int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, const struct drm_framebuffer_funcs *funcs) { + unsigned int i; int ret; + bool exists; if (WARN_ON_ONCE(fb->dev != dev || !fb->format)) return -EINVAL; + for (i = 0; i < fb->format->num_planes; i++) { + if (drm_WARN_ON_ONCE(dev, fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i))) + fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + if (fb->obj[i]) { + exists = drm_gem_object_handle_get_if_exists_unlocked(fb->obj[i]); + if (exists) + fb->internal_flags |= DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + } + } + INIT_LIST_HEAD(&fb->filp_head); fb->funcs = funcs; @@ -873,7 +885,7 @@ int drm_framebuffer_init(struct drm_devi ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB, false, drm_framebuffer_free); if (ret) - goto out; + goto err; mutex_lock(&dev->mode_config.fb_lock); dev->mode_config.num_fb++; @@ -881,7 +893,16 @@ int drm_framebuffer_init(struct drm_devi mutex_unlock(&dev->mode_config.fb_lock); drm_mode_object_register(dev, &fb->base); -out: + + return 0; + +err: + for (i = 0; i < fb->format->num_planes; i++) { + if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) { + drm_gem_object_handle_put_unlocked(fb->obj[i]); + fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i); + } + } return ret; } EXPORT_SYMBOL(drm_framebuffer_init); @@ -958,6 +979,12 @@ EXPORT_SYMBOL(drm_framebuffer_unregister void drm_framebuffer_cleanup(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev; + unsigned int i; + + for (i = 0; i < fb->format->num_planes; i++) { + if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) + drm_gem_object_handle_put_unlocked(fb->obj[i]); + } mutex_lock(&dev->mode_config.fb_lock); list_del(&fb->head); --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -197,23 +197,34 @@ static void drm_gem_object_handle_get(st } /** - * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * drm_gem_object_handle_get_if_exists_unlocked - acquire reference on user-space handle, if any * @obj: GEM object * - * Acquires a reference on the GEM buffer object's handle. Required - * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() - * to release the reference. + * Acquires a reference on the GEM buffer object's handle. Required to keep + * the GEM object alive. Call drm_gem_object_handle_put_if_exists_unlocked() + * to release the reference. Does nothing if the buffer object has no handle. + * + * Returns: + * True if a handle exists, or false otherwise */ -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; guard(mutex)(&dev->object_name_lock); - drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */ + /* + * First ref taken during GEM object creation, if any. Some + * drivers set up internal framebuffers with GEM objects that + * do not have a GEM handle. Hence, this counter can be zero. + */ + if (!obj->handle_count) + return false; + drm_gem_object_handle_get(obj); + + return true; } -EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); /** * drm_gem_object_handle_free - release resources bound to userspace handles @@ -246,7 +257,7 @@ static void drm_gem_object_exported_dma_ } /** - * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * drm_gem_object_handle_put_unlocked - releases reference on user-space handle * @obj: GEM object * * Releases a reference on the GEM buffer object's handle. Possibly releases @@ -257,14 +268,14 @@ void drm_gem_object_handle_put_unlocked( struct drm_device *dev = obj->dev; bool final = false; - if (WARN_ON(READ_ONCE(obj->handle_count) == 0)) + if (drm_WARN_ON(dev, READ_ONCE(obj->handle_count) == 0)) return; /* - * Must bump handle count first as this may be the last - * ref, in which case the object would disappear before we - * checked for a name - */ + * Must bump handle count first as this may be the last + * ref, in which case the object would disappear before + * we checked for a name. + */ mutex_lock(&dev->object_name_lock); if (--obj->handle_count == 0) { @@ -277,7 +288,6 @@ void drm_gem_object_handle_put_unlocked( if (final) drm_gem_object_put(obj); } -EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -99,7 +99,7 @@ void drm_gem_fb_destroy(struct drm_frame unsigned int i; for (i = 0; i < fb->format->num_planes; i++) - drm_gem_object_handle_put_unlocked(fb->obj[i]); + drm_gem_object_put(fb->obj[i]); drm_framebuffer_cleanup(fb); kfree(fb); @@ -182,10 +182,8 @@ int drm_gem_fb_init_with_funcs(struct dr if (!objs[i]) { drm_dbg_kms(dev, "Failed to lookup GEM object\n"); ret = -ENOENT; - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; } - drm_gem_object_handle_get_unlocked(objs[i]); - drm_gem_object_put(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -195,22 +193,22 @@ int drm_gem_fb_init_with_funcs(struct dr drm_dbg_kms(dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); - drm_gem_object_handle_put_unlocked(objs[i]); + drm_gem_object_put(objs[i]); ret = -EINVAL; - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; } } ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs); if (ret) - goto err_gem_object_handle_put_unlocked; + goto err_gem_object_put; return 0; -err_gem_object_handle_put_unlocked: +err_gem_object_put: while (i > 0) { --i; - drm_gem_object_handle_put_unlocked(objs[i]); + drm_gem_object_put(objs[i]); } return ret; } --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -153,7 +153,7 @@ void drm_sysfs_lease_event(struct drm_de /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj); void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -23,6 +23,7 @@ #ifndef __DRM_FRAMEBUFFER_H__ #define __DRM_FRAMEBUFFER_H__ +#include <linux/bits.h> #include <linux/ctype.h> #include <linux/list.h> #include <linux/sched.h> @@ -100,6 +101,8 @@ struct drm_framebuffer_funcs { unsigned num_clips); }; +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i) BIT(0u + (_i)) + /** * struct drm_framebuffer - frame buffer object * @@ -189,6 +192,10 @@ struct drm_framebuffer { */ int flags; /** + * @internal_flags: Framebuffer flags like DRM_FRAMEBUFFER_HAS_HANDLE_REF. + */ + unsigned int internal_flags; + /** * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock. */ struct list_head filp_head;

2 months

[PATCH 6.12 074/163] drm/gem: Acquire references on GEM handles for framebuffers

by Greg Kroah-Hartman

6.12-stable review patch. If anyone has any objections, please let me know. ------------------ From: Thomas Zimmermann <tzimmermann(a)suse.de> commit 5307dce878d4126e1b375587318955bd019c3741 upstream. A GEM handle can be released while the GEM buffer object is attached to a DRM framebuffer. This leads to the release of the dma-buf backing the buffer object, if any. [1] Trying to use the framebuffer in further mode-setting operations leads to a segmentation fault. Most easily happens with driver that use shadow planes for vmap-ing the dma-buf during a page flip. An example is shown below. [ 156.791968] ------------[ cut here ]------------ [ 156.796830] WARNING: CPU: 2 PID: 2255 at drivers/dma-buf/dma-buf.c:1527 dma_buf_vmap+0x224/0x430 [...] [ 156.942028] RIP: 0010:dma_buf_vmap+0x224/0x430 [ 157.043420] Call Trace: [ 157.045898] <TASK> [ 157.048030] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.052436] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.056836] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.061253] ? drm_gem_shmem_vmap+0x74/0x710 [ 157.065567] ? dma_buf_vmap+0x224/0x430 [ 157.069446] ? __warn.cold+0x58/0xe4 [ 157.073061] ? dma_buf_vmap+0x224/0x430 [ 157.077111] ? report_bug+0x1dd/0x390 [ 157.080842] ? handle_bug+0x5e/0xa0 [ 157.084389] ? exc_invalid_op+0x14/0x50 [ 157.088291] ? asm_exc_invalid_op+0x16/0x20 [ 157.092548] ? dma_buf_vmap+0x224/0x430 [ 157.096663] ? dma_resv_get_singleton+0x6d/0x230 [ 157.101341] ? __pfx_dma_buf_vmap+0x10/0x10 [ 157.105588] ? __pfx_dma_resv_get_singleton+0x10/0x10 [ 157.110697] drm_gem_shmem_vmap+0x74/0x710 [ 157.114866] drm_gem_vmap+0xa9/0x1b0 [ 157.118763] drm_gem_vmap_unlocked+0x46/0xa0 [ 157.123086] drm_gem_fb_vmap+0xab/0x300 [ 157.126979] drm_atomic_helper_prepare_planes.part.0+0x487/0xb10 [ 157.133032] ? lockdep_init_map_type+0x19d/0x880 [ 157.137701] drm_atomic_helper_commit+0x13d/0x2e0 [ 157.142671] ? drm_atomic_nonblocking_commit+0xa0/0x180 [ 157.147988] drm_mode_atomic_ioctl+0x766/0xe40 [...] [ 157.346424] ---[ end trace 0000000000000000 ]--- Acquiring GEM handles for the framebuffer's GEM buffer objects prevents this from happening. The framebuffer's cleanup later puts the handle references. Commit 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance") triggers the segmentation fault easily by using the dma-buf field more widely. The underlying issue with reference counting has been present before. v2: - acquire the handle instead of the BO (Christian) - fix comment style (Christian) - drop the Fixes tag (Christian) - rename err_ gotos - add missing Link tag Suggested-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Link: https://elixir.bootlin.com/linux/v6.15/source/drivers/gpu/drm/drm_gem.c#L241 # [1] Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://lore.kernel.org/r/20250630084001.293053-1-tzimmermann@suse.de Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 44 ++++++++++++++++++++++++--- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 +++++---- drivers/gpu/drm/drm_internal.h | 2 + 3 files changed, 51 insertions(+), 11 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -186,6 +186,35 @@ void drm_gem_private_object_fini(struct } EXPORT_SYMBOL(drm_gem_private_object_fini); +static void drm_gem_object_handle_get(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock)); + + if (obj->handle_count++ == 0) + drm_gem_object_get(obj); +} + +/** + * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * @obj: GEM object + * + * Acquires a reference on the GEM buffer object's handle. Required + * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() + * to release the reference. + */ +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + guard(mutex)(&dev->object_name_lock); + + drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */ + drm_gem_object_handle_get(obj); +} +EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); + /** * drm_gem_object_handle_free - release resources bound to userspace handles * @obj: GEM object to clean up. @@ -216,8 +245,14 @@ static void drm_gem_object_exported_dma_ } } -static void -drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) +/** + * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * @obj: GEM object + * + * Releases a reference on the GEM buffer object's handle. Possibly releases + * the GEM buffer object and associated dma-buf objects. + */ +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; bool final = false; @@ -242,6 +277,7 @@ drm_gem_object_handle_put_unlocked(struc if (final) drm_gem_object_put(obj); } +EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's @@ -363,8 +399,8 @@ drm_gem_handle_create_tail(struct drm_fi int ret; WARN_ON(!mutex_is_locked(&dev->object_name_lock)); - if (obj->handle_count++ == 0) - drm_gem_object_get(obj); + + drm_gem_object_handle_get(obj); /* * Get the user-visible handle using idr. Preload and perform --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -99,7 +99,7 @@ void drm_gem_fb_destroy(struct drm_frame unsigned int i; for (i = 0; i < fb->format->num_planes; i++) - drm_gem_object_put(fb->obj[i]); + drm_gem_object_handle_put_unlocked(fb->obj[i]); drm_framebuffer_cleanup(fb); kfree(fb); @@ -182,8 +182,10 @@ int drm_gem_fb_init_with_funcs(struct dr if (!objs[i]) { drm_dbg_kms(dev, "Failed to lookup GEM object\n"); ret = -ENOENT; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } + drm_gem_object_handle_get_unlocked(objs[i]); + drm_gem_object_put(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -193,22 +195,22 @@ int drm_gem_fb_init_with_funcs(struct dr drm_dbg_kms(dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); ret = -EINVAL; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } } ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs); if (ret) - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; return 0; -err_gem_object_put: +err_gem_object_handle_put_unlocked: while (i > 0) { --i; - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); } return ret; } --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -153,6 +153,8 @@ void drm_sysfs_lease_event(struct drm_de /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, u32 *handlep);

2 months

Re: [PATCH v6 09/12] tee: add Qualcomm TEE driver

by kernel test robot

Hi Amirreza, kernel test robot noticed the following build errors: [auto build test ERROR on 835244aba90de290b4b0b1fa92b6734f3ee7b3d9] url: https://github.com/intel-lab-lkp/linux/commits/Amirreza-Zarrabi/tee-allow-a… base: 835244aba90de290b4b0b1fa92b6734f3ee7b3d9 patch link: https://lore.kernel.org/r/20250713-qcom-tee-using-tee-ss-without-mem-obj-v6… patch subject: [PATCH v6 09/12] tee: add Qualcomm TEE driver config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250715/202507150221.oWiaX1I9-lkp@…) compiler: sh4-linux-gcc (GCC) 15.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250715/202507150221.oWiaX1I9-lkp@…) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp(a)intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202507150221.oWiaX1I9-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/tee/qcomtee/qcomtee.h:12, from drivers/tee/qcomtee/async.c:8: drivers/tee/qcomtee/qcomtee_msg.h: In function 'qcomtee_msg_num_ib': >> drivers/tee/qcomtee/qcomtee_msg.h:172:16: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration] 172 | return FIELD_GET(QCOMTEE_MASK_IB, counts); | ^~~~~~~~~ -- In file included from drivers/tee/qcomtee/qcomtee.h:12, from drivers/tee/qcomtee/core.c:14: drivers/tee/qcomtee/qcomtee_msg.h: In function 'qcomtee_msg_num_ib': >> drivers/tee/qcomtee/qcomtee_msg.h:172:16: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration] 172 | return FIELD_GET(QCOMTEE_MASK_IB, counts); | ^~~~~~~~~ drivers/tee/qcomtee/core.c: In function 'qcomtee_object_user_init': drivers/tee/qcomtee/core.c:303:17: warning: function 'qcomtee_object_user_init' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format] 303 | object->name = kvasprintf_const(GFP_KERNEL, fmt, ap); | ^~~~~~ drivers/tee/qcomtee/core.c: In function 'qcomtee_prepare_msg': drivers/tee/qcomtee/core.c:417:26: error: implicit declaration of function 'copy_from_user' [-Wimplicit-function-declaration] 417 | else if (copy_from_user(ptr, u[i].b.uaddr, u[i].b.size)) | ^~~~~~~~~~~~~~ drivers/tee/qcomtee/core.c: In function 'qcomtee_update_args': drivers/tee/qcomtee/core.c:496:26: error: implicit declaration of function 'copy_to_user' [-Wimplicit-function-declaration] 496 | else if (copy_to_user(u[i].b.uaddr, ptr, u[i].b.size)) | ^~~~~~~~~~~~ -- In file included from drivers/tee/qcomtee/qcomtee.h:12, from drivers/tee/qcomtee/user_obj.c:10: drivers/tee/qcomtee/qcomtee_msg.h: In function 'qcomtee_msg_num_ib': >> drivers/tee/qcomtee/qcomtee_msg.h:172:16: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration] 172 | return FIELD_GET(QCOMTEE_MASK_IB, counts); | ^~~~~~~~~ drivers/tee/qcomtee/user_obj.c: In function 'qcomtee_cb_params_from_args': drivers/tee/qcomtee/user_obj.c:449:29: error: implicit declaration of function 'copy_to_user' [-Wimplicit-function-declaration] 449 | if (copy_to_user(params[i].u.ubuf.uaddr, u[i].b.addr, | ^~~~~~~~~~~~ drivers/tee/qcomtee/user_obj.c: In function 'qcomtee_cb_params_to_args': drivers/tee/qcomtee/user_obj.c:526:29: error: implicit declaration of function 'copy_from_user' [-Wimplicit-function-declaration] 526 | if (copy_from_user(u[i].b.addr, params[i].u.ubuf.uaddr, | ^~~~~~~~~~~~~~ vim +/FIELD_GET +172 drivers/tee/qcomtee/qcomtee_msg.h 169 170 static inline unsigned int qcomtee_msg_num_ib(u32 counts) 171 { > 172 return FIELD_GET(QCOMTEE_MASK_IB, counts); 173 } 174 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

2 months

Re: [PATCH RFC net-next 0/5] net: ethernet: ti: am65-cpsw: add AF_XDP zero copy support

by Jakub Kicinski

On Mon, 14 Jul 2025 14:50:05 +0530 Malladi, Meghana wrote: > > AF_XDP performance using 64 byte packets in Kpps. > > Benchmark: XDP-SKB XDP-Native XDP-Native(ZeroCopy) > > rxdrop 317 504 824 > > txonly 400 405 757 > > l2fwd 207 264 0 > > > > AF_XDP performance using 1500 byte packets in Kpps. > > Benchmark: XDP-SKB XDP-Native XDP-Native(ZeroCopy) > > rxdrop 82 82 82 > > txonly 82 82 82 > > l2fwd 82 82 82 > > > > [1]: https://github.com/xdp-project/bpf-examples/tree/master/AF_XDP-example > > > > To: > > > > Signed-off-by: Roger Quadros <rogerq(a)kernel.org> > > This series crashes Linux on am64xx-hsevm, when I tried nfs boot using > AM65-CPSW-NUSS driver: > logs: > https://gist.github.com/MeghanaMalladiTI/d655a1c8ca88113ee7f5f57d6ab0ec4c > > Seems like you have reverted the fix for the same bug which was reported > by Siddharth and fixed by Julien: > https://lore.kernel.org/all/7f7fb71a-6d15-46f1-b63c-b569a2e230b7@baylibre.c… > > reverted lines: > if (!common->ports[port].ndev) > /* FIXME should we BUG here? */ > continue; > > Can you please take a look at it. Just to be clear -- you're reporting this problem to Roger so that its fixed before the series is reposted? I don't see this in the tree, I wanted to make sure it's not something I need to track as a regression.

2 months

Re: [PATCH v2 3/3] drm: tiny: Add support for Mayqueen Pixpaper e-ink panel

by Thomas Zimmermann

Hi Am 14.07.25 um 04:59 schrieb LiangCheng Wang: > Introduce a DRM driver for the Mayqueen Pixpaper e-ink display panel, > which is controlled via SPI. The driver supports a 122x250 resolution > display with XRGB8888 format. > > Also, add a MAINTAINERS entry for the Pixpaper driver. > > Signed-off-by: LiangCheng Wang <zaq14760(a)gmail.com> > --- > MAINTAINERS | 6 + > drivers/gpu/drm/tiny/Kconfig | 15 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/pixpaper.c | 777 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 799 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index fad6cb025a1918beec113b576cf28b76151745ef..0613f32ef8a702e508c9a2e51853f8fe6a38ba42 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7756,6 +7756,12 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git > F: Documentation/devicetree/bindings/display/repaper.txt > F: drivers/gpu/drm/tiny/repaper.c > > +DRM DRIVER FOR PIXPAPER E-INK PANEL > +M: LiangCheng Wang <zaq14760(a)gmail.com> > +S: Maintained > +F: Documentation/devicetree/bindings/display/mayqueen,pixpaper.yaml > +F: drivers/gpu/drm/tiny/pixpaper.c > + > DRM DRIVER FOR QEMU'S CIRRUS DEVICE > M: Dave Airlie <airlied(a)redhat.com> > M: Gerd Hoffmann <kraxel(a)redhat.com> > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > index 06e54694a7f2fe1649e1886f039926b24f698e0d..f9b814e87348cad1946dd5e2ff54d304100ef264 100644 > --- a/drivers/gpu/drm/tiny/Kconfig > +++ b/drivers/gpu/drm/tiny/Kconfig > @@ -164,6 +164,21 @@ config TINYDRM_MI0283QT > DRM driver for the Multi-Inno MI0283QT display panel > If M is selected the module will be called mi0283qt. > > +config TINYDRM_PIXPAPER We don't use the TINYDRM_ prefix any longer. Just call it DRM_PIXPAPER. > + tristate "DRM support for PIXPAPER display panels" > + depends on DRM && SPI > + select DRM_CLIENT_SELECTION > + select DRM_KMS_HELPER > + select DRM_GEM_DMA_HELPER Alphabetical sorting please. > + help > + DRM driver for the Mayqueen Pixpaper e-ink display panel. > + > + This driver supports small e-paper displays connected over SPI, > + with a resolution of 122x250 and XRGB8888 framebuffer format. > + It is intended for low-power embedded applications. > + > + If M is selected, the module will be built as pixpaper.ko. > + > config TINYDRM_REPAPER > tristate "DRM support for Pervasive Displays RePaper panels (V231)" > depends on DRM && SPI > diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile > index 4a9ff61ec25420e2c0a648c04eaab7ca25dd5407..40d613b57f5ee990dbf170d69939a39546be21b7 100644 > --- a/drivers/gpu/drm/tiny/Makefile > +++ b/drivers/gpu/drm/tiny/Makefile > @@ -12,5 +12,6 @@ obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o > obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o > obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o > obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o > +obj-$(CONFIG_TINYDRM_PIXPAPER) += pixpaper.o > obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o > obj-$(CONFIG_TINYDRM_SHARP_MEMORY) += sharp-memory.o > diff --git a/drivers/gpu/drm/tiny/pixpaper.c b/drivers/gpu/drm/tiny/pixpaper.c > new file mode 100644 > index 0000000000000000000000000000000000000000..f134cacbb3d2b2792b94fc37cddb0b264348f192 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/pixpaper.c > @@ -0,0 +1,777 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DRM driver for PIXPAPER e-ink panel > + * > + * Author: LiangCheng Wang <zaq14760(a)gmail.com>, > + */ > +#include <drm/clients/drm_client_setup.h> > + Blank line before this include, not after. > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_fbdev_dma.h> > +#include <drm/drm_framebuffer.h> > +#include <drm/drm_gem_atomic_helper.h> > +#include <drm/drm_gem_dma_helper.h> > +#include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_probe_helper.h> > + > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> The <linux/> includes go before the <drm/> ones. > + > +MODULE_IMPORT_NS("DMA_BUF"); > + > +#define PIXPAPER_WIDTH 122 > +#define PIXPAPER_HEIGHT 250 > + > +#define PANEL_BUFFER_WIDTH 128 > +#define PANEL_BUFFER_TWO_BYTES_PER_ROW (PANEL_BUFFER_WIDTH / 4) > + > +#define PIXPAPER_SPI_SPEED_DEFAULT 1000000 > + > +#define PIXPAPER_CMD_PANEL_SETTING 0x00 > +#define PIXPAPER_CMD_POWER_SETTING 0x01 > +#define PIXPAPER_CMD_POWER_OFF 0x02 > +#define PIXPAPER_CMD_POWER_OFF_SEQUENCE 0x03 > +#define PIXPAPER_CMD_POWER_ON 0x04 > +#define PIXPAPER_CMD_BOOSTER_SOFT_START 0x06 > +#define PIXPAPER_CMD_DEEP_SLEEP 0x07 > +#define PIXPAPER_CMD_DATA_START_TRANSMISSION 0x10 > +#define PIXPAPER_CMD_DISPLAY_REFRESH 0x12 > +#define PIXPAPER_CMD_PLL_CONTROL 0x30 > +#define PIXPAPER_CMD_TEMP_SENSOR_CALIB 0x41 > +#define PIXPAPER_CMD_UNKNOWN_4D 0x4D > +#define PIXPAPER_CMD_VCOM_INTERVAL 0x50 > +#define PIXPAPER_CMD_TCON_SETTING 0x60 > +#define PIXPAPER_CMD_RESOLUTION_SETTING 0x61 > +#define PIXPAPER_CMD_GATE_SOURCE_START 0x65 > +#define PIXPAPER_CMD_UNKNOWN_B4 0xB4 > +#define PIXPAPER_CMD_UNKNOWN_B5 0xB5 > +#define PIXPAPER_CMD_CASCADE_SETTING 0xE0 > +#define PIXPAPER_CMD_POWER_SAVING 0xE3 > +#define PIXPAPER_CMD_AUTO_MEASURE_VCOM 0xE7 > +#define PIXPAPER_CMD_UNKNOWN_E9 0xE9 > + > +static int pixpaper_crtc_helper_atomic_check(struct drm_crtc *crtc, > + struct drm_atomic_state *state); > +static int pixpaper_plane_helper_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state); > +static void pixpaper_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_state *state); > +static void pixpaper_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_state *state); > +static void pixpaper_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_state *state); > +static int pixpaper_connector_get_modes(struct drm_connector *connector); Please don't declare those functions. Interleave the implementation with the structs that use them. > + > +struct pixpaper_panel { > + struct drm_device drm; > + struct drm_plane plane; > + struct drm_crtc crtc; > + struct drm_encoder encoder; > + struct drm_connector connector; > + > + struct spi_device *spi; > + struct gpio_desc *reset; > + struct gpio_desc *busy; > + struct gpio_desc *dc; > +}; > + > +static const struct drm_plane_funcs pixpaper_plane_funcs = { > + .update_plane = drm_atomic_helper_update_plane, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = drm_plane_cleanup, > + DRM_GEM_SHADOW_PLANE_FUNCS, > +}; > + > +static const struct drm_plane_helper_funcs pixpaper_plane_helper_funcs = { > + DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, > + .atomic_check = pixpaper_plane_helper_atomic_check, > + .atomic_update = pixpaper_plane_atomic_update, For example, these functions should be implemented right before declaring the global variable; same goes for the others. > +}; > + > +static const struct drm_crtc_funcs pixpaper_crtc_funcs = { > + .set_config = drm_atomic_helper_set_config, > + .page_flip = drm_atomic_helper_page_flip, > + .reset = drm_atomic_helper_crtc_reset, > + .destroy = drm_crtc_cleanup, > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > +}; > + > +static const struct drm_crtc_helper_funcs pixpaper_crtc_helper_funcs = { > + .atomic_check = pixpaper_crtc_helper_atomic_check, > + .atomic_enable = pixpaper_crtc_atomic_enable, > + .atomic_disable = pixpaper_crtc_atomic_disable, > +}; > + > +static const struct drm_encoder_funcs pixpaper_encoder_funcs = { > + .destroy = drm_encoder_cleanup, > +}; > + > +static const struct drm_connector_funcs pixpaper_connector_funcs = { > + .reset = drm_atomic_helper_connector_reset, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = drm_connector_cleanup, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static const struct drm_connector_helper_funcs pixpaper_connector_helper_funcs = { > + .get_modes = pixpaper_connector_get_modes, > +}; > + > +static int pixpaper_plane_helper_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); > + struct drm_crtc *new_crtc = new_plane_state->crtc; > + struct drm_crtc_state *new_crtc_state = NULL; > + int ret; > + > + if (new_crtc) > + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc); > + > + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, > + DRM_PLANE_NO_SCALING, > + DRM_PLANE_NO_SCALING, > + false, false); > + if (ret) > + return ret; > + else if (!new_plane_state->visible) > + return 0; > + > + return 0; > +} > + > +static int pixpaper_crtc_helper_atomic_check(struct drm_crtc *crtc, > + struct drm_atomic_state *state) > +{ > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + > + if (!crtc_state->enable) > + return 0; > + > + return drm_atomic_helper_check_crtc_primary_plane(crtc_state); > +} > + > +static void pixpaper_wait_busy(struct pixpaper_panel *panel) > +{ > + unsigned int delay_us = 1000; > + > + usleep_range(delay_us, delay_us + 500); > + while (true) > + if (gpiod_get_value_cansleep(panel->busy) == 1) > + break; > +} This function should eventually time out. > + > +static int pixpaper_spi_sync(struct spi_device *spi, struct spi_message *msg) > +{ > + int ret; > + > + ret = spi_sync(spi, msg); > + if (ret < 0) > + dev_err(&spi->dev, "SPI sync failed: %d\n", ret); Please use DRM's error reporting. dev_err() becomes drm_err(), dev_dbg() becomes drm_dbg(). Further below is WARN_ON(), which should become drm_WARN_ON(). Here and everwhere else in the driver. The only exception is in places where you don't have a DRM device yet, sch as early device probing. > + > + return ret; > +} > + > +static int pixpaper_send_cmd(struct pixpaper_panel *panel, u8 cmd) > +{ > + struct spi_transfer xfer = { > + .tx_buf = &cmd, > + .len = 1, > + }; > + struct spi_message msg; > + int ret; > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfer, &msg); > + > + gpiod_set_value_cansleep(panel->dc, 0); > + usleep_range(1, 5); > + ret = pixpaper_spi_sync(panel->spi, &msg); > + > + return ret; > +} > + > +static int pixpaper_send_data(struct pixpaper_panel *panel, u8 data) > +{ > + struct spi_transfer xfer = { > + .tx_buf = &data, > + .len = 1, > + }; > + struct spi_message msg; > + int ret; > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfer, &msg); > + > + gpiod_set_value_cansleep(panel->dc, 1); > + usleep_range(1, 5); > + ret = pixpaper_spi_sync(panel->spi, &msg); > + > + return ret; > +} > + > +static int pixpaper_panel_hw_init(struct pixpaper_panel *panel) > +{ > + struct device *dev = &panel->spi->dev; > + int ret = 0; > + > + dev_info(dev, "%s: Starting hardware initialization\n", __func__); > + > + gpiod_set_value_cansleep(panel->reset, 0); > + msleep(50); > + gpiod_set_value_cansleep(panel->reset, 1); > + msleep(50); > + > + pixpaper_wait_busy(panel); > + dev_info(dev, "Hardware reset complete, panel idle.\n"); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_UNKNOWN_4D); > + ret |= pixpaper_send_data(panel, 0x78); This error handling isn't working. Your code OR's multiple errno codes and the result will have a different meaning. The easiest way to avoid excessive error checking is to add an error context as final parameter, like this struct pixpaper_error_ctx { int errno_code; }; pixpaper_spi_sync(..., pixpaper_error_ctx *err) { if (err->errno_code) return; ret = spi_sync(...) if (ret) err->errno_code = ret; } send_cmd(..., pix_error_ctx *err) { pixppaper_spi_sync(..., err); } send_data(..., pix_error_ctx *err) { pixppaper_spi_sync(..., err); } int my_pixpaper_driver_func() { sturct pixpaper_error_ctx = {} send_cmd(..., &err); send_data(..., &err); send_data(..., &err); send_data(..., &err); send_cmd(..., &err); send_data(..., &err); send_data(..., &err); send_data(..., &err); if (err->errno_code) return errno_code return 0; } This pushes error handling down into the call tree and you only have to test for the final result. > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); And you can extend other functions, such as pixpaper_wait_busy(), with the error context. Integrates them with the rest of the I/O code. > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_PANEL_SETTING); > + ret |= pixpaper_send_data(panel, 0x0F); > + ret |= pixpaper_send_data(panel, 0x09); There are *a lot* of magic values in this function. Please document the meaning of these. > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_SETTING); > + ret |= pixpaper_send_data(panel, 0x07); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0x22); > + ret |= pixpaper_send_data(panel, 0x78); > + ret |= pixpaper_send_data(panel, 0x0A); > + ret |= pixpaper_send_data(panel, 0x22); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_OFF_SEQUENCE); > + ret |= pixpaper_send_data(panel, 0x10); > + ret |= pixpaper_send_data(panel, 0x54); > + ret |= pixpaper_send_data(panel, 0x44); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_BOOSTER_SOFT_START); > + ret |= pixpaper_send_data(panel, 0x0F); > + ret |= pixpaper_send_data(panel, 0x0A); > + ret |= pixpaper_send_data(panel, 0x2F); > + ret |= pixpaper_send_data(panel, 0x25); > + ret |= pixpaper_send_data(panel, 0x22); > + ret |= pixpaper_send_data(panel, 0x2E); > + ret |= pixpaper_send_data(panel, 0x21); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_PLL_CONTROL); > + ret |= pixpaper_send_data(panel, 0x02); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_TEMP_SENSOR_CALIB); > + ret |= pixpaper_send_data(panel, 0x00); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_VCOM_INTERVAL); > + ret |= pixpaper_send_data(panel, 0x37); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_TCON_SETTING); > + ret |= pixpaper_send_data(panel, 0x02); > + ret |= pixpaper_send_data(panel, 0x02); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_RESOLUTION_SETTING); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0x80); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0xFA); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_GATE_SOURCE_START); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0x00); > + ret |= pixpaper_send_data(panel, 0x00); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_AUTO_MEASURE_VCOM); > + ret |= pixpaper_send_data(panel, 0x1C); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_SAVING); > + ret |= pixpaper_send_data(panel, 0x22); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_CASCADE_SETTING); > + ret |= pixpaper_send_data(panel, 0x00); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_UNKNOWN_B4); > + ret |= pixpaper_send_data(panel, 0xD0); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_UNKNOWN_B5); > + ret |= pixpaper_send_data(panel, 0x03); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + ret |= pixpaper_send_cmd(panel, PIXPAPER_CMD_UNKNOWN_E9); > + ret |= pixpaper_send_data(panel, 0x01); > + if (ret) > + goto init_fail; > + pixpaper_wait_busy(panel); > + > + dev_info(dev, "%s: Hardware initialization successful\n", __func__); Never print this type of status messages. If nothing exceptional happens, you driver should be quite. At most, use drm_dbg() here. > + return 0; > + > +init_fail: > + dev_err(dev, "%s: Hardware initialization failed (err=%d)\n", __func__, > + ret); Error messages are OK, but don't include the function name. > + return ret; > +} > + > +static void pixpaper_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_state *state) > +{ > + struct pixpaper_panel *panel = > + container_of(crtc, struct pixpaper_panel, crtc); Instead of semi-manual upcast, please write a helper that upcasts from struct drm_device struct pixpaper_panel *to_pixpaper_panel(struct drm_device *drm) { return container_of(drm, struct pixpaper_panel, drm); } and call it with the CRTC's device to_pixpaper_panel(crtc->dev); > + struct drm_device *drm = &panel->drm; > + int ret; > + > + dev_info(drm->dev, "%s: Enabling pipe\n", __func__); Same here. The driver should just be quite. > + > + ret = pixpaper_panel_hw_init(panel); Do you really need a full hardware init for an atomic_enable? You should rather do this during device probing. > + if (ret) { > + dev_err(drm->dev, "Panel HW Init failed during enable: %d\n", > + ret); > + return; > + } > + > + dev_info(drm->dev, "Sending Power ON (PON)\n"); > + ret = pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_ON); > + if (ret) { > + dev_err(drm->dev, "Failed to send PON command: %d\n", ret); > + return; > + } > + > + usleep_range(10000, 11000); > + > + pixpaper_wait_busy(panel); > + > + dev_info(drm->dev, "Panel enabled and powered on\n"); > +} > + > +static void pixpaper_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_state *state) > +{ > + struct pixpaper_panel *panel = > + container_of(crtc, struct pixpaper_panel, crtc); > + struct drm_device *drm = &panel->drm; > + int ret; > + > + dev_dbg(drm->dev, "%s: Disabling pipe\n", __func__); > + > + ret = pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_OFF); > + if (!ret) { > + usleep_range(10000, 11000); > + pixpaper_wait_busy(panel); > + } else { > + dev_warn(drm->dev, "Failed to send POF command: %d\n", ret); > + } > + dev_info(drm->dev, "Panel disabled\n"); > +} > + > +static u8 pack_pixels_to_byte(u32 *src_pixels, int i, int j, > + struct drm_framebuffer *fb) > +{ > + u8 packed_byte = 0; > + int k; > + > + for (k = 0; k < 4; k++) { > + int current_pixel_x = j * 4 + k; > + u8 two_bit_val; > + > + if (current_pixel_x < PIXPAPER_WIDTH) { > + u32 pixel_offset = > + (i * (fb->pitches[0] / 4)) + current_pixel_x; > + u32 pixel = src_pixels[pixel_offset]; > + u32 r = (pixel >> 16) & 0xFF; > + u32 g = (pixel >> 8) & 0xFF; > + u32 b = pixel & 0xFF; > + u32 gray_val = > + (r * 299 + g * 587 + b * 114 + 500) / 1000; > + > + if (gray_val < 64) > + two_bit_val = 0b00; > + else if (gray_val < 128) > + two_bit_val = 0b01; > + else if (gray_val < 192) > + two_bit_val = 0b10; > + else > + two_bit_val = 0b11; > + } else { > + two_bit_val = 0b11; > + } > + > + packed_byte |= two_bit_val << ((3 - k) * 2); > + } > + > + return packed_byte; > +} > + > +static void pixpaper_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_plane_state *plane_state = > + drm_atomic_get_new_plane_state(state, plane); > + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); > + struct drm_crtc *crtc = plane_state->crtc; > + struct pixpaper_panel *panel = > + container_of(crtc, struct pixpaper_panel, crtc); > + > + struct drm_device *drm = &panel->drm; > + struct drm_framebuffer *fb = plane_state->fb; > + struct iosys_map map = shadow_plane_state->data[0]; > + void *vaddr = map.vaddr; > + int i, j, ret = 0; > + u32 *src_pixels = NULL; > + > + dev_info(drm->dev, "Starting frame update (phys=%dx%d, buf_w=%d)\n", > + PIXPAPER_WIDTH, PIXPAPER_HEIGHT, PANEL_BUFFER_WIDTH); Remove all these info statements please. At most, they should be drm_dbg(). > + > + if (!fb || !plane_state->visible) { > + dev_err(drm->dev, > + "No framebuffer or plane not visible, skipping update\n"); > + return; > + } > + > + if (ret) { > + dev_err(drm->dev, "Failed to vmap dma_buf\n"); > + return; > + } > + > + src_pixels = (u32 *)vaddr; > + > + dev_info(drm->dev, "Sending DTM command\n"); > + ret = pixpaper_send_cmd(panel, PIXPAPER_CMD_DATA_START_TRANSMISSION); > + if (ret) > + goto update_cleanup; > + > + usleep_range(10000, 11000); > + pixpaper_wait_busy(panel); That driver does quite a bit of waiting. Is that really necessary or in case? > + > + dev_info(drm->dev, > + "Panel idle after DTM command, starting data batch send.\n"); > + > + for (i = 0; i < PIXPAPER_HEIGHT; i++) { > + for (j = 0; j < PANEL_BUFFER_TWO_BYTES_PER_ROW; j++) { > + u8 packed_byte = > + pack_pixels_to_byte(src_pixels, i, j, fb); > + > + pixpaper_wait_busy(panel); > + pixpaper_send_data(panel, packed_byte); > + } > + } You're updating the full display AFAICT. That's not optimal, but OK if the hardware can't do better. > + pixpaper_wait_busy(panel); > + > + dev_info(drm->dev, "Sending PON + 0x00 before DRF\n"); > + ret = pixpaper_send_cmd(panel, PIXPAPER_CMD_POWER_ON); > + if (ret) > + goto update_cleanup; > + ret = pixpaper_send_data(panel, 0x00); > + if (ret) { > + dev_err(drm->dev, > + "Failed sending data after PON-before-DRF: %d\n", ret); > + goto update_cleanup; > + } > + usleep_range(10000, 11000); > + pixpaper_wait_busy(panel); > + > + dev_info(drm->dev, "Triggering display refresh (DRF)\n"); > + ret = pixpaper_send_cmd(panel, PIXPAPER_CMD_DISPLAY_REFRESH); > + if (ret) > + goto update_cleanup; > + ret = pixpaper_send_data(panel, 0x00); > + if (ret) { > + dev_err(drm->dev, "Failed sending data after DRF: %d\n", ret); > + goto update_cleanup; > + } > + usleep_range(10000, 11000); > + pixpaper_wait_busy(panel); > + > + dev_info(drm->dev, "Frame update completed and refresh triggered\n"); > + > +update_cleanup: > + if (ret && ret != -ETIMEDOUT) > + dev_err(drm->dev, > + "Frame update function failed with error %d\n", ret); > +} > + > +static int pixpaper_connector_get_modes(struct drm_connector *connector) > +{ > + struct drm_display_mode *mode; > + > + dev_info(connector->dev->dev, "%s: CALLED for connector %s (id: %d)\n", > + __func__, connector->name, connector->base.id); > + > + mode = drm_mode_create(connector->dev); > + if (!mode) { > + DRM_ERROR("Failed to create mode for connector %s\n", > + connector->name); > + return 0; > + } > + > + mode->hdisplay = PIXPAPER_WIDTH; > + mode->vdisplay = PIXPAPER_HEIGHT; > + > + mode->htotal = mode->hdisplay + 80; > + mode->hsync_start = mode->hdisplay + 8; > + mode->hsync_end = mode->hdisplay + 8 + 32; > + mode->vtotal = mode->vdisplay + 10; > + mode->vsync_start = mode->vdisplay + 2; > + mode->vsync_end = mode->vdisplay + 2 + 2; > + > + mode->clock = 6000; > + > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > + drm_mode_set_name(mode); > + > + if (drm_mode_validate_size(mode, connector->dev->mode_config.max_width, > + connector->dev->mode_config.max_height) != > + MODE_OK) { > + DRM_WARN("%s: Mode %s (%dx%d) failed size validation against max %dx%d\n", > + __func__, mode->name, mode->hdisplay, mode->vdisplay, > + connector->dev->mode_config.max_width, > + connector->dev->mode_config.max_height); > + drm_mode_destroy(connector->dev, mode); > + return 0; > + } > + > + drm_mode_probed_add(connector, mode); > + dev_info(connector->dev->dev, > + "%s: Added mode '%s' (%dx%d@%d) to connector %s\n", __func__, > + mode->name, mode->hdisplay, mode->vdisplay, > + drm_mode_vrefresh(mode), connector->name); > + > + connector->display_info.width_mm = 30; > + connector->display_info.height_mm = 47; > + > + return 1; > +} > + > +DEFINE_DRM_GEM_DMA_FOPS(pixpaper_fops); > + > +static struct drm_driver pixpaper_drm_driver = { > + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, > + .fops = &pixpaper_fops, > + .name = "pixpaper", > + .desc = "DRM driver for PIXPAPER e-ink", > + .major = 1, > + .minor = 0, > + DRM_GEM_DMA_DRIVER_OPS_VMAP, > + DRM_FBDEV_DMA_DRIVER_OPS, Is there a specific reason why you need GEM DMA? Since you're hardware doesn't use DMA, GEM SHMEM might be a better choice. > +}; > + > +static int pixpaper_mode_valid(struct drm_device *dev, > + const struct drm_display_mode *mode) > +{ > + if (mode->hdisplay == PIXPAPER_WIDTH && > + mode->vdisplay == PIXPAPER_HEIGHT) { > + return MODE_OK; > + } > + dev_dbg(dev->dev, "Rejecting mode %dx%d (supports only %dx%d)\n", > + mode->hdisplay, mode->vdisplay, PIXPAPER_WIDTH, > + PIXPAPER_HEIGHT); > + return MODE_BAD; > +} > + > +static const struct drm_mode_config_funcs pixpaper_mode_config_funcs = { > + .fb_create = drm_gem_fb_create_with_dirty, > + .mode_valid = pixpaper_mode_valid, > + .atomic_check = drm_atomic_helper_check, > + .atomic_commit = drm_atomic_helper_commit, > +}; > + > +static int pixpaper_probe(struct spi_device *spi) > +{ > + struct device *dev = &spi->dev; > + struct pixpaper_panel *panel; > + struct drm_device *drm; > + int ret; > + > + dev_info(dev, "Probing PIXPAPER panel driver\n"); > + > + panel = devm_drm_dev_alloc(dev, &pixpaper_drm_driver, > + struct pixpaper_panel, drm); > + if (IS_ERR(panel)) { > + ret = PTR_ERR(panel); > + dev_err(dev, "Failed to allocate DRM device: %d\n", ret); > + return ret; > + } > + drm = &panel->drm; > + panel->spi = spi; > + spi_set_drvdata(spi, panel); > + > + spi->mode = SPI_MODE_0; > + spi->bits_per_word = 8; > + > + if (!spi->max_speed_hz) { > + dev_warn(dev, "spi-max-frequency not specified in DT, using default %u Hz\n", > + PIXPAPER_SPI_SPEED_DEFAULT); > + spi->max_speed_hz = PIXPAPER_SPI_SPEED_DEFAULT; > + } else { > + dev_info(dev, "Using spi-max-frequency from DT: %u Hz\n", > + spi->max_speed_hz); > + } > + > + ret = spi_setup(spi); > + if (ret < 0) { > + dev_err(dev, "SPI setup failed: %d\n", ret); > + return ret; > + } > + dev_info(dev, "SPI setup OK (mode=%d, speed=%u Hz, bpw=%d)\n", > + spi->mode, spi->max_speed_hz, spi->bits_per_word); > + > + if (!dev->dma_mask) > + dev->dma_mask = &dev->coherent_dma_mask; > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > + if (ret) { > + dev_err(dev, "Failed to set DMA mask: %d\n", ret); > + return ret; > + } > + dev_dbg(dev, "DMA mask set\n"); > + > + panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(panel->reset)) > + return dev_err_probe(dev, PTR_ERR(panel->reset), > + "Failed to get 'reset' GPIO\n"); > + panel->busy = devm_gpiod_get(dev, "busy", GPIOD_IN); > + if (IS_ERR(panel->busy)) > + return dev_err_probe(dev, PTR_ERR(panel->busy), > + "Failed to get 'busy' GPIO\n"); > + panel->dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_HIGH); > + if (IS_ERR(panel->dc)) > + return dev_err_probe(dev, PTR_ERR(panel->dc), > + "Failed to get 'dc' GPIO\n"); > + dev_info(dev, "All required GPIOs obtained successfully.\n"); > + > + ret = drmm_mode_config_init(drm); > + if (ret) > + return ret; > + drm->mode_config.funcs = &pixpaper_mode_config_funcs; > + drm->mode_config.min_width = PIXPAPER_WIDTH; > + drm->mode_config.max_width = PIXPAPER_WIDTH; > + drm->mode_config.min_height = PIXPAPER_HEIGHT; > + drm->mode_config.max_height = PIXPAPER_HEIGHT; > + > + ret = drm_universal_plane_init(drm, &panel->plane, 1, &pixpaper_plane_funcs, > + (const uint32_t[]){ DRM_FORMAT_XRGB8888 }, 1, NULL, > + DRM_PLANE_TYPE_PRIMARY, NULL); > + drm_plane_enable_fb_damage_clips(&panel->plane); Since the driver always updates the full display anyway, you don't need to enable damage clipping. > + if (ret) > + return ret; > + drm_plane_helper_add(&panel->plane, &pixpaper_plane_helper_funcs); > + > + ret = drm_crtc_init_with_planes(drm, &panel->crtc, &panel->plane, NULL, > + &pixpaper_crtc_funcs, NULL); > + if (ret) > + return ret; > + drm_crtc_helper_add(&panel->crtc, &pixpaper_crtc_helper_funcs); > + > + ret = drm_encoder_init(drm, &panel->encoder, &pixpaper_encoder_funcs, > + DRM_MODE_ENCODER_DAC, NULL); ENCODER_DAC is for VGA connectors. Rather use ENCODER_NONE here. > + if (ret) > + return ret; > + panel->encoder.possible_crtcs = drm_crtc_mask(&panel->crtc); > + > + ret = drm_connector_init(drm, &panel->connector, > + &pixpaper_connector_funcs, > + DRM_MODE_CONNECTOR_SPI); > + if (ret) > + return ret; > + > + drm_connector_helper_add(&panel->connector, > + &pixpaper_connector_helper_funcs); > + drm_connector_attach_encoder(&panel->connector, &panel->encoder); > + > + drm_mode_config_reset(drm); > + > + ret = drm_dev_register(drm, 0); > + if (ret) > + return ret; > + > + drm_client_setup(drm, NULL); > + > + dev_info(dev, "Initialized PIXPAPER panel driver successfully\n"); > + return 0; > +} > + > +static void pixpaper_remove(struct spi_device *spi) > +{ > + struct pixpaper_panel *panel = spi_get_drvdata(spi); > + > + if (!panel) > + return; > + > + dev_info(&spi->dev, "Removing PIXPAPER panel driver\n"); > + > + drm_dev_unplug(&panel->drm); This signals to DRM that the hardware device has been removed. You should put drm_dev_enter() and drm_dev_exit() to the top and bottom of each of the DRM callbacks. The driver will then not try to access the device. Best regards Thomas > + drm_atomic_helper_shutdown(&panel->drm); > +} > + > +static const struct spi_device_id pixpaper_ids[] = { { "pixpaper", 0 }, {} }; > +MODULE_DEVICE_TABLE(spi, pixpaper_ids); > + > +static const struct of_device_id pixpaper_dt_ids[] = { > + { .compatible = "mayqueen,pixpaper" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, pixpaper_dt_ids); > + > +static struct spi_driver pixpaper_spi_driver = { > + .driver = { > + .name = "pixpaper", > + .of_match_table = pixpaper_dt_ids, > + }, > + .id_table = pixpaper_ids, > + .probe = pixpaper_probe, > + .remove = pixpaper_remove, > +}; > + > +module_spi_driver(pixpaper_spi_driver); > + > +MODULE_AUTHOR("LiangCheng Wang"); > +MODULE_DESCRIPTION("DRM SPI driver for PIXPAPER e-ink panel"); > +MODULE_LICENSE("GPL"); > -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)

2 months

Re: [PATCH v6 08/12] firmware: qcom: tzmem: export shm_bridge create/delete

by kernel test robot

Hi Amirreza, kernel test robot noticed the following build warnings: [auto build test WARNING on 835244aba90de290b4b0b1fa92b6734f3ee7b3d9] url: https://github.com/intel-lab-lkp/linux/commits/Amirreza-Zarrabi/tee-allow-a… base: 835244aba90de290b4b0b1fa92b6734f3ee7b3d9 patch link: https://lore.kernel.org/r/20250713-qcom-tee-using-tee-ss-without-mem-obj-v6… patch subject: [PATCH v6 08/12] firmware: qcom: tzmem: export shm_bridge create/delete config: arc-randconfig-001-20250714 (https://download.01.org/0day-ci/archive/20250714/202507141458.kBLqFFYk-lkp@…) compiler: arc-linux-gcc (GCC) 8.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250714/202507141458.kBLqFFYk-lkp@…) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp(a)intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202507141458.kBLqFFYk-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from include/linux/device.h:15, from include/linux/dma-mapping.h:5, from drivers/firmware/qcom/qcom_tzmem.c:10: drivers/firmware/qcom/qcom_tzmem.c: In function 'qcom_tzmem_shm_bridge_create': >> drivers/firmware/qcom/qcom_tzmem.c:139:27: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'phys_addr_t' {aka 'unsigned int'} [-Wformat=] dev_err(qcom_tzmem_dev, "SHM Bridge failed: ret %d paddr 0x%llx, size%zu\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:110:16: note: in definition of macro 'dev_printk_index_wrap' _p_func(dev, fmt, ##__VA_ARGS__); \ ^~~ include/linux/dev_printk.h:154:49: note: in expansion of macro 'dev_fmt' dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__) ^~~~~~~ drivers/firmware/qcom/qcom_tzmem.c:139:3: note: in expansion of macro 'dev_err' dev_err(qcom_tzmem_dev, "SHM Bridge failed: ret %d paddr 0x%llx, size%zu\n", ^~~~~~~ vim +139 drivers/firmware/qcom/qcom_tzmem.c 110 111 /** 112 * qcom_tzmem_shm_bridge_create() - Create a SHM bridge. 113 * @paddr: Physical address of the memory to share. 114 * @size: Size of the memory to share. 115 * @handle: Handle to the SHM bridge. 116 * 117 * On platforms that support SHM bridge, this function creates a SHM bridge 118 * for the given memory region with QTEE. The handle returned by this function 119 * must be passed to qcom_tzmem_shm_bridge_delete() to free the SHM bridge. 120 * 121 * Return: On success, returns 0; on failure, returns < 0. 122 */ 123 int qcom_tzmem_shm_bridge_create(phys_addr_t paddr, size_t size, u64 *handle) 124 { 125 u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags; 126 int ret; 127 128 if (!qcom_tzmem_using_shm_bridge) 129 return 0; 130 131 pfn_and_ns_perm = paddr | QCOM_SCM_PERM_RW; 132 ipfn_and_s_perm = paddr | QCOM_SCM_PERM_RW; 133 size_and_flags = size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); 134 135 ret = qcom_scm_shm_bridge_create(pfn_and_ns_perm, ipfn_and_s_perm, 136 size_and_flags, QCOM_SCM_VMID_HLOS, 137 handle); 138 if (ret) { > 139 dev_err(qcom_tzmem_dev, "SHM Bridge failed: ret %d paddr 0x%llx, size%zu\n", 140 ret, paddr, size); 141 142 return ret; 143 } 144 145 return 0; 146 } 147 EXPORT_SYMBOL_GPL(qcom_tzmem_shm_bridge_create); 148 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

2 months

Re: [PATCH v2 2/3] dt-bindings: display: Add Mayqueen Pixpaper e-ink panel

by Krzysztof Kozlowski

On 14/07/2025 04:59, LiangCheng Wang wrote: > The binding is for the Mayqueen Pixpaper e-ink display panel, > controlled via an SPI interface. > > Signed-off-by: LiangCheng Wang <zaq14760(a)gmail.com> <form letter> This is a friendly reminder during the review process. It looks like you received a tag and forgot to add it. If you do not know the process, here is a short explanation: Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions of patchset, under or above your Signed-off-by tag, unless patch changed significantly (e.g. new properties added to the DT bindings). Tag is "received", when provided in a message replied to you on the mailing list. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for tags received on the version they apply. Please read: https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/sub… If a tag was not added on purpose, please state why and what changed. </form letter> Best regards, Krzysztof

2 months

Re: [PATCH v2 1/3] dt-bindings: vendor-prefixes: Add Mayqueen name

by Krzysztof Kozlowski

On 14/07/2025 04:59, LiangCheng Wang wrote: > From: Wig Cheng <onlywig(a)gmail.com> > > Mayqueen is a Taiwan-based company primarily focused on the development > of arm64 development boards and e-paper displays. > > Signed-off-by: Wig Cheng <onlywig(a)gmail.com> > --- <form letter> This is a friendly reminder during the review process. It looks like you received a tag and forgot to add it. If you do not know the process, here is a short explanation: Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions of patchset, under or above your Signed-off-by tag, unless patch changed significantly (e.g. new properties added to the DT bindings). Tag is "received", when provided in a message replied to you on the mailing list. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for tags received on the version they apply. Please read: https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/sub… If a tag was not added on purpose, please state why and what changed. </form letter> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> Best regards, Krzysztof

2 months

← Newer
1
...
5
6
7
8
9
10
11
...
303
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig