Re: [Freedreno] [PATCH 10/14] drm/tidss: Convert to Linux IRQ interfaces
On 27/07/2021 21:27, Thomas Zimmermann wrote: Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers don't benefit from using it. DRM IRQ callbacks are now being called directly or inlined. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/tidss/tidss_drv.c | 15 +-- drivers/gpu/drm/tidss/tidss_drv.h | 2 ++ drivers/gpu/drm/tidss/tidss_irq.c | 27 --- drivers/gpu/drm/tidss/tidss_irq.h | 4 +--- 4 files changed, 32 insertions(+), 16 deletions(-) Reviewed-by: Tomi Valkeinen Works fine on AM6 EVM. Some cleanups can be done in tidss_irq_install(), but those can be done later. Tomi ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 10/20] drm/omapdrm: Introduce GEM object functions
Hi,
On 13/08/2020 11:36, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in omapdrm.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/omapdrm/omap_drv.c | 9 -
> drivers/gpu/drm/omapdrm/omap_gem.c | 16 +++-
> drivers/gpu/drm/omapdrm/omap_gem.h | 1 -
> 3 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 53d5e184ee77..2e598b8b72af 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -521,12 +521,6 @@ static int dev_open(struct drm_device *dev, struct
> drm_file *file)
> return 0;
> }
>
> -static const struct vm_operations_struct omap_gem_vm_ops = {
> - .fault = omap_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
> static const struct file_operations omapdriver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> @@ -549,10 +543,7 @@ static struct drm_driver omap_drm_driver = {
> #endif
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = omap_gem_prime_export,
> .gem_prime_import = omap_gem_prime_import,
> - .gem_free_object_unlocked = omap_gem_free_object,
> - .gem_vm_ops = &omap_gem_vm_ops,
> .dumb_create = omap_gem_dumb_create,
> .dumb_map_offset = omap_gem_dumb_map_offset,
> .ioctls = ioctls,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d0d12d5dd76c..d68dc63dea0a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -487,7 +487,7 @@ static vm_fault_t omap_gem_fault_2d(struct drm_gem_object
> *obj,
> * vma->vm_private_data points to the GEM object that is backing this
> * mapping.
> */
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct drm_gem_object *obj = vma->vm_private_data;
> @@ -1169,6 +1169,18 @@ static bool omap_gem_validate_flags(struct drm_device
> *dev, u32 flags)
> return true;
> }
>
> +static const struct vm_operations_struct omap_gem_vm_ops = {
> + .fault = omap_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs omap_gem_object_funcs = {
> + .free = omap_gem_free_object,
> + .export = omap_gem_prime_export,
> + .vm_ops = &omap_gem_vm_ops,
> +};
> +
> /* GEM buffer object constructor */
> struct drm_gem_object *omap_gem_new(struct drm_device *dev,
> union omap_gem_size gsize, u32 flags)
> @@ -1236,6 +1248,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device
> *dev,
> size = PAGE_ALIGN(gsize.bytes);
> }
>
> + obj->funcs = &omap_gem_object_funcs;
> +
> /* Initialize the GEM object. */
> if (!(flags & OMAP_BO_MEM_SHMEM)) {
> drm_gem_private_object_init(dev, obj, size);
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h
> b/drivers/gpu/drm/omapdrm/omap_gem.h
> index 729b7812a815..9e6b5c8195d9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.h
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.h
> @@ -69,7 +69,6 @@ struct dma_buf *omap_gem_prime_export(struct drm_gem_object
> *obj, int flags);
> struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
> struct dma_buf *buffer);
>
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf);
> int omap_gem_roll(struct drm_gem_object *obj, u32 roll);
> void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff);
> void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,
omap_gem_free_object() can also be made static, and removed from omap_gem.h.
Tested on AM5 EVM.
Reviewed-by: Tomi Valkeinen
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 3/3] gpu: drm: drivers: Convert printk(KERN_ to pr_
On 28/02/17 14:55, Joe Perches wrote: > Use a more common logging style. > > Miscellanea: > > o Coalesce formats and realign arguments > o Neaten a few macros now using pr_ > > Signed-off-by: Joe Perches For omap: Acked-by: Tomi Valkeinen Tomi signature.asc Description: OpenPGP digital signature ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/3] drm/bridge: migrate bridge_chains to per-encoder file
oder->funcs->debugfs_init(encoder, root); } diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c339fc85fd07..902bc3f99c2a 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -950,6 +950,4 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, } #endif -void drm_bridge_debugfs_init(struct drm_minor *minor); - #endif It would be nice to have a review from Tomi since he pushed the bridge chains debugfs. Apart that it looks fine: Reviewed-by: Neil Armstrong This change does more than move the code to per-encoder debugfs file: it changes the formatting, adding textual representations for the flags, and drops the use of drm_printer. I'd prefer to have such changes separately, but as it's a small patch I guess it's fine-ish. But they should at least be mentioned in the patch description. With that addressed: Reviewed-by: Tomi Valkeinen Tomi
Re: [Freedreno] [PATCH v1 0/7] drm/bridge_connector: perform HPD enablement automatically
Hi,
On 29/04/2022 21:51, Dmitry Baryshkov wrote:
From all the drivers using drm_bridge_connector only iMX/dcss and OMAP
DRM driver do a proper work of calling
drm_bridge_connector_en/disable_hpd() in right places. Rather than
teaching each and every driver how to properly handle
drm_bridge_connector's HPD, make that automatic.
Add two additional drm_connector helper funcs: enable_hpd() and
disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this
is the time where the drm_bridge_connector's functions are called by the
drivers too).
09077bc3116581f4d1cb961ec359ad56586e370b ("drm/bridge_connector: enable
HPD by default if supported") was merged in March, but I think that one
is bit broken
(https://lore.kernel.org/all/a28a4858-c66a-6737-a9fc-502f591ba...@ideasonboard.com/).
To get omapdrm work without WARNs we could just revert that commit, but
I think this series makes things cleaner.
There's one small problem with this series: in drm_bridge_connector.c
the drm_bridge_hpd_disable() function is called from
_drm_bridge_connector_disable_hpd() and from
drm_bridge_connector_destroy(). This causes two hpd_disable calls when
removing the driver modules. I think the call from
drm_bridge_connector_destroy() could be removed, as
_drm_bridge_connector_disable_hpd() should always get called when
removing the drivers.
Dmitry, are you still interested in this series? Can you rebase on top
of current upstream, and revert 09077bc3116581f4d1cb961ec359ad56586e370b
first?
Tomi
Re: [Freedreno] [PATCH v1 7/7] drm/bridge_connector: drop drm_bridge_connector_en/disable_hpd()
On 29/04/2022 21:51, Dmitry Baryshkov wrote:
Now as all drivers stopped calling drm_bridge_connector_enable_hpd() and
drm_bridge_connector_disable_hpd() it is safe to remove them complelely.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/drm_bridge_connector.c | 25 -
include/drm/drm_bridge_connector.h | 2 --
2 files changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge_connector.c
b/drivers/gpu/drm/drm_bridge_connector.c
index 1592da3b9403..d9c1f61b6fb6 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -128,18 +128,6 @@ static void drm_bridge_connector_hpd_cb(void *cb_data,
drm_kms_helper_hotplug_event(dev);
}
-/**
- * drm_bridge_connector_enable_hpd - Enable hot-plug detection for the
connector
- * @connector: The DRM bridge connector
- *
- * This function enables hot-plug detection for the given bridge connector.
- * This is typically used by display drivers in their resume handler.
- */
-void drm_bridge_connector_enable_hpd(struct drm_connector *connector)
-{
-}
-EXPORT_SYMBOL_GPL(drm_bridge_connector_enable_hpd);
-
static void _drm_bridge_connector_enable_hpd(struct drm_connector *connector)
Here (and for _drm_bridge_connector_disable_hpd) you could remove the
prefix underscore.
Tomi
Re: [Freedreno] [PATCH v2 2/7] drm/probe-helper: enable and disable HPD on connectors
On 24/10/2022 18:39, Dmitry Baryshkov wrote:
Intruct two drm_connector_helper_funcs: enable_hpd() and disable_hpd().
"Introduce"?
They are called by drm_kms_helper_poll_enable() and
drm_kms_helper_poll_disable() (and thus drm_kms_helper_poll_init() and
drm_kms_helper_poll_fini()) respectively.
This allows drivers to rely on drm_kms_helper_poll for enabling and
disabling HPD detection rather than doing that manually.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/drm_probe_helper.c | 19 +++
include/drm/drm_modeset_helper_vtables.h | 22 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index f97fda3b1d34..a7b4590d8ec1 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -251,6 +251,12 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter) {
+ const struct drm_connector_helper_funcs *funcs =
+ connector->helper_private;
+
+ if (funcs && funcs->enable_hpd)
+ funcs->enable_hpd(connector);
+
if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT))
poll = true;
@@ -805,12 +811,25 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
static void drm_kms_helper_poll_disable_fini(struct drm_device *dev, bool fini)
{
+ struct drm_connector *connector;
+ struct drm_connector_list_iter conn_iter;
+
if (!dev->mode_config.poll_enabled)
return;
if (fini)
dev->mode_config.poll_enabled = false;
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ const struct drm_connector_helper_funcs *funcs =
+ connector->helper_private;
+
+ if (funcs && funcs->disable_hpd)
+ funcs->disable_hpd(connector);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+
cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
}
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index fafa70ac1337..7aa1f01223f9 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1143,6 +1143,28 @@ struct drm_connector_helper_funcs {
*/
void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
struct drm_writeback_job *job);
+
+ /**
+* @enable_hpd:
+*
+* Enable hot-plug detection for the connector.
+*
+* This operation is optional.
+*
+* This callback is used by the drm_kms_helper_poll_enable() helpers.
+*/
+ void (*enable_hpd)(struct drm_connector *connector);
+
+ /**
+* @disable_hpd:
+*
+* Disable hot-plug detection for the connector.
+*
+* This operation is optional.
+*
+* This callback is used by the drm_kms_helper_poll_disable() helpers.
+*/
+ void (*disable_hpd)(struct drm_connector *connector);
};
/**
Re: [Freedreno] [PATCH v2 0/7] drm/bridge_connector: perform HPD enablement automatically
On 24/10/2022 18:39, Dmitry Baryshkov wrote: From all the drivers using drm_bridge_connector only iMX/dcss and OMAP DRM driver do a proper work of calling drm_bridge_connector_en/disable_hpd() in right places. Rather than teaching each and every driver how to properly handle drm_bridge_connector's HPD, make that automatic. Add two additional drm_connector helper funcs: enable_hpd() and disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this is the time where the drm_bridge_connector's functions are called by the drivers too). Changes since v1: - Rebased on top of v6.1-rc1 - Removed the drm_bridge_connector_enable_hpd() from drm_bridge_connector_init() - Removed extra underscore prefix from drm_bridge_connector_en/disable_hpd() helpers Dmitry Baryshkov (7): drm/poll-helper: merge drm_kms_helper_poll_disable() and _fini() drm/probe-helper: enable and disable HPD on connectors drm/bridge_connector: rely on drm_kms_helper_poll_* for HPD enablement drm/imx/dcss: stop using drm_bridge_connector_en/disable_hpd() drm/msm/hdmi: stop using drm_bridge_connector_en/disable_hpd() drm/omap: stop using drm_bridge_connector_en/disable_hpd() drm/bridge_connector: drop drm_bridge_connector_en/disable_hpd() drivers/gpu/drm/drm_bridge_connector.c | 27 +++- drivers/gpu/drm/drm_probe_helper.c | 40 ++- drivers/gpu/drm/imx/dcss/dcss-dev.c | 4 --- drivers/gpu/drm/imx/dcss/dcss-kms.c | 2 -- drivers/gpu/drm/msm/hdmi/hdmi.c | 2 -- drivers/gpu/drm/omapdrm/omap_drv.c | 41 include/drm/drm_bridge_connector.h | 2 -- include/drm/drm_modeset_helper_vtables.h | 22 + 8 files changed, 59 insertions(+), 81 deletions(-) For the series: Reviewed-by: Tomi Valkeinen This fixes the issue (WARN for "Hot plug detection already enabled") introduced by 09077bc3116581f4d1cb961ec359ad56586e370b, which you revert in the third patch. You could mention this, maybe as a fixes tag and a mention in the description. Tomi
Re: [Freedreno] [PATCH v3 0/7] drm/bridge_connector: perform HPD enablement automatically
On 28/12/2022 23:58, Dmitry Baryshkov wrote: On 02/11/2022 20:06, Dmitry Baryshkov wrote: From all the drivers using drm_bridge_connector only iMX/dcss and OMAP DRM driver do a proper work of calling drm_bridge_connector_en/disable_hpd() in right places. Rather than teaching each and every driver how to properly handle drm_bridge_connector's HPD, make that automatic. Add two additional drm_connector helper funcs: enable_hpd() and disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this is the time where the drm_bridge_connector's functions are called by the drivers too). Since we are at the beginning of the development window, gracious ping for this patchset. It would be nice to finally handle the bridge_connector's hpd properly. Calling drm_bridge_connector_enable_hpd() from drm_bridge_connector_init() is not a proper way to do this. It results in calling bridge->funcs->hpd_enable() before the rest of the pipeline was set up properly. For the series: Reviewed-by: Tomi Valkeinen I've been using this series in my local branch for quite a while to fix the HPD issues. Works for me. I still think the "fix" aspect should be highlighted more here, as the current upstream triggers a WARN for "Hot plug detection already enabled" (at least) on OMAP. Tomi
Re: [Freedreno] [PATCH v3 0/7] drm/bridge_connector: perform HPD enablement automatically
On 05/01/2023 14:31, Dmitry Baryshkov wrote: On 04/01/2023 11:05, Neil Armstrong wrote: On 04/01/2023 08:29, Tomi Valkeinen wrote: On 28/12/2022 23:58, Dmitry Baryshkov wrote: On 02/11/2022 20:06, Dmitry Baryshkov wrote: From all the drivers using drm_bridge_connector only iMX/dcss and OMAP DRM driver do a proper work of calling drm_bridge_connector_en/disable_hpd() in right places. Rather than teaching each and every driver how to properly handle drm_bridge_connector's HPD, make that automatic. Add two additional drm_connector helper funcs: enable_hpd() and disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this is the time where the drm_bridge_connector's functions are called by the drivers too). Since we are at the beginning of the development window, gracious ping for this patchset. It would be nice to finally handle the bridge_connector's hpd properly. Calling drm_bridge_connector_enable_hpd() from drm_bridge_connector_init() is not a proper way to do this. It results in calling bridge->funcs->hpd_enable() before the rest of the pipeline was set up properly. For the series: Reviewed-by: Tomi Valkeinen I've been using this series in my local branch for quite a while to fix the HPD issues. Works for me. Thanks! I still think the "fix" aspect should be highlighted more here, as the current upstream triggers a WARN for "Hot plug detection already enabled" (at least) on OMAP. LGTM then ! Tomi, Dmitry, I can push the whole serie via drm-misc-next or -fixes then, as you wish. I'm fine either way. We have been living with the warning for some time, so I don't think there is any urgency to get rid of it immediately. Yes, drm-misc-next is fine for me too. Tomi
Re: [Freedreno] [PATCH v5 08/13] drm/omapdrm: Use regular fbdev I/O helpers
On 30/05/2023 18:02, Thomas Zimmermann wrote:
Use the regular fbdev helpers for framebuffer I/O instead of DRM's
helpers. Omapdrm does not use damage handling, so DRM's fbdev helpers
are mere wrappers around the fbdev code.
By using fbdev helpers directly within each DRM fbdev emulation,
we can eventually remove DRM's wrapper functions entirely.
v4:
* use initializer macros for struct fb_ops
v2:
* use FB_SYS_HELPERS option
Signed-off-by: Thomas Zimmermann
Acked-by: Sam Ravnborg
Cc: Tomi Valkeinen
---
drivers/gpu/drm/omapdrm/Kconfig | 1 +
drivers/gpu/drm/omapdrm/omap_fbdev.c | 11 +++
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
index 76ded1568bd0..b4ac76c9f31b 100644
--- a/drivers/gpu/drm/omapdrm/Kconfig
+++ b/drivers/gpu/drm/omapdrm/Kconfig
@@ -4,6 +4,7 @@ config DRM_OMAP
depends on DRM && OF
depends on ARCH_OMAP2PLUS
select DRM_KMS_HELPER
+ select FB_SYS_HELPERS if DRM_FBDEV_EMULATION
select VIDEOMODE_HELPERS
select HDMI
default n
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index b950e93b3846..b7ccce0704a3 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -4,6 +4,8 @@
* Author: Rob Clark
*/
+#include
+
#include
#include
#include
@@ -95,20 +97,13 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
static const struct fb_ops omap_fb_ops = {
.owner = THIS_MODULE,
-
+ FB_DEFAULT_SYS_OPS,
.fb_check_var = drm_fb_helper_check_var,
.fb_set_par = drm_fb_helper_set_par,
.fb_setcmap = drm_fb_helper_setcmap,
.fb_blank = drm_fb_helper_blank,
.fb_pan_display = omap_fbdev_pan_display,
.fb_ioctl = drm_fb_helper_ioctl,
-
- .fb_read = drm_fb_helper_sys_read,
- .fb_write = drm_fb_helper_sys_write,
- .fb_fillrect = drm_fb_helper_sys_fillrect,
- .fb_copyarea = drm_fb_helper_sys_copyarea,
- .fb_imageblit = drm_fb_helper_sys_imageblit,
-
.fb_destroy = omap_fbdev_fb_destroy,
};
Reviewed-by: Tomi Valkeinen
Tomi
Re: [Freedreno] [PATCH 00/53] drm: Convert to platform remove callback returning void
On 08/06/2023 19:26, Laurent Pinchart wrote: Hi Doug, On Thu, Jun 08, 2023 at 09:08:15AM -0700, Doug Anderson wrote: On Thu, Jun 1, 2023 at 8:40 AM Uwe Kleine-König wrote: On Sun, May 07, 2023 at 06:25:23PM +0200, Uwe Kleine-König wrote: this patch series adapts the platform drivers below drivers/gpu/drm to use the .remove_new() callback. Compared to the traditional .remove() callback .remove_new() returns no value. This is a good thing because the driver core doesn't (and cannot) cope for errors during remove. The only effect of a non-zero return value in .remove() is that the driver core emits a warning. The device is removed anyhow and an early return from .remove() usually yields a resource leak. By changing the remove callback to return void driver authors cannot reasonably (but wrongly) assume any more that there happens some kind of cleanup later. I wonder if someone would volunteer to add the whole series to drm-misc-next?! It looks as if Neil applied quite a few of them already, so I looked at what was left... I'm a little hesitant to just apply the whole kit-and-caboodle to drm-misc-next since there are specific DRM trees for a bunch of them and it would be better if they landed there. ...so I went through all the patches that still applied to drm-misc-next, then used 'scripts/get_maintainer.pl --scm' to check if they were maintained through drm-misc. That still left quite a few patches. I've applied those ones and pushed to drm-misc-next: 71722685cd17 drm/xlnx/zynqmp_dpsub: Convert to platform remove callback returning void 1ed54a19f3b3 drm/vc4: Convert to platform remove callback returning void b957812839f8 drm/v3d: Convert to platform remove callback returning void e2fd3192e267 drm/tve200: Convert to platform remove callback returning void 84e6da7ad553 drm/tiny: Convert to platform remove callback returning void 34cdd1f691ad drm/tidss: Convert to platform remove callback returning void d665e3c9d37a drm/sun4i: Convert to platform remove callback returning void 0c259ab19146 drm/stm: Convert to platform remove callback returning void 9a865e45884a drm/sti: Convert to platform remove callback returning void 3c855610840e drm/rockchip: Convert to platform remove callback returning void e41977a83b71 drm/panfrost: Convert to platform remove callback returning void cef3776d0b5a drm/panel: Convert to platform remove callback returning void bd296a594e87 drm/mxsfb: Convert to platform remove callback returning void 38ca2d93d323 drm/meson: Convert to platform remove callback returning void fd1457d84bae drm/mcde: Convert to platform remove callback returning void 41a56a18615c drm/logicvc: Convert to platform remove callback returning void 980ec6444372 drm/lima: Convert to platform remove callback returning void 82a2c0cc1a22 drm/hisilicon: Convert to platform remove callback returning void c3b28b29ac0a drm/fsl-dcu: Convert to platform remove callback returning void a118fc6e71f9 drm/atmel-hlcdc: Convert to platform remove callback returning void 9a32dd324c46 drm/aspeed: Convert to platform remove callback returning void 2c7d291c498c drm/arm/malidp: Convert to platform remove callback returning void a920028df679 drm/arm/hdlcd: Convert to platform remove callback returning void 1bf3d76a7d15 drm/komeda: Convert to platform remove callback returning void The following ones appeared to apply to the top of drm-misc-next, but I didn't apply them since get_maintainer didn't say they were part of drm-misc-next: drm/tiny: Convert to platform remove callback returning void drm/tilcdc: Convert to platform remove callback returning void drm/sprd: Convert to platform remove callback returning void drm/shmobile: Convert to platform remove callback returning void drm/rcar-du: Convert to platform remove callback returning void If you don't mind, could you take the rcar-du patch through drm-misc too ? I don't plan to send another pull request for v6.5. drm/omap: Convert to platform remove callback returning void Tomi, should drm/omap moved to being maintained through drm-misc ? Yes. tilcdc, tidss and omapdrm are all maintained through drm-misc. But I guess I need to add something to the MAINTAINERS to make this clear. I'll look at it. Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
On 19/01/2025 16:59, Sui Jingfeng wrote:
But userspace must be able to continue allocating YUV buffers through
CREATE_DUMB.
I think, allocating YUV buffers through CREATE_DUMB interface is just
an *abuse* and *misuse* of this API for now.
Take the NV12 format as an example, NV12 is YUV420 planar format, have
two planar: the Y-planar and the UV-planar. The Y-planar appear first
in memory as an array of unsigned char values. The Y-planar is followed
immediately by the UV-planar, which is also an array of unsigned char
values that contains packed U (Cb) and V (Cr) samples.
But the 'drm_mode_create_dumb' structure is only intend to provide
descriptions for *one* planar.
struct drm_mode_create_dumb {
__u32 height;
__u32 width;
__u32 bpp;
__u32 flags;
__u32 handle;
__u32 pitch;
__u64 size;
};
An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4)
bytes.
So we can allocate an *equivalent* sized buffer to store the NV12 raw
data.
Either 'width * (height * 3/2)' where each pixel take up 8 bits,
or just 'with * height' where each pixels take up 12 bits.
However, all those math are just equivalents description to the original
NV12 format, neither are concrete correct physical description.
I don't see the problem. Allocating dumb buffers, if we don't have any
heuristics related to RGB behind it, is essentially just allocating a
specific amount of memory, defined by width, height and bitsperpixel.
I think, the problem will be that the 'width', 'height' and 'bpp'
are originally used to describe one plane. Those three parameters
has perfectly defined physical semantics.
But with multi planar formats, take NV12 image as an example,
for a 2×2 square of pixels, there are 4 Y samples but only 1 U
sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
to store the 2x2 square.
So its depth is 12 bits per pixel (48 / (2 * 2)).
so my problem is that the mentioned 12bpp in this example only
make sense in mathematics, it doesn't has a good physical
interpret. Do you agree with me on this technique point?
If I want to create an NV12 framebuffer, I allocate two dumb buffers,
one for Y and one for UV planes, and size them accordingly. And then
create the DRM framebuffer with those.
Then how you fill the value of the 'width', 'height' and 'bpp' of each
dumb buffers?
For 640x480-NV12:
plane 0: width = 640, height = 480, bpp = 8
plane 1: width = 640 / 2, height = 480 / 2, bpp = 16
Why not allocate storage for the whole on one shoot?
You can, if you adjust the parameters accordingly. However, if the
strides of the planes are not equal, I guess it might cause problems on
some platforms.
But I think it's usually simpler to allocate one buffer per plane, and
perhaps even better as it doesn't require as large contiguous memory area.
The modetest in libdrm can be an good example, send it[1] to you as an
reference.
Right, so modetest already does it successfully. So... What is the issue?
Everyone agrees that CREATE_DUMB is not the best ioctl to allocate
buffers, and one can't consider it to work identically across the
platforms. But it's what we have and what has been used for ages.
Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi,
On 19/01/2025 13:29, Sui Jingfeng wrote:
Hi,
On 2025/1/16 18:35, Dmitry Baryshkov wrote:
On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
wrote:
On 16/01/2025 10:09, Thomas Zimmermann wrote:
Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
[...]
My point is that we have the current UAPI, and we have userspace
using
it, but we don't have clear rules what the ioctl does with specific
parameters, and we don't document how it has to be used.
Perhaps the situation is bad, and all we can really say is that
CREATE_DUMB only works for use with simple RGB formats, and the
behavior for all other formats is platform specific. But I think even
that would be valuable in the UAPI docs.
To be honest, I would not want to specify behavior for anything but
the
linear RGB formats. If anything, I'd take Daniel's reply mail for
documentation as-is. Anyone stretching the UAPI beyond RGB is on
their own.
Thinking about this, I wonder if this change is good for omapdrm or
xilinx (probably other platforms too that support non-simple non-RGB
formats via dumb buffers): without this patch, in both drivers, the
pitch calculations just take the bpp as bit-per-pixels, align it up,
and that's it.
With this patch we end up using drm_driver_color_mode_format(), and
aligning buffers according to RGB formats figured out via heuristics.
It does happen to work, for the formats I tested, but it sounds like
something that might easily not work, as it's doing adjustments based
on wrong format.
Should we have another version of drm_mode_size_dumb() which just
calculates using the bpp, without the drm_driver_color_mode_format()
path? Or does the drm_driver_color_mode_format() path provide some
value for the drivers that do not currently do anything similar?
With the RGB-only rule, using drm_driver_color_mode_format() makes
sense. It aligns dumb buffers and video=, provides error checking, and
overall harmonizes code. The fallback is only required because of the
existing odd cases that already bend the UAPI's rules.
I have to disagree here.
On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
buffers are the only buffers you can get from the DRM driver. The dumb
buffers have been used to allocate linear and multiplanar YUV buffers
for a very long time on those platforms.
I tried to look around, but I did not find any mentions that
CREATE_DUMB
should only be used for RGB buffers. Is anyone outside the core
developers even aware of it?
If we don't use dumb buffers there, where do we get the buffers? Maybe
from a v4l2 device or from a gpu device, but often you don't have
those.
DMA_HEAP is there, of course.
Why can't there be a variant that takes a proper fourcc format
instead of
an imprecise bpp value?
Backwards compatibility. We can add an IOCTL for YUV / etc.
[...]
But userspace must be able to continue allocating YUV buffers through
CREATE_DUMB.
I think, allocating YUV buffers through CREATE_DUMB interface is just
an *abuse* and *misuse* of this API for now.
Take the NV12 format as an example, NV12 is YUV420 planar format, have
two planar: the Y-planar and the UV-planar. The Y-planar appear first
in memory as an array of unsigned char values. The Y-planar is followed
immediately by the UV-planar, which is also an array of unsigned char
values that contains packed U (Cb) and V (Cr) samples.
But the 'drm_mode_create_dumb' structure is only intend to provide
descriptions for *one* planar.
struct drm_mode_create_dumb {
__u32 height;
__u32 width;
__u32 bpp;
__u32 flags;
__u32 handle;
__u32 pitch;
__u64 size;
};
An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) bytes.
So we can allocate an *equivalent* sized buffer to store the NV12 raw data.
Either 'width * (height * 3/2)' where each pixel take up 8 bits,
or just 'with * height' where each pixels take up 12 bits.
However, all those math are just equivalents description to the original
NV12 format, neither are concrete correct physical description.
I don't see the problem. Allocating dumb buffers, if we don't have any
heuristics related to RGB behind it, is essentially just allocating a
specific amount of memory, defined by width, height and bitsperpixel.
If I want to create an NV12 framebuffer, I allocate two dumb buffers,
one for Y and one for UV planes, and size them accordingly. And then
create the DRM framebuffer with those.
Therefore, allocating YUV buffers through the dumb interface is just an
abuse for that API. We certainly can abuse more by allocating two dumb
buffers, one for Y-planer, another one for the UV-planer. But again,dumb
buffers can be (and must be) used for *scanout* directly. What will
yield if I commit the YUV buffers you allocated to the CRTC directly?
You'll see it on the screen? I don&
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 20/01/2025 09:49, Thomas Zimmermann wrote: Hi Am 16.01.25 um 11:03 schrieb Tomi Valkeinen: [...] Aligning video= and dumb buffers almost sounds like going backwards. video= parameter is bad, Who told you that? Video= is still the way to specify an initial display mode to the kernel and it will remain so. You did =). "It aligns dumb buffers and video=". I understand the need for drm_driver_color_mode_format() for video=. But I think it's bad for CREATE_DUMB, at least for the platforms which have never aimed for "RGB-only". So you're not in favor of a drm_mode_size_dumb() version that does not use drm_driver_color_mode_format(), for these platforms? I'm still at loss as to why we would want to change the behavior of CREATE_DUMB. I see no upside, but I see the chance of regressions. Tomi
Re: [PATCH v2 15/25] drm/omapdrm: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi,
On 09/01/2025 16:57, Thomas Zimmermann wrote:
Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 8.
Signed-off-by: Thomas Zimmermann
Cc: Tomi Valkeinen
---
drivers/gpu/drm/omapdrm/omap_gem.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index b9c67e4ca360..b8413a2dcdeb 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -11,6 +11,7 @@
#include
#include
+#include
#include
#include
@@ -583,15 +584,13 @@ static int omap_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
struct drm_mode_create_dumb *args)
{
- union omap_gem_size gsize;
-
- args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-
- args->size = PAGE_ALIGN(args->pitch * args->height);
+ union omap_gem_size gsize = { };
+ int ret;
- gsize = (union omap_gem_size){
- .bytes = args->size,
- };
+ ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+ if (ret)
+ return ret;
+ gsize.bytes = args->size;
return omap_gem_new_handle(dev, file, gsize,
OMAP_BO_SCANOUT | OMAP_BO_WC, &args->handle);
Tested on dra76 evm.
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 15/01/2025 13:37, Thomas Zimmermann wrote: Hi Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: [...] These are all good points. Did you read my discussion with Andy on patch 2? I think it resolves all the points you have. The current CREATE_DUMB I had missed the discussion, and, indeed, the patch you attached fixes the problem on Xilinx. Great. Thanks for testing. ioctl is unsuited for anything but the simple RGB formats. The bpp It's a bit difficult to use, but is it really unsuited? bitsperpixel, width and height do give an exact pitch and size, do they not? It does require the userspace to handle the subsampling and planes, though, so far from perfect. The bpp value sets the number of bits per pixel; except for bpp==15 (XRGB1555), where it sets the color depth. OR bpp is the color depth; except for bpp==32 (XRGB), where it is the number of bits per pixel. It's therefore best to interpret it like a color-mode enum. Ah, right... That's horrible =). And I assume it's not really possible to define the bpp to mean bits per pixel, except for a few special cases like 15? Why do we even really care about color depth here? We're just allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for XRGB1555 too? So, I'm all for a new ioctl, but I don't right away see why the current ioctl couldn't be used. Which makes me wonder about the drm_warn() in your patch, and the "userspace throws in arbitrary values for bpp and relies on the kernel to figure it out". Maybe I'm missing something here. I was unsure about the drm_warn() as well. It's not really wrong to have odd bpp values, but handing in an unknown bpp value might point to a user-space error. At least there should be a drm_dbg(). parameter is not very precise. The solution would be a new ioctl call that receives the DRM format and returns a buffer for each individual plane. Yes, I think that makes sense. That's a long road, though =). So my question is, is CREATE_DUMB really unsuitable for other than simple RGB formats, or can it be suitable if we just define how the userspace should use it for multiplanar, subsampled formats? That would duplicate format and hardware information in user-space. Some But we already have that, don't we? We have drivers and userspace that support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we don't document how CREATE_DUMB has to be used to allocate multiplanar subsampled buffers, so the userspace devs have to "guess". hardware might have odd per-plane limitations that only the driver knows about. For example, there's another discussion on dri-devel about pitch- alignment requirements of DRM_FORMAT_MOD_LINEAR on various hardware. That affects dumb buffers as well. I don't think that there's an immediate need for a CREATE_DUMB2, but it seems worth to keep in mind. Yes, the current CREATE_DUMB can't cover all the hardware. We do need CREATE_DUMB2, sooner or later. I just hope we can define and document a set of rules that allows using CREATE_DUMB for the cases where it sensibly works (and is already being used). Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 15/01/2025 14:34, Thomas Zimmermann wrote: Hi Am 15.01.25 um 13:06 schrieb Tomi Valkeinen: Hi, On 15/01/2025 13:37, Thomas Zimmermann wrote: Hi Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: [...] These are all good points. Did you read my discussion with Andy on patch 2? I think it resolves all the points you have. The current CREATE_DUMB I had missed the discussion, and, indeed, the patch you attached fixes the problem on Xilinx. Great. Thanks for testing. ioctl is unsuited for anything but the simple RGB formats. The bpp It's a bit difficult to use, but is it really unsuited? bitsperpixel, width and height do give an exact pitch and size, do they not? It does require the userspace to handle the subsampling and planes, though, so far from perfect. The bpp value sets the number of bits per pixel; except for bpp==15 (XRGB1555), where it sets the color depth. OR bpp is the color depth; except for bpp==32 (XRGB), where it is the number of bits per pixel. It's therefore best to interpret it like a color-mode enum. Ah, right... That's horrible =). And I assume it's not really possible to define the bpp to mean bits per pixel, except for a few special cases like 15? Why do we even really care about color depth here? We're just allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for XRGB1555 too? Drivers always did that, but it does not work correctly for (bpp < 8). As we already have helpers to deal with bpp, it makes sense to use them. This also aligns dumb buffers with the kernel's video= parameter, which as the same odd semantics. The fallback that uses bpp directly will hopefully be the exception. Hmm, well... If we had a 64-bit RGB format in the list of "legacy fb formats", I wouldn't have noticed anything. And if I would just use 32 as the bpp, and adjust width accordingly, it would also have worked. So, I expect the fallback to be an exception, And by working I mean that I can run my apps fine, but the internal operation would sure be odd: allocating any YUV buffer will cause drm_mode_size_dumb() to get an RGB format from drm_driver_color_mode_format(), and get a drm_format_info_min_pitch() for an RGB format. So, I'm all for a new ioctl, but I don't right away see why the current ioctl couldn't be used. Which makes me wonder about the drm_warn() in your patch, and the "userspace throws in arbitrary values for bpp and relies on the kernel to figure it out". Maybe I'm missing something here. I was unsure about the drm_warn() as well. It's not really wrong to have odd bpp values, but handing in an unknown bpp value might point to a user-space error. At least there should be a drm_dbg(). parameter is not very precise. The solution would be a new ioctl call that receives the DRM format and returns a buffer for each individual plane. Yes, I think that makes sense. That's a long road, though =). So my question is, is CREATE_DUMB really unsuitable for other than simple RGB formats, or can it be suitable if we just define how the userspace should use it for multiplanar, subsampled formats? That would duplicate format and hardware information in user-space. Some But we already have that, don't we? We have drivers and userspace that support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we don't document how CREATE_DUMB has to be used to allocate multiplanar subsampled buffers, so the userspace devs have to "guess". Yeah, there are constrains in the scanline and buffer alignments and orientation. And if we say that bpp==12 means NV12, it will be a problem for all other cases where bpp==12 makes sense. I feel I still don't quite understand. Can't we define and document CREATE_DUMB like this: If (bpp < 8 || is_power_of_two(bpp)) bpp means bitsperpixel pitch is args->width * args->bpp / 8, aligned up to driver-specific-align else bpp is a legacy parameter, and we deal with it case by case. list the cases and what they mean And describe that when allocating subsampled buffers, the caller must adjust the width and height accordingly. And that the bpp and width can also refer to pixel groups. Or if the currently existing code prevents the above for 16 and 32 bpps, how about defining that any non-RGB or not-simple buffer has to be allocated with bpp=8, and the userspace has to align the pitch correctly according to the format and platform's hw restrictions? Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi,
On 15/01/2025 12:26, Thomas Zimmermann wrote:
Hi
Am 15.01.25 um 11:13 schrieb Tomi Valkeinen:
Hi!
On 09/01/2025 16:57, Thomas Zimmermann wrote:
Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.
Signed-off-by: Thomas Zimmermann
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/
xlnx/zynqmp_kms.c
index b47463473472..7ea0cd4f71d3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct
drm_file *file_priv,
struct drm_mode_create_dumb *args)
{
struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
- unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+ int ret;
/* Enforce the alignment constraints of the DMA engine. */
- args->pitch = ALIGN(pitch, dpsub->dma_align);
+ ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
+ if (ret)
+ return ret;
return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
}
I have some trouble with this one.
I have sent a series to add some pixel formats:
https://lore.kernel.org/all/20250115-xilinx-formats-
v2-0-160327ca6...@ideasonboard.com/
Let's look at XV15. It's similar to NV12, but 10 bits per component,
and some packing and padding.
First plane: 3 pixels in a 32 bit group
Second plane: 3 pixels in a 64 bit group, 2x2 subsampled
So, on average, a pixel on the first plane takes 32 / 3 = 10.666...
bits on a line. That's not a usable number for the
DRM_IOCTL_MODE_CREATE_DUMB ioctl.
What I did was to use the pixel group size as "bpp" for
DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:
Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes
First plane: 720 / 3 = 240 pixel groups
Second plane: 720 / 2 / 3 = 120 pixel groups
So I allocated the two planes with:
240 x 576 with 32 bitspp
120 x 288 with 64 bitspp
This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the
docs, I can't right away see anything there that says my tactic was
not allowed.
The above doesn't work anymore with this patch, as the code calls
drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a
bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB
fourcc for a dumb buffer allocation.
So, what to do here? Am I doing something silly? What's the correct
way to allocate the buffers for XV15? Should I just use 32 bitspp for
the plane 2 too, and double the width (this works)?
Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The
xilinx driver can, of course, just not use drm_mode_size_dumb(). But
if so, I guess the limitations of drm_mode_size_dumb() should be
documented.
Do we need a new dumb-alloc ioctl that takes the format and plane
number as parameters? Or alternatively a simpler dumb-alloc that
doesn't have width and bpp, but instead takes a stride and height as
parameters? I think those would be easier for the userspace to use,
instead of trying to adjust the parameters to be suitable for the kernel.
These are all good points. Did you read my discussion with Andy on patch
2? I think it resolves all the points you have. The current CREATE_DUMB
I had missed the discussion, and, indeed, the patch you attached fixes
the problem on Xilinx.
ioctl is unsuited for anything but the simple RGB formats. The bpp
It's a bit difficult to use, but is it really unsuited? bitsperpixel,
width and height do give an exact pitch and size, do they not? It does
require the userspace to handle the subsampling and planes, though, so
far from perfect.
So, I'm all for a new ioctl, but I don't right away see why the current
ioctl couldn't be used. Which makes me wonder about the drm_warn() in
your patch, and the "userspace throws in arbitrary values for bpp and
relies on the kernel to figure it out". Maybe I'm missing something here.
parameter is not very precise. The solution would be a new ioctl call
that receives the DRM format and returns a buffer for each individual
plane.
Yes, I think that makes sense. That's a long road, though =). So my
question is, is CREATE_DUMB really unsuitable for other than simple RGB
formats, or can it be suitable if we just define how the userspace
should use it for multiplanar, subsampled formats?
Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 16/01/2025 11:38, Laurent Pinchart wrote: On Thu, Jan 16, 2025 at 10:43:40AM +0200, Laurent Pinchart wrote: On Wed, Jan 15, 2025 at 02:34:26PM +, Daniel Stone wrote: On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen wrote: No disagreement there, we need CREATE_DUMB2. My point is that we have the current UAPI, and we have userspace using it, but we don't have clear rules what the ioctl does with specific parameters, and we don't document how it has to be used. Perhaps the situation is bad, and all we can really say is that CREATE_DUMB only works for use with simple RGB formats, and the behavior for all other formats is platform specific. But I think even that would be valuable in the UAPI docs. Yeah, CREATE_DUMB only works for use with simple RGB formats in a linear layout. Not monochrome or YUV or tiled or displayed rotated or whatever. If it happens to accidentally work for other uses, that's fine, but it's not generically reliable for anything other than simple linear RGB. It's intended to let you do splash screens, consoles, recovery password entries, and software-rendered compositors if you really want. Anything more than that isn't 'dumb'. We have lots of software out there that rely on CREATE_DUMB supporting YUV linear formats, and lots of drivers (mostly on Arm I suppose) that implement YUV support in CREATE_DUMB. I'm fine replacing it with something better, but I think we need a standard ioctl that can create linear YUV buffers. I've been told many times that DRM doesn't want to standardize buffer allocation further than what CREATE_DUMB is made for. Can we reconsider this rule then ? Actually... Instead of adding a CREATE_DUMB2, it would be best on trying to leverage DMA heaps and deprecate allocating from the KMS device. If we allocate from DMA heaps, I think we then need a DRM ioctl which will tell you how big buffer(s) you need, based on the size and format. Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 16/01/2025 10:09, Thomas Zimmermann wrote: Hi Am 15.01.25 um 15:20 schrieb Tomi Valkeinen: [...] My point is that we have the current UAPI, and we have userspace using it, but we don't have clear rules what the ioctl does with specific parameters, and we don't document how it has to be used. Perhaps the situation is bad, and all we can really say is that CREATE_DUMB only works for use with simple RGB formats, and the behavior for all other formats is platform specific. But I think even that would be valuable in the UAPI docs. To be honest, I would not want to specify behavior for anything but the linear RGB formats. If anything, I'd take Daniel's reply mail for documentation as-is. Anyone stretching the UAPI beyond RGB is on their own. Thinking about this, I wonder if this change is good for omapdrm or xilinx (probably other platforms too that support non-simple non-RGB formats via dumb buffers): without this patch, in both drivers, the pitch calculations just take the bpp as bit-per-pixels, align it up, and that's it. With this patch we end up using drm_driver_color_mode_format(), and aligning buffers according to RGB formats figured out via heuristics. It does happen to work, for the formats I tested, but it sounds like something that might easily not work, as it's doing adjustments based on wrong format. Should we have another version of drm_mode_size_dumb() which just calculates using the bpp, without the drm_driver_color_mode_format() path? Or does the drm_driver_color_mode_format() path provide some value for the drivers that do not currently do anything similar? With the RGB-only rule, using drm_driver_color_mode_format() makes sense. It aligns dumb buffers and video=, provides error checking, and overall harmonizes code. The fallback is only required because of the existing odd cases that already bend the UAPI's rules. I have to disagree here. On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb buffers are the only buffers you can get from the DRM driver. The dumb buffers have been used to allocate linear and multiplanar YUV buffers for a very long time on those platforms. I tried to look around, but I did not find any mentions that CREATE_DUMB should only be used for RGB buffers. Is anyone outside the core developers even aware of it? If we don't use dumb buffers there, where do we get the buffers? Maybe from a v4l2 device or from a gpu device, but often you don't have those. DMA_HEAP is there, of course. So we have the option to get DMA_HEAP buffers, specifying just the size of the buffer. Here we only specify the size, so the userspace has to understand the requirements for the format and the platform. Or we can use CREATE_DUMB, specifying the width, height and bitsperpixel, and if we don't have any heuristics about figuring out the pixel format (as it has been), the end result is exactly the same as with DMA_HEAP (i.e. we essentially define the size of the buffer). So, on these platforms (omap, tidss, xilinx, rcar), the CREATE_DUMB has always been just "give me X amount of memory that can be used for scanout". With this series, the meaning of the ioctl changes, and it's now "give me an memory buffer buffer that works with an RGB format with this width, height, bpp". In practice I believe that doesn't cause regressions, as aligning buffers according to RGB pixel format rules happens to be fine for YUV formats too, but I'm not sure (and it already almost caused a regression with bpp=64). And I'm having trouble seeing the upside. Aligning video= and dumb buffers almost sounds like going backwards. video= parameter is bad, so let's also make dumb buffers bad? Harmonizing code is fine, but I think that can be done with a function that only does the fallback-case. So... I can only speak for the platforms I'm using and maintaining, but I'd rather keep the old behavior for CREATE_DUMB that we've had for ages. Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi, On 16/01/2025 12:17, Geert Uytterhoeven wrote: On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen wrote: On 16/01/2025 10:09, Thomas Zimmermann wrote: Am 15.01.25 um 15:20 schrieb Tomi Valkeinen: [...] My point is that we have the current UAPI, and we have userspace using it, but we don't have clear rules what the ioctl does with specific parameters, and we don't document how it has to be used. Perhaps the situation is bad, and all we can really say is that CREATE_DUMB only works for use with simple RGB formats, and the behavior for all other formats is platform specific. But I think even that would be valuable in the UAPI docs. To be honest, I would not want to specify behavior for anything but the linear RGB formats. If anything, I'd take Daniel's reply mail for documentation as-is. Anyone stretching the UAPI beyond RGB is on their own. Thinking about this, I wonder if this change is good for omapdrm or xilinx (probably other platforms too that support non-simple non-RGB formats via dumb buffers): without this patch, in both drivers, the pitch calculations just take the bpp as bit-per-pixels, align it up, and that's it. With this patch we end up using drm_driver_color_mode_format(), and aligning buffers according to RGB formats figured out via heuristics. It does happen to work, for the formats I tested, but it sounds like something that might easily not work, as it's doing adjustments based on wrong format. Should we have another version of drm_mode_size_dumb() which just calculates using the bpp, without the drm_driver_color_mode_format() path? Or does the drm_driver_color_mode_format() path provide some value for the drivers that do not currently do anything similar? With the RGB-only rule, using drm_driver_color_mode_format() makes sense. It aligns dumb buffers and video=, provides error checking, and overall harmonizes code. The fallback is only required because of the existing odd cases that already bend the UAPI's rules. I have to disagree here. On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb buffers are the only buffers you can get from the DRM driver. The dumb buffers have been used to allocate linear and multiplanar YUV buffers for a very long time on those platforms. I tried to look around, but I did not find any mentions that CREATE_DUMB should only be used for RGB buffers. Is anyone outside the core developers even aware of it? If we don't use dumb buffers there, where do we get the buffers? Maybe from a v4l2 device or from a gpu device, but often you don't have those. DMA_HEAP is there, of course. Why can't there be a variant that takes a proper fourcc format instead of an imprecise bpp value? There can, but it's somewhat a different topic, although it's been covered a bit in this thread. My specific concern here is the current CREATE_DUMB, and (not) changing how it behaves. Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
On 15/01/2025 15:45, Thomas Zimmermann wrote: Hi Am 15.01.25 um 14:33 schrieb Tomi Valkeinen: [...] Yeah, there are constrains in the scanline and buffer alignments and orientation. And if we say that bpp==12 means NV12, it will be a problem for all other cases where bpp==12 makes sense. I feel I still don't quite understand. Can't we define and document CREATE_DUMB like this: If (bpp < 8 || is_power_of_two(bpp)) bpp means bitsperpixel pitch is args->width * args->bpp / 8, aligned up to driver- specific-align else bpp is a legacy parameter, and we deal with it case by case. list the cases and what they mean And describe that when allocating subsampled buffers, the caller must adjust the width and height accordingly. And that the bpp and width can also refer to pixel groups. Or if the currently existing code prevents the above for 16 and 32 bpps, how about defining that any non-RGB or not-simple buffer has to be allocated with bpp=8, and the userspace has to align the pitch correctly according to the format and platform's hw restrictions? What if a hardware requires certain per-format alignments? Or requires certain alignments for each plane? Or only supports tile modes? Or has strict limits on the maximum buffer size? It is not possible to encode all this in a simple 32-bit value. So user- space code has to be aware of all this and tweak bpp-based allocation to make it work. Obviously you can use the current UAPI for your use case. It's just not optimal or future proof. No disagreement there, we need CREATE_DUMB2. My point is that we have the current UAPI, and we have userspace using it, but we don't have clear rules what the ioctl does with specific parameters, and we don't document how it has to be used. Perhaps the situation is bad, and all we can really say is that CREATE_DUMB only works for use with simple RGB formats, and the behavior for all other formats is platform specific. But I think even that would be valuable in the UAPI docs. Thinking about this, I wonder if this change is good for omapdrm or xilinx (probably other platforms too that support non-simple non-RGB formats via dumb buffers): without this patch, in both drivers, the pitch calculations just take the bpp as bit-per-pixels, align it up, and that's it. With this patch we end up using drm_driver_color_mode_format(), and aligning buffers according to RGB formats figured out via heuristics. It does happen to work, for the formats I tested, but it sounds like something that might easily not work, as it's doing adjustments based on wrong format. Should we have another version of drm_mode_size_dumb() which just calculates using the bpp, without the drm_driver_color_mode_format() path? Or does the drm_driver_color_mode_format() path provide some value for the drivers that do not currently do anything similar? Tomi
Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi!
On 09/01/2025 16:57, Thomas Zimmermann wrote:
Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.
Signed-off-by: Thomas Zimmermann
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index b47463473472..7ea0cd4f71d3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct drm_file
*file_priv,
struct drm_mode_create_dumb *args)
{
struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
- unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+ int ret;
/* Enforce the alignment constraints of the DMA engine. */
- args->pitch = ALIGN(pitch, dpsub->dma_align);
+ ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
+ if (ret)
+ return ret;
return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
}
I have some trouble with this one.
I have sent a series to add some pixel formats:
https://lore.kernel.org/all/20250115-xilinx-formats-v2-0-160327ca6...@ideasonboard.com/
Let's look at XV15. It's similar to NV12, but 10 bits per component, and
some packing and padding.
First plane: 3 pixels in a 32 bit group
Second plane: 3 pixels in a 64 bit group, 2x2 subsampled
So, on average, a pixel on the first plane takes 32 / 3 = 10.666... bits
on a line. That's not a usable number for the DRM_IOCTL_MODE_CREATE_DUMB
ioctl.
What I did was to use the pixel group size as "bpp" for
DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:
Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes
First plane: 720 / 3 = 240 pixel groups
Second plane: 720 / 2 / 3 = 120 pixel groups
So I allocated the two planes with:
240 x 576 with 32 bitspp
120 x 288 with 64 bitspp
This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the
docs, I can't right away see anything there that says my tactic was not
allowed.
The above doesn't work anymore with this patch, as the code calls
drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a
bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB fourcc
for a dumb buffer allocation.
So, what to do here? Am I doing something silly? What's the correct way
to allocate the buffers for XV15? Should I just use 32 bitspp for the
plane 2 too, and double the width (this works)?
Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The
xilinx driver can, of course, just not use drm_mode_size_dumb(). But if
so, I guess the limitations of drm_mode_size_dumb() should be documented.
Do we need a new dumb-alloc ioctl that takes the format and plane number
as parameters? Or alternatively a simpler dumb-alloc that doesn't have
width and bpp, but instead takes a stride and height as parameters? I
think those would be easier for the userspace to use, instead of trying
to adjust the parameters to be suitable for the kernel.
Tomi
Re: [PATCH 29/34] drm: zynqmp_dp: convert to devm_drm_bridge_alloc() API
Hi, On 07/04/2025 17:23, Luca Ceresoli wrote: This is the new API for allocating DRM bridges. This driver has a peculiar structure. zynqmp_dpsub.c is the actual driver, which delegates to a submodule (zynqmp_dp.c) the allocation of a sub-structure embedding the drm_bridge and its initialization, however it does not delegate the drm_bridge_add(). Hence, following carefully the code flow, it is correct to change the allocation function and .funcs assignment in the submodule, while the drm_bridge_add() is not in that submodule. Signed-off-by: Luca Ceresoli --- Cc: Laurent Pinchart Cc: Michal Simek Cc: Tomi Valkeinen --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 11d2415fb5a1f7fad03421898331289f2295d68b..de22b6457a78a7a2110f9f308d0b5a8700544010 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -2439,9 +2439,9 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) struct zynqmp_dp *dp; int ret; - dp = kzalloc(sizeof(*dp), GFP_KERNEL); - if (!dp) - return -ENOMEM; + dp = devm_drm_bridge_alloc(&pdev->dev, struct zynqmp_dp, bridge, &zynqmp_dp_bridge_funcs); + if (IS_ERR(dp)) + return PTR_ERR(dp); dp->dev = &pdev->dev; dp->dpsub = dpsub; @@ -2488,7 +2488,6 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) /* Initialize the bridge. */ bridge = &dp->bridge; - bridge->funcs = &zynqmp_dp_bridge_funcs; bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD; bridge->type = DRM_MODE_CONNECTOR_DisplayPort; I haven't had time to look at this more, but jfyi: I got this when unloading modules, but it doesn't seem to happen every time: [ 103.010533] [ cut here ] [ 103.015415] refcount_t: underflow; use-after-free. [ 103.020657] WARNING: CPU: 2 PID: 392 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x148 [ 103.029056] Modules linked in: zynqmp_dpsub(-) display_connector drm_display_helper drm_dma_helper drm_kms_helper drm drm_p anel_orientation_quirks [ 103.042437] CPU: 2 UID: 0 PID: 392 Comm: rmmod Not tainted 6.15.0-rc2+ #3 PREEMPT [ 103.050035] Hardware name: ZynqMP ZCU106 RevA (DT) [ 103.054836] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 103.061814] pc : refcount_warn_saturate+0xf4/0x148 [ 103.066632] lr : refcount_warn_saturate+0xf4/0x148 [ 103.071441] sp : 800083b5bbb0 [ 103.074766] x29: 800083b5bbb0 x28: 000806b23780 x27: [ 103.081953] x26: x25: x24: 000801a68400 [ 103.089141] x23: 800081311a20 x22: 800083b5bc38 x21: 000801a68010 [ 103.096329] x20: 0008040676c0 x19: 000804067240 x18: 0006 [ 103.103517] x17: 2e30303030303464 x16: 662d7968703a7968 x15: 800083b5b5a0 [ 103.110705] x14: x13: 000c x12: [ 103.117892] x11: 80008163d6bc x10: 0028 x9 : 800080ead38c [ 103.125080] x8 : 800083b5b908 x7 : x6 : 800083b5b9c0 [ 103.132268] x5 : 800083b5b948 x4 : 0001 x3 : 00db [ 103.139455] x2 : x1 : x0 : 000806b23780 [ 103.146644] Call trace: [ 103.149102] refcount_warn_saturate+0xf4/0x148 (P) [ 103.153918] drm_bridge_put.part.0+0x88/0xa0 [drm] [ 103.159188] drm_bridge_put_void+0x1c/0x38 [drm] [ 103.164231] devm_action_release+0x1c/0x30 [ 103.168354] release_nodes+0x68/0xa8 [ 103.171957] devres_release_all+0x98/0xf0 [ 103.175993] device_unbind_cleanup+0x20/0x70 [ 103.180291] device_release_driver_internal+0x208/0x250 [ 103.185542] driver_detach+0x54/0xa8 [ 103.189145] bus_remove_driver+0x78/0x108 [ 103.193181] driver_unregister+0x38/0x70 [ 103.197131] platform_driver_unregister+0x1c/0x30 [ 103.201862] zynqmp_dpsub_driver_exit+0x18/0x1100 [zynqmp_dpsub] [ 103.207931] __arm64_sys_delete_module+0x1a8/0x2d0 [ 103.212748] invoke_syscall+0x50/0x120 [ 103.216524] el0_svc_common.constprop.0+0x48/0xf0 [ 103.221256] do_el0_svc+0x24/0x38 [ 103.224598] el0_svc+0x48/0x128 [ 103.227766] el0t_64_sync_handler+0x10c/0x138 [ 103.232150] el0t_64_sync+0x1a4/0x1a8 [ 103.235841] irq event stamp: 7936 [ 103.239173] hardirqs last enabled at (7935): [] finish_task_switch.isra.0+0xb0/0x2a0 [ 103.248600] hardirqs last disabled at (7936): [] el1_dbg+0x24/0x90 [ 103.256369] softirqs last enabled at (7930): [] handle_softirqs+0x4a0/0x4c0 [ 103.265007] softirqs last disabled at (7905): [] __do_softirq+0x1c/0x28 [ 103.273211] ---[ end trace ]--- Tomi
Re: [PATCH 29/34] drm: zynqmp_dp: convert to devm_drm_bridge_alloc() API
Hi, On 07/04/2025 17:23, Luca Ceresoli wrote: This is the new API for allocating DRM bridges. This driver has a peculiar structure. zynqmp_dpsub.c is the actual driver, which delegates to a submodule (zynqmp_dp.c) the allocation of a sub-structure embedding the drm_bridge and its initialization, however it does not delegate the drm_bridge_add(). Hence, following carefully the code flow, it is correct to change the allocation function and .funcs assignment in the submodule, while the drm_bridge_add() is not in that submodule. Signed-off-by: Luca Ceresoli --- Cc: Laurent Pinchart Cc: Michal Simek Cc: Tomi Valkeinen --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 11d2415fb5a1f7fad03421898331289f2295d68b..de22b6457a78a7a2110f9f308d0b5a8700544010 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -2439,9 +2439,9 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) struct zynqmp_dp *dp; int ret; - dp = kzalloc(sizeof(*dp), GFP_KERNEL); - if (!dp) - return -ENOMEM; + dp = devm_drm_bridge_alloc(&pdev->dev, struct zynqmp_dp, bridge, &zynqmp_dp_bridge_funcs); + if (IS_ERR(dp)) + return PTR_ERR(dp); dp->dev = &pdev->dev; dp->dpsub = dpsub; @@ -2488,7 +2488,6 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) /* Initialize the bridge. */ bridge = &dp->bridge; - bridge->funcs = &zynqmp_dp_bridge_funcs; bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD; bridge->type = DRM_MODE_CONNECTOR_DisplayPort; To add to my last mail, this clearly cannot be right, as it changes kzalloc call to devm_* call, without removing the kfree()s... Tomi
Re: [PATCH v3 02/25] drm/dumb-buffers: Provide helper to set pitch and size
Hi, On 20/02/2025 12:05, Thomas Zimmermann wrote: Hi Am 20.02.25 um 10:18 schrieb Tomi Valkeinen: [...] + * Color modes of 10, 12, 15, 30 and 64 are only supported for use by + * legacy user space. Please don't use them in new code. Other modes + * are not support. + * + * Do not attempt to allocate anything but linear framebuffer memory + * with single-plane RGB data. Allocation of other framebuffer + * layouts requires dedicated ioctls in the respective DRM driver. According to this, every driver that supports, say, NV12, should implement their own custom ioctl to do the exact same thing? And, of course, every userspace app that uses, say, NV12, should then add code for all these platforms to call the custom ioctls? Yes, that's exactly the current status. There has been discussion about a new dumb-create ioctl that takes a DRM format as parameter. I'm all for it, but it's out of the scope for this series. As libdrm's modetest currently supports YUV formats with dumb buffers, should we remove that code, as it's not correct and I'm sure people use libdrm code as a reference? Of course not. Well, I'm not serious above, but I think all my points from the earlier version are still valid. I don't like this. It changes the parameters of the ioctl (bpp used to be bits-per-pixel, not it's "color mode"), and the behavior of the ioctl, behavior that we've had for a very long time, and we have no idea how many users there are that will break (could be none, of course). And the documentation changes make the current behavior and uses wrong or legacy. Before I go into details about this statement, what use case exactly are you referring to when you say that behavior changes? For every dumb_buffer allocation with bpp that is not divisible by 8, the result is different, i.e. instead of DIV_ROUND_UP(width * bpp, 8), we now have width * DIV_ROUND_UP(bpp, 8). This, of course, depends on the driver implementation. Some already do the latter. This change also first calls the drm_driver_color_mode_format(), which could change the behavior even more, but afaics at the moment does not. Although, maybe some platform does width * DIV_ROUND_UP(bpp, 8) even for bpp < 8, and then this series changes it for 1, 2 and 4 bpps (but not for 3, 5, 6, 7, if I'm not mistaken). However, as the bpp is getting rounded up, this probably won't break any user. But it _is_ a change in the behavior of a uapi, and every time we change a uapi that's been out there for a long time, I'm getting slightly uncomfortable. So, as a summary, I have a feeling that nothing will break, but I can't say for sure. And as I'm having trouble seeing the benefit of this change for the user, I get even more uncomfortable. Tomi
Re: [PATCH v3 02/25] drm/dumb-buffers: Provide helper to set pitch and size
Hi,
On 18/02/2025 16:23, Thomas Zimmermann wrote:
Add drm_modes_size_dumb(), a helper to calculate the dumb-buffer
scanline pitch and allocation size. Implementations of struct
drm_driver.dumb_create can call the new helper for their size
computations.
There is currently quite a bit of code duplication among DRM's
memory managers. Each calculates scanline pitch and buffer size
from the given arguments, but the implementations are inconsistent
in how they treat alignment and format support. Later patches will
unify this code on top of drm_mode_size_dumb() as much as possible.
drm_mode_size_dumb() uses existing 4CC format helpers to interpret
the given color mode. This makes the dumb-buffer interface behave
similar the kernel's video= parameter. Current per-driver implementations
again likely have subtle differences or bugs in how they support color
modes.
The dumb-buffer UAPI is only specified for known color modes. These
values describe linear, single-plane RGB color formats or legacy index
formats. Other values should not be specified. But some user space
still does. So for unknown color modes, there are a number of known
exceptions for which drm_mode_size_dumb() calculates the pitch from
the bpp value, as before. All other values work the same but print
an error.
v3:
- document the UAPI semantics
- compute scanline pitch from for unknown color modes (Andy, Tomi)
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_dumb_buffers.c | 116 +
include/drm/drm_dumb_buffers.h | 14
include/uapi/drm/drm_mode.h| 46 +++-
3 files changed, 175 insertions(+), 1 deletion(-)
create mode 100644 include/drm/drm_dumb_buffers.h
diff --git a/drivers/gpu/drm/drm_dumb_buffers.c
b/drivers/gpu/drm/drm_dumb_buffers.c
index 9916aaf5b3f2..600ab281712b 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -25,6 +25,8 @@
#include
#include
+#include
+#include
#include
#include
@@ -57,6 +59,120 @@
* a hardware-specific ioctl to allocate suitable buffer objects.
*/
+static int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
+ unsigned long pitch_align,
+ unsigned long size_align)
+{
+ u32 pitch = args->pitch;
+ u32 size;
+
+ if (!pitch)
+ return -EINVAL;
+
+ if (pitch_align)
+ pitch = roundup(pitch, pitch_align);
+
+ /* overflow checks for 32bit size calculations */
+ if (args->height > U32_MAX / pitch)
+ return -EINVAL;
+
+ if (!size_align)
+ size_align = PAGE_SIZE;
+ else if (!IS_ALIGNED(size_align, PAGE_SIZE))
+ return -EINVAL;
+
+ size = ALIGN(args->height * pitch, size_align);
+ if (!size)
+ return -EINVAL;
+
+ args->pitch = pitch;
+ args->size = size;
+
+ return 0;
+}
+
+/**
+ * drm_mode_size_dumb - Calculates the scanline and buffer sizes for dumb
buffers
+ * @dev: DRM device
+ * @args: Parameters for the dumb buffer
+ * @pitch_align: Scanline alignment in bytes
+ * @size_align: Buffer-size alignment in bytes
+ *
+ * The helper drm_mode_size_dumb() calculates the size of the buffer
+ * allocation and the scanline size for a dumb buffer. Callers have to
+ * set the buffers width, height and color mode in the argument @arg.
+ * The helper validates the correctness of the input and tests for
+ * possible overflows. If successful, it returns the dumb buffer's
+ * required scanline pitch and size in &args.
+ *
+ * The parameter @pitch_align allows the driver to specifies an
+ * alignment for the scanline pitch, if the hardware requires any. The
+ * calculated pitch will be a multiple of the alignment. The parameter
+ * @size_align allows to specify an alignment for buffer sizes. The
+ * returned size is always a multiple of PAGE_SIZE.
+ *
+ * Returns:
+ * Zero on success, or a negative error code otherwise.
+ */
+int drm_mode_size_dumb(struct drm_device *dev,
+ struct drm_mode_create_dumb *args,
+ unsigned long pitch_align,
+ unsigned long size_align)
+{
+ u64 pitch = 0;
+ u32 fourcc;
+
+ /*
+* The scanline pitch depends on the buffer width and the color
+* format. The latter is specified as a color-mode constant for
+* which we first have to find the corresponding color format.
+*
+* Different color formats can have the same color-mode constant.
+* For example XRGB and BGRX both have a color mode of 32.
+* It is possible to use different formats for dumb-buffer allocation
+* and rendering as long as all involved formats share the same
+* color-mode constant.
+*/
+ fourcc = drm_driver_color_mode_format(dev, args->bpp);
+ if (fourcc != DRM_FORMAT_INVALID) {
+ const struct drm
Re: [PATCH v3 02/25] drm/dumb-buffers: Provide helper to set pitch and size
Hi, On 21/02/2025 11:19, Thomas Zimmermann wrote: Hi Am 20.02.25 um 11:53 schrieb Tomi Valkeinen: Hi, On 20/02/2025 12:05, Thomas Zimmermann wrote: Hi Am 20.02.25 um 10:18 schrieb Tomi Valkeinen: [...] + * Color modes of 10, 12, 15, 30 and 64 are only supported for use by + * legacy user space. Please don't use them in new code. Other modes + * are not support. + * + * Do not attempt to allocate anything but linear framebuffer memory + * with single-plane RGB data. Allocation of other framebuffer + * layouts requires dedicated ioctls in the respective DRM driver. According to this, every driver that supports, say, NV12, should implement their own custom ioctl to do the exact same thing? And, of course, every userspace app that uses, say, NV12, should then add code for all these platforms to call the custom ioctls? Yes, that's exactly the current status. There has been discussion about a new dumb-create ioctl that takes a DRM format as parameter. I'm all for it, but it's out of the scope for this series. As libdrm's modetest currently supports YUV formats with dumb buffers, should we remove that code, as it's not correct and I'm sure people use libdrm code as a reference? Of course not. Well, I'm not serious above, but I think all my points from the earlier version are still valid. I don't like this. It changes the parameters of the ioctl (bpp used to be bits-per-pixel, not it's "color mode"), and the behavior of the ioctl, behavior that we've had for a very long time, and we have no idea how many users there are that will break (could be none, of course). And the documentation changes make the current behavior and uses wrong or legacy. Before I go into details about this statement, what use case exactly are you referring to when you say that behavior changes? For every dumb_buffer allocation with bpp that is not divisible by 8, the result is different, i.e. instead of DIV_ROUND_UP(width * bpp, 8), we now have width * DIV_ROUND_UP(bpp, 8). This, of course, depends on the driver implementation. Some already do the latter. The current dumb-buffer code does a stride computation at [1], which is correct for all cases; although over-allocates sometimes. It's the one you describe as "width * DIV_ROUND_UP(bpp, 8)". It's in the ioctl entry point, so it's somewhat authoritative for all driver's implementations. It's also used by several drivers. The other variant, "DIV_ROUND_UP(width * bpp, 8)", is used by gem-dma, gem-shmem and others. It can give incorrect results and possibly OOBs. To give a simple example, let's allocate 15-bit XRGB1555. Bpp is 15. With a width of 1024, that would result in 1920 bytes per scanline. But because XRGB1555 has a filler bit, so the pixel is actually 16 bit and a scanline needs to be 2048 bytes. The new code fixes that. This is not just a hypothetical scenario: we do have drivers that support XRGB1555 and some of them also export a preferred_depth of 15 to userspace. [2] In the nearby comment, you'll see that this value is meant for dumb buffers. Rounding up the depth value in user space is possible for RGB, but not for YUV. Here different pixel planes have a different number of bits. Sometimes pixels are sharing bits. The value of bits-per-pixel becomes meaningless. That's why it's also deprecated in struct drm_format_info. The struct instead uses a more complicated per-plane calculation to compute the number of bits per plane. [3] The user-space code currently doing YUV on dumb buffers simply got lucky. [1] https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/ drm_dumb_buffers.c#L77 [2] https://elixir.bootlin.com/linux/v6.13.3/source/include/drm/ drm_mode_config.h#L885 [3] https://elixir.bootlin.com/linux/v6.13.3/source/include/drm/ drm_fourcc.h#L83 This change also first calls the drm_driver_color_mode_format(), which could change the behavior even more, but afaics at the moment does not. Because currently each driver does its own thing, it can be hard to write user space that reliably allocates on all drivers. That's why it's important that parameters are not just raw numbers, but have well- defined semantics. The raw bpp is meaningless; it's also important to know which formats are associated with each value. Otherwise, you might get a dumb buffer with a bpp of 15, but it will be displayed incorrectly. This patch series finally implements this and clearly documents the assumptions behind the interfaces. The assumptions themselves have always existed. This is perhaps where the biggest gap in understanding/view is: I have always thought dumb-buffer's "bpp" to mean bits-per-pixel, where, for more complex formats, "pixel" is not necessarily a visible pixel but a container unit of some kind. So bpp * width = st
Re: [PATCH v4 02/25] drm/dumb-buffers: Provide helper to set pitch and size
*/
> + case 10: // DRM_FORMAT_NV{15,20,30}, DRM_FORMAT_P010
> + case 64: // DRM_FORMAT_{XRGB,XBGR,ARGB,ABGR}16161616F
> + pitch = args->width * DIV_ROUND_UP(args->bpp, SZ_8);
> + break;
> + }
> + }
> +
> + if (!pitch || pitch > U32_MAX)
> + return -EINVAL;
> +
> + args->pitch = pitch;
> +
> + return drm_mode_align_dumb(args, hw_pitch_align, hw_size_align);
> +}
> +EXPORT_SYMBOL(drm_mode_size_dumb);
> +
> int drm_mode_create_dumb(struct drm_device *dev,
>struct drm_mode_create_dumb *args,
>struct drm_file *file_priv)
> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
> new file mode 100644
> index ..1f3a8236fb3d
> --- /dev/null
> +++ b/include/drm/drm_dumb_buffers.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef __DRM_DUMB_BUFFERS_H__
> +#define __DRM_DUMB_BUFFERS_H__
> +
> +struct drm_device;
> +struct drm_mode_create_dumb;
> +
> +int drm_mode_size_dumb(struct drm_device *dev,
> +struct drm_mode_create_dumb *args,
> +unsigned long hw_pitch_align,
> +unsigned long hw_size_align);
> +
> +#endif
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index c082810c08a8..efe8f5ad35ee 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1058,7 +1058,7 @@ struct drm_mode_crtc_page_flip_target {
> * struct drm_mode_create_dumb - Create a KMS dumb buffer for scanout.
> * @height: buffer height in pixels
> * @width: buffer width in pixels
> - * @bpp: bits per pixel
> + * @bpp: color mode
> * @flags: must be zero
> * @handle: buffer object handle
> * @pitch: number of bytes between two consecutive lines
> @@ -1066,6 +1066,54 @@ struct drm_mode_crtc_page_flip_target {
> *
> * User-space fills @height, @width, @bpp and @flags. If the IOCTL succeeds,
> * the kernel fills @handle, @pitch and @size.
> + *
> + * The value of @bpp is a color-mode number describing a specific format
> + * or a variant thereof. The value often corresponds to the number of bits
> + * per pixel for most modes, although there are exceptions. Each color mode
> + * maps to a DRM format plus a number of modes with similar pixel layout.
> + * Framebuffer layout is always linear.
> + *
> + * Support for all modes and formats is optional. Even if dumb-buffer
> + * creation with a certain color mode succeeds, it is not guaranteed that
> + * the DRM driver supports any of the related formats. Most drivers support
> + * a color mode of 32 with a format of DRM_FORMAT_XRGB on their primary
> + * plane.
> + *
> + * ++++
> + * | Color mode | Framebuffer format | Compatible formats |
> + * ++++
> + * | 32 | * DRM_FORMAT_XRGB | * DRM_FORMAT_BGRX |
> + * ||| * DRM_FORMAT_RGBX |
> + * ||| * DRM_FORMAT_XBGR |
> + * ++++
> + * | 24 | * DRM_FORMAT_RGB888 | * DRM_FORMAT_BGR888 |
> + * ++++
> + * | 16 | * DRM_FORMAT_RGB565 | * DRM_FORMAT_BGR565 |
> + * ++++
> + * | 15 | * DRM_FORMAT_XRGB1555 | * DRM_FORMAT_BGRX1555 |
> + * ||| * DRM_FORMAT_RGBX1555 |
> + * ||| * DRM_FORMAT_XBGR1555 |
> + * ++++
> + * | 8 | * DRM_FORMAT_C8 | * DRM_FORMAT_D8 |
> + * ||| * DRM_FORMAT_R8 |
> + * ++++
> + * | 4 | * DRM_FORMAT_C4 | * DRM_FORMAT_D4 |
> + * | | | * DRM_FORMAT_R4 |
> + * ++++
> + * | 2 | * DRM_FORMAT_C2 | * DRM_FORMAT_D2 |
> + * ||| * DRM_FORMAT_R2 |
> + * ++++
> + * | 1 | * DRM_FORMAT_C1 | * DRM_FORMAT_D1 |
> + * ||| * DRM_FORMAT_R1 |
> + * ++++
> + *
> + * Color modes of 10, 12, 15, 30 and 64 are only supported for use by
> + * legacy user space. Please don't use them in new code. Other modes
> + * are not support.
> + *
> + * Do not attempt to allocate anything but linear framebuffer memory
> + * with single-plane RGB data. Allocation of other framebuffer
> + * layouts requires dedicated ioctls in the respective DRM driver.
> */
> struct drm_mode_create_dumb {
> __u32 height;
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v4 03/25] drm/gem-dma: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi,
On 11/03/2025 17:47, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch to a multiple of 8.
>
> Push the current calculation into the only direct caller imx. Imx's
> hardware requires the framebuffer width to be aligned to 8. The
> driver's current approach is actually incorrect, as it only guarantees
> this implicitly and requires bpp to be a multiple of 8 already. A
> later commit will fix this problem by aligning the scanline pitch
> such that an aligned width still fits into each scanline's memory.
>
> A number of other drivers are build on top of gem-dma helpers and
> implement their own dumb-buffer allocation. These drivers invoke
> drm_gem_dma_dumb_create_internal(), which is not affected by this
> commit.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_gem_dma_helper.c | 7 +--
> drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c
> b/drivers/gpu/drm/drm_gem_dma_helper.c
> index b7f033d4352a..49be9b033610 100644
> --- a/drivers/gpu/drm/drm_gem_dma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv,
> struct drm_mode_create_dumb *args)
> {
> struct drm_gem_dma_object *dma_obj;
> + int ret;
>
> - args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> - args->size = args->pitch * args->height;
> + ret = drm_mode_size_dumb(drm, args, SZ_8, 0);
> + if (ret)
> + return ret;
>
> dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size,
>&args->handle);
> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> index ec5fd9a01f1e..e7025df7b978 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> @@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
> int ret;
>
> args->width = ALIGN(width, 8);
> + args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> + args->size = args->pitch * args->height;
>
> ret = drm_gem_dma_dumb_create(file_priv, drm, args);
> if (ret)
Won't the pitch and size just be overwritten by the
drm_gem_dma_dumb_create() call?
Tomi
Re: [PATCH v4 01/25] drm/dumb-buffers: Sanitize output on errors
Hi,
On 11/03/2025 17:47, Thomas Zimmermann wrote:
> The ioctls MODE_CREATE_DUMB and MODE_MAP_DUMB return results into a
> memory buffer supplied by user space. On errors, it is possible that
> intermediate values are being returned. The exact semantics depends
> on the DRM driver's implementation of these ioctls. Although this is
> most-likely not a security problem in practice, avoid any uncertainty
> by clearing the memory to 0 on errors.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_dumb_buffers.c | 40 ++
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c
> b/drivers/gpu/drm/drm_dumb_buffers.c
> index 70032bba1c97..9916aaf5b3f2 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -99,7 +99,30 @@ int drm_mode_create_dumb(struct drm_device *dev,
> int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> - return drm_mode_create_dumb(dev, data, file_priv);
> + struct drm_mode_create_dumb *args = data;
> + int err;
> +
> + err = drm_mode_create_dumb(dev, args, file_priv);
> + if (err) {
> + args->handle = 0;
> + args->pitch = 0;
> + args->size = 0;
> + }
> + return err;
> +}
> +
> +static int drm_mode_mmap_dumb(struct drm_device *dev, struct
> drm_mode_map_dumb *args,
> + struct drm_file *file_priv)
> +{
> + if (!dev->driver->dumb_create)
> + return -ENOSYS;
> +
> + if (dev->driver->dumb_map_offset)
> + return dev->driver->dumb_map_offset(file_priv, dev,
> args->handle,
> + &args->offset);
> + else
> + return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> +&args->offset);
> }
>
> /**
> @@ -120,17 +143,12 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>void *data, struct drm_file *file_priv)
> {
> struct drm_mode_map_dumb *args = data;
> + int err;
>
> - if (!dev->driver->dumb_create)
> - return -ENOSYS;
> -
> - if (dev->driver->dumb_map_offset)
> - return dev->driver->dumb_map_offset(file_priv, dev,
> - args->handle,
> - &args->offset);
> - else
> - return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> - &args->offset);
> + err = drm_mode_mmap_dumb(dev, args, file_priv);
> + if (err)
> + args->offset = 0;
> + return err;
> }
>
> int drm_mode_destroy_dumb(struct drm_device *dev, u32 handle,
Reviewed-by: Tomi Valkeinen
Tomi
