[PATCH 0/3] drm: Fix initializing format info in drm_framebuffer

[PATCH 0/3] drm: Fix initializing format info in drm_framebuffer

[Imre Deak]

This patchset fixes initializing the format info in drm_framebuffer by
drm_helper_mode_fill_fb_struct() in the omap, nouveau, radeon drivers
after patch [1] in patchset [2], the latter one missing the conversion
of these 3 drivers. This patchset converts the 3 drivers similarly to
the other drivers converted by [2].

This patchset was only compile tested.

[1] https://lore.kernel.org/all/20250701090722.13645-20-ville.syrjala@linux.intel.com
[2] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: amd-gfx@lists.freedesktop.org

Imre Deak (3):
  drm/omap: Pass along the format info from .fb_create() to
    drm_helper_mode_fill_fb_struct()
  drm/nouveau: Pass along the format info from .fb_create() to
    drm_helper_mode_fill_fb_struct()
  drm/radeon: Pass along the format info from .fb_create() to
    drm_helper_mode_fill_fb_struct()

 drivers/gpu/drm/nouveau/nouveau_display.c |  9 +++------
 drivers/gpu/drm/nouveau/nouveau_display.h |  3 +++
 drivers/gpu/drm/omapdrm/omap_fb.c         | 23 ++++++++++-------------
 drivers/gpu/drm/omapdrm/omap_fb.h         |  2 ++
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  5 ++++-
 drivers/gpu/drm/radeon/radeon_display.c   |  5 +++--
 drivers/gpu/drm/radeon/radeon_fbdev.c     | 11 ++++++-----
 drivers/gpu/drm/radeon/radeon_mode.h      |  2 ++
 8 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.49.1

[PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Plumb the format info from .fb_create() all the way to
drm_helper_mode_fill_fb_struct() to avoid the redundant
lookup.

For the fbdev case a manual drm_get_format_info() lookup
is needed.

The patch is based on the driver parts of the patchset at Link:
below, which missed converting the omap driver.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
Reported-by: Mark Brown <broonie@kernel.org>
Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
 drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
 drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 30c81e2e5d6b3..bb3105556f193 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 		}
 	}
 
-	fb = omap_framebuffer_init(dev, mode_cmd, bos);
+	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
 	if (IS_ERR(fb))
 		goto error;
 
@@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 }
 
 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
+		const struct drm_format_info *info,
 		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
 {
-	const struct drm_format_info *format = NULL;
 	struct omap_framebuffer *omap_fb = NULL;
 	struct drm_framebuffer *fb = NULL;
 	unsigned int pitch = mode_cmd->pitches[0];
@@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
 			(char *)&mode_cmd->pixel_format);
 
-	format = drm_get_format_info(dev, mode_cmd->pixel_format,
-				     mode_cmd->modifier[0]);
-
 	for (i = 0; i < ARRAY_SIZE(formats); i++) {
 		if (formats[i] == mode_cmd->pixel_format)
 			break;
 	}
 
-	if (!format || i == ARRAY_SIZE(formats)) {
+	if (i == ARRAY_SIZE(formats)) {
 		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
 			(char *)&mode_cmd->pixel_format);
 		ret = -EINVAL;
@@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	}
 
 	fb = &omap_fb->base;
-	omap_fb->format = format;
+	omap_fb->format = info;
 	mutex_init(&omap_fb->lock);
 
 	/*
@@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	 * that the two planes of multiplane formats need the same number of
 	 * bytes per pixel.
 	 */
-	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
+	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
 		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
 		ret = -EINVAL;
 		goto fail;
 	}
 
-	if (pitch % format->cpp[0]) {
+	if (pitch % info->cpp[0]) {
 		dev_dbg(dev->dev,
 			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
-			pitch, format->cpp[0]);
+			pitch, info->cpp[0]);
 		ret = -EINVAL;
 		goto fail;
 	}
 
-	for (i = 0; i < format->num_planes; i++) {
+	for (i = 0; i < info->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		unsigned int vsub = i == 0 ? 1 : format->vsub;
+		unsigned int vsub = i == 0 ? 1 : info->vsub;
 		unsigned int size;
 
 		size = pitch * mode_cmd->height / vsub;
@@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		plane->dma_addr  = 0;
 	}
 
-	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
+	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
 
 	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
 	if (ret) {
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
index 0873f953cf1d1..e6010302a22bd 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.h
+++ b/drivers/gpu/drm/omapdrm/omap_fb.h
@@ -13,6 +13,7 @@ struct drm_connector;
 struct drm_device;
 struct drm_file;
 struct drm_framebuffer;
+struct drm_format_info;
 struct drm_gem_object;
 struct drm_mode_fb_cmd2;
 struct drm_plane_state;
@@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 		struct drm_file *file, const struct drm_format_info *info,
 		const struct drm_mode_fb_cmd2 *mode_cmd);
 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
+		const struct drm_format_info *info,
 		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
 int omap_framebuffer_pin(struct drm_framebuffer *fb);
 void omap_framebuffer_unpin(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 7b63968906817..948af7ec1130b 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 		goto fail;
 	}
 
-	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
+	fb = omap_framebuffer_init(dev,
+				   drm_get_format_info(dev, mode_cmd.pixel_format,
+						       mode_cmd.modifier[0]),
+				   &mode_cmd, &bo);
 	if (IS_ERR(fb)) {
 		dev_err(dev->dev, "failed to allocate fb\n");
 		/* note: if fb creation failed, we can't rely on fb destroy
-- 
2.49.1

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

On Mon, Jul 28, 2025 at 01:16:01PM +0300, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.

Tested-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Hi Tomi, Maarten, Maxime, Thomas, Dave, Simona,

could you please ack this patch? Note that it fixes commit 41ab92d35ccd
("drm: Make passing of format info to drm_helper_mode_fill_fb_struct()
mandatory") which is already part of Dave's 6.17-rc1 pull request at:

https://lore.kernel.org/all/CAPM=9tzVm80-v6_5nt6kko3nR+aQLZ7R98i419FV8f4-ayQWUw@mail.gmail.com

Alex already acked the patchset, but patch 1 and 2 are still missing an
ack/reviewed-by from maintainers.

Thanks,
Imre

On Mon, Jul 28, 2025 at 01:16:01PM +0300, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.
> 
> For the fbdev case a manual drm_get_format_info() lookup
> is needed.
> 
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the omap driver.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 30c81e2e5d6b3..bb3105556f193 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  		}
>  	}
>  
> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
>  	if (IS_ERR(fb))
>  		goto error;
>  
> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  }
>  
>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> +		const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
>  {
> -	const struct drm_format_info *format = NULL;
>  	struct omap_framebuffer *omap_fb = NULL;
>  	struct drm_framebuffer *fb = NULL;
>  	unsigned int pitch = mode_cmd->pitches[0];
> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
>  			(char *)&mode_cmd->pixel_format);
>  
> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
> -				     mode_cmd->modifier[0]);
> -
>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>  		if (formats[i] == mode_cmd->pixel_format)
>  			break;
>  	}
>  
> -	if (!format || i == ARRAY_SIZE(formats)) {
> +	if (i == ARRAY_SIZE(formats)) {
>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
>  			(char *)&mode_cmd->pixel_format);
>  		ret = -EINVAL;
> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	}
>  
>  	fb = &omap_fb->base;
> -	omap_fb->format = format;
> +	omap_fb->format = info;
>  	mutex_init(&omap_fb->lock);
>  
>  	/*
> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	 * that the two planes of multiplane formats need the same number of
>  	 * bytes per pixel.
>  	 */
> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
>  		ret = -EINVAL;
>  		goto fail;
>  	}
>  
> -	if (pitch % format->cpp[0]) {
> +	if (pitch % info->cpp[0]) {
>  		dev_dbg(dev->dev,
>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> -			pitch, format->cpp[0]);
> +			pitch, info->cpp[0]);
>  		ret = -EINVAL;
>  		goto fail;
>  	}
>  
> -	for (i = 0; i < format->num_planes; i++) {
> +	for (i = 0; i < info->num_planes; i++) {
>  		struct plane *plane = &omap_fb->planes[i];
> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
>  		unsigned int size;
>  
>  		size = pitch * mode_cmd->height / vsub;
> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  		plane->dma_addr  = 0;
>  	}
>  
> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>  
>  	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> index 0873f953cf1d1..e6010302a22bd 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> @@ -13,6 +13,7 @@ struct drm_connector;
>  struct drm_device;
>  struct drm_file;
>  struct drm_framebuffer;
> +struct drm_format_info;
>  struct drm_gem_object;
>  struct drm_mode_fb_cmd2;
>  struct drm_plane_state;
> @@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  		struct drm_file *file, const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd);
>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> +		const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index 7b63968906817..948af7ec1130b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  		goto fail;
>  	}
>  
> -	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
> +	fb = omap_framebuffer_init(dev,
> +				   drm_get_format_info(dev, mode_cmd.pixel_format,
> +						       mode_cmd.modifier[0]),
> +				   &mode_cmd, &bo);
>  	if (IS_ERR(fb)) {
>  		dev_err(dev->dev, "failed to allocate fb\n");
>  		/* note: if fb creation failed, we can't rely on fb destroy
> -- 
> 2.49.1
> 

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Tomi Valkeinen]

Hi Imre,

On 28/07/2025 13:16, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.
> 
> For the fbdev case a manual drm_get_format_info() lookup
> is needed.
> 
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the omap driver.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 30c81e2e5d6b3..bb3105556f193 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  		}
>  	}
>  
> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
>  	if (IS_ERR(fb))
>  		goto error;
>  
> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  }
>  
>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> +		const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
>  {
> -	const struct drm_format_info *format = NULL;
>  	struct omap_framebuffer *omap_fb = NULL;
>  	struct drm_framebuffer *fb = NULL;
>  	unsigned int pitch = mode_cmd->pitches[0];
> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
>  			(char *)&mode_cmd->pixel_format);
>  
> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
> -				     mode_cmd->modifier[0]);
> -
>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>  		if (formats[i] == mode_cmd->pixel_format)
>  			break;
>  	}
>  
> -	if (!format || i == ARRAY_SIZE(formats)) {
> +	if (i == ARRAY_SIZE(formats)) {
>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
>  			(char *)&mode_cmd->pixel_format);
>  		ret = -EINVAL;
> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	}
>  
>  	fb = &omap_fb->base;
> -	omap_fb->format = format;
> +	omap_fb->format = info;
>  	mutex_init(&omap_fb->lock);
>  
>  	/*
> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	 * that the two planes of multiplane formats need the same number of
>  	 * bytes per pixel.
>  	 */
> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
>  		ret = -EINVAL;
>  		goto fail;
>  	}
>  
> -	if (pitch % format->cpp[0]) {
> +	if (pitch % info->cpp[0]) {
>  		dev_dbg(dev->dev,
>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> -			pitch, format->cpp[0]);
> +			pitch, info->cpp[0]);
>  		ret = -EINVAL;
>  		goto fail;
>  	}
>  
> -	for (i = 0; i < format->num_planes; i++) {
> +	for (i = 0; i < info->num_planes; i++) {
>  		struct plane *plane = &omap_fb->planes[i];
> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
>  		unsigned int size;
>  
>  		size = pitch * mode_cmd->height / vsub;
> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  		plane->dma_addr  = 0;
>  	}
>  
> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);

Isn't the fix really a one-liner, just passing "format" here instead of
NULL?

 Tomi

>  
>  	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> index 0873f953cf1d1..e6010302a22bd 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> @@ -13,6 +13,7 @@ struct drm_connector;
>  struct drm_device;
>  struct drm_file;
>  struct drm_framebuffer;
> +struct drm_format_info;
>  struct drm_gem_object;
>  struct drm_mode_fb_cmd2;
>  struct drm_plane_state;
> @@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  		struct drm_file *file, const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd);
>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> +		const struct drm_format_info *info,
>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index 7b63968906817..948af7ec1130b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  		goto fail;
>  	}
>  
> -	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
> +	fb = omap_framebuffer_init(dev,
> +				   drm_get_format_info(dev, mode_cmd.pixel_format,
> +						       mode_cmd.modifier[0]),
> +				   &mode_cmd, &bo);
>  	if (IS_ERR(fb)) {
>  		dev_err(dev->dev, "failed to allocate fb\n");
>  		/* note: if fb creation failed, we can't rely on fb destroy

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Hi Tomi,

On Tue, Aug 05, 2025 at 02:53:36PM +0300, Tomi Valkeinen wrote:
> Hi Imre,
> 
> On 28/07/2025 13:16, Imre Deak wrote:
> > Plumb the format info from .fb_create() all the way to
> > drm_helper_mode_fill_fb_struct() to avoid the redundant
> > lookup.
> > 
> > For the fbdev case a manual drm_get_format_info() lookup
> > is needed.
> > 
> > The patch is based on the driver parts of the patchset at Link:
> > below, which missed converting the omap driver.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> > Reported-by: Mark Brown <broonie@kernel.org>
> > Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
> > Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
> >  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
> >  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
> >  3 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> > index 30c81e2e5d6b3..bb3105556f193 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> > @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >  		}
> >  	}
> >  
> > -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
> > +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
> >  	if (IS_ERR(fb))
> >  		goto error;
> >  
> > @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >  }
> >  
> >  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> > +		const struct drm_format_info *info,
> >  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
> >  {
> > -	const struct drm_format_info *format = NULL;
> >  	struct omap_framebuffer *omap_fb = NULL;
> >  	struct drm_framebuffer *fb = NULL;
> >  	unsigned int pitch = mode_cmd->pitches[0];
> > @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
> >  			(char *)&mode_cmd->pixel_format);
> >  
> > -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
> > -				     mode_cmd->modifier[0]);
> > -
> >  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> >  		if (formats[i] == mode_cmd->pixel_format)
> >  			break;
> >  	}
> >  
> > -	if (!format || i == ARRAY_SIZE(formats)) {
> > +	if (i == ARRAY_SIZE(formats)) {
> >  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
> >  			(char *)&mode_cmd->pixel_format);
> >  		ret = -EINVAL;
> > @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >  	}
> >  
> >  	fb = &omap_fb->base;
> > -	omap_fb->format = format;
> > +	omap_fb->format = info;
> >  	mutex_init(&omap_fb->lock);
> >  
> >  	/*
> > @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >  	 * that the two planes of multiplane formats need the same number of
> >  	 * bytes per pixel.
> >  	 */
> > -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> > +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> >  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
> >  		ret = -EINVAL;
> >  		goto fail;
> >  	}
> >  
> > -	if (pitch % format->cpp[0]) {
> > +	if (pitch % info->cpp[0]) {
> >  		dev_dbg(dev->dev,
> >  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> > -			pitch, format->cpp[0]);
> > +			pitch, info->cpp[0]);
> >  		ret = -EINVAL;
> >  		goto fail;
> >  	}
> >  
> > -	for (i = 0; i < format->num_planes; i++) {
> > +	for (i = 0; i < info->num_planes; i++) {
> >  		struct plane *plane = &omap_fb->planes[i];
> > -		unsigned int vsub = i == 0 ? 1 : format->vsub;
> > +		unsigned int vsub = i == 0 ? 1 : info->vsub;
> >  		unsigned int size;
> >  
> >  		size = pitch * mode_cmd->height / vsub;
> > @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >  		plane->dma_addr  = 0;
> >  	}
> >  
> > -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> > +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
> 
> Isn't the fix really a one-liner, just passing "format" here instead of
> NULL?

That would fix the issue of fb->format being initialized to NULL yes.

However, I think the change should be in sync with the conversion of the
rest of the drivers in patchset [1]. IOW, what this patch is meant to
fix is that [1] missed converting the OMAP driver similarly to the other
drivers.

I think the change is equivalent to the above one-liner you suggest with
the following differences:

- omap_framebuffer_init() uses the drm_format_info passed down from either
  drm_internal_framebuffer_create(), or omap_fbdev_driver_fbdev_probe().
  In both cases the passed down format info matches what
  omap_framebuffer_init() would look up again.

- Skip the format == NULL check. format can't be NULL in any case, since
  that would have emitted a WARN already in drm_get_format_info() ->
  drm_format_info().

[1] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com

>  Tomi
> 
> >  
> >  	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
> >  	if (ret) {
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> > index 0873f953cf1d1..e6010302a22bd 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> > @@ -13,6 +13,7 @@ struct drm_connector;
> >  struct drm_device;
> >  struct drm_file;
> >  struct drm_framebuffer;
> > +struct drm_format_info;
> >  struct drm_gem_object;
> >  struct drm_mode_fb_cmd2;
> >  struct drm_plane_state;
> > @@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >  		struct drm_file *file, const struct drm_format_info *info,
> >  		const struct drm_mode_fb_cmd2 *mode_cmd);
> >  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> > +		const struct drm_format_info *info,
> >  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
> >  int omap_framebuffer_pin(struct drm_framebuffer *fb);
> >  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > index 7b63968906817..948af7ec1130b 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > @@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
> >  		goto fail;
> >  	}
> >  
> > -	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
> > +	fb = omap_framebuffer_init(dev,
> > +				   drm_get_format_info(dev, mode_cmd.pixel_format,
> > +						       mode_cmd.modifier[0]),
> > +				   &mode_cmd, &bo);
> >  	if (IS_ERR(fb)) {
> >  		dev_err(dev->dev, "failed to allocate fb\n");
> >  		/* note: if fb creation failed, we can't rely on fb destroy
> 

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Tomi Valkeinen]

Hi,

On 05/08/2025 17:49, Imre Deak wrote:
> Hi Tomi,
> 
> On Tue, Aug 05, 2025 at 02:53:36PM +0300, Tomi Valkeinen wrote:
>> Hi Imre,
>>
>> On 28/07/2025 13:16, Imre Deak wrote:
>>> Plumb the format info from .fb_create() all the way to
>>> drm_helper_mode_fill_fb_struct() to avoid the redundant
>>> lookup.
>>>
>>> For the fbdev case a manual drm_get_format_info() lookup
>>> is needed.
>>>
>>> The patch is based on the driver parts of the patchset at Link:
>>> below, which missed converting the omap driver.
>>>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
>>> Reported-by: Mark Brown <broonie@kernel.org>
>>> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
>>> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
>>>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
>>>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
>>>  3 files changed, 16 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
>>> index 30c81e2e5d6b3..bb3105556f193 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
>>> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>>>  		}
>>>  	}
>>>  
>>> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
>>> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
>>>  	if (IS_ERR(fb))
>>>  		goto error;
>>>  
>>> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>>>  }
>>>  
>>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>> +		const struct drm_format_info *info,
>>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
>>>  {
>>> -	const struct drm_format_info *format = NULL;
>>>  	struct omap_framebuffer *omap_fb = NULL;
>>>  	struct drm_framebuffer *fb = NULL;
>>>  	unsigned int pitch = mode_cmd->pitches[0];
>>> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
>>>  			(char *)&mode_cmd->pixel_format);
>>>  
>>> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
>>> -				     mode_cmd->modifier[0]);
>>> -
>>>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>>>  		if (formats[i] == mode_cmd->pixel_format)
>>>  			break;
>>>  	}
>>>  
>>> -	if (!format || i == ARRAY_SIZE(formats)) {
>>> +	if (i == ARRAY_SIZE(formats)) {
>>>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
>>>  			(char *)&mode_cmd->pixel_format);
>>>  		ret = -EINVAL;
>>> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>  	}
>>>  
>>>  	fb = &omap_fb->base;
>>> -	omap_fb->format = format;
>>> +	omap_fb->format = info;
>>>  	mutex_init(&omap_fb->lock);
>>>  
>>>  	/*
>>> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>  	 * that the two planes of multiplane formats need the same number of
>>>  	 * bytes per pixel.
>>>  	 */
>>> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>>> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>>>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
>>>  		ret = -EINVAL;
>>>  		goto fail;
>>>  	}
>>>  
>>> -	if (pitch % format->cpp[0]) {
>>> +	if (pitch % info->cpp[0]) {
>>>  		dev_dbg(dev->dev,
>>>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
>>> -			pitch, format->cpp[0]);
>>> +			pitch, info->cpp[0]);
>>>  		ret = -EINVAL;
>>>  		goto fail;
>>>  	}
>>>  
>>> -	for (i = 0; i < format->num_planes; i++) {
>>> +	for (i = 0; i < info->num_planes; i++) {
>>>  		struct plane *plane = &omap_fb->planes[i];
>>> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
>>> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
>>>  		unsigned int size;
>>>  
>>>  		size = pitch * mode_cmd->height / vsub;
>>> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>  		plane->dma_addr  = 0;
>>>  	}
>>>  
>>> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
>>> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>>
>> Isn't the fix really a one-liner, just passing "format" here instead of
>> NULL?
> 
> That would fix the issue of fb->format being initialized to NULL yes.
> 
> However, I think the change should be in sync with the conversion of the
> rest of the drivers in patchset [1]. IOW, what this patch is meant to
> fix is that [1] missed converting the OMAP driver similarly to the other
> drivers.
> 
> I think the change is equivalent to the above one-liner you suggest with
> the following differences:
> 
> - omap_framebuffer_init() uses the drm_format_info passed down from either
>   drm_internal_framebuffer_create(), or omap_fbdev_driver_fbdev_probe().
>   In both cases the passed down format info matches what
>   omap_framebuffer_init() would look up again.
> 
> - Skip the format == NULL check. format can't be NULL in any case, since
>   that would have emitted a WARN already in drm_get_format_info() ->
>   drm_format_info().
> 
> [1] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com

Yep, I have no issue with the patch as such. But if it's a fix, going
into an rc, I think it's better if it's as small as possible, and do the
cleanups/refactorings as a non-fix later.

The patch description here sounds more like it's just refactoring the
code a bit, but if I understand correctly, it's fixing an issue that
cause a WARN?

So... Either we could 1) split the patch, have the WARN fix as a fix
patch to the current rc, and the rest later in the next merge window, or
2) if you want to keep this as a single patch, I think it makes sense to
change the subject and description to highlight the fix aspect.

I think my comments apply to all patches in this series, as they're
essentially doing the same thing...

 Tomi

>>  Tomi
>>
>>>  
>>>  	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
>>>  	if (ret) {
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
>>> index 0873f953cf1d1..e6010302a22bd 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
>>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
>>> @@ -13,6 +13,7 @@ struct drm_connector;
>>>  struct drm_device;
>>>  struct drm_file;
>>>  struct drm_framebuffer;
>>> +struct drm_format_info;
>>>  struct drm_gem_object;
>>>  struct drm_mode_fb_cmd2;
>>>  struct drm_plane_state;
>>> @@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>>>  		struct drm_file *file, const struct drm_format_info *info,
>>>  		const struct drm_mode_fb_cmd2 *mode_cmd);
>>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>> +		const struct drm_format_info *info,
>>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
>>>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
>>>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>>> index 7b63968906817..948af7ec1130b 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>>> @@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>>>  		goto fail;
>>>  	}
>>>  
>>> -	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
>>> +	fb = omap_framebuffer_init(dev,
>>> +				   drm_get_format_info(dev, mode_cmd.pixel_format,
>>> +						       mode_cmd.modifier[0]),
>>> +				   &mode_cmd, &bo);
>>>  	if (IS_ERR(fb)) {
>>>  		dev_err(dev->dev, "failed to allocate fb\n");
>>>  		/* note: if fb creation failed, we can't rely on fb destroy
>>

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

On Tue, Aug 05, 2025 at 06:43:04PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 05/08/2025 17:49, Imre Deak wrote:
> > Hi Tomi,
> > 
> > On Tue, Aug 05, 2025 at 02:53:36PM +0300, Tomi Valkeinen wrote:
> >> Hi Imre,
> >>
> >> On 28/07/2025 13:16, Imre Deak wrote:
> >>> Plumb the format info from .fb_create() all the way to
> >>> drm_helper_mode_fill_fb_struct() to avoid the redundant
> >>> lookup.
> >>>
> >>> For the fbdev case a manual drm_get_format_info() lookup
> >>> is needed.
> >>>
> >>> The patch is based on the driver parts of the patchset at Link:
> >>> below, which missed converting the omap driver.
> >>>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: Maxime Ripard <mripard@kernel.org>
> >>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> >>> Reported-by: Mark Brown <broonie@kernel.org>
> >>> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
> >>> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> >>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
> >>>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
> >>>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
> >>>  3 files changed, 16 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> >>> index 30c81e2e5d6b3..bb3105556f193 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> >>> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >>>  		}
> >>>  	}
> >>>  
> >>> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
> >>> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
> >>>  	if (IS_ERR(fb))
> >>>  		goto error;
> >>>  
> >>> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >>>  }
> >>>  
> >>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>> +		const struct drm_format_info *info,
> >>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
> >>>  {
> >>> -	const struct drm_format_info *format = NULL;
> >>>  	struct omap_framebuffer *omap_fb = NULL;
> >>>  	struct drm_framebuffer *fb = NULL;
> >>>  	unsigned int pitch = mode_cmd->pitches[0];
> >>> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
> >>>  			(char *)&mode_cmd->pixel_format);
> >>>  
> >>> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
> >>> -				     mode_cmd->modifier[0]);
> >>> -
> >>>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> >>>  		if (formats[i] == mode_cmd->pixel_format)
> >>>  			break;
> >>>  	}
> >>>  
> >>> -	if (!format || i == ARRAY_SIZE(formats)) {
> >>> +	if (i == ARRAY_SIZE(formats)) {
> >>>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
> >>>  			(char *)&mode_cmd->pixel_format);
> >>>  		ret = -EINVAL;
> >>> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>  	}
> >>>  
> >>>  	fb = &omap_fb->base;
> >>> -	omap_fb->format = format;
> >>> +	omap_fb->format = info;
> >>>  	mutex_init(&omap_fb->lock);
> >>>  
> >>>  	/*
> >>> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>  	 * that the two planes of multiplane formats need the same number of
> >>>  	 * bytes per pixel.
> >>>  	 */
> >>> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> >>> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> >>>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
> >>>  		ret = -EINVAL;
> >>>  		goto fail;
> >>>  	}
> >>>  
> >>> -	if (pitch % format->cpp[0]) {
> >>> +	if (pitch % info->cpp[0]) {
> >>>  		dev_dbg(dev->dev,
> >>>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> >>> -			pitch, format->cpp[0]);
> >>> +			pitch, info->cpp[0]);
> >>>  		ret = -EINVAL;
> >>>  		goto fail;
> >>>  	}
> >>>  
> >>> -	for (i = 0; i < format->num_planes; i++) {
> >>> +	for (i = 0; i < info->num_planes; i++) {
> >>>  		struct plane *plane = &omap_fb->planes[i];
> >>> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
> >>> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
> >>>  		unsigned int size;
> >>>  
> >>>  		size = pitch * mode_cmd->height / vsub;
> >>> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>  		plane->dma_addr  = 0;
> >>>  	}
> >>>  
> >>> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> >>> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
> >>
> >> Isn't the fix really a one-liner, just passing "format" here instead of
> >> NULL?
> > 
> > That would fix the issue of fb->format being initialized to NULL yes.
> > 
> > However, I think the change should be in sync with the conversion of the
> > rest of the drivers in patchset [1]. IOW, what this patch is meant to
> > fix is that [1] missed converting the OMAP driver similarly to the other
> > drivers.
> > 
> > I think the change is equivalent to the above one-liner you suggest with
> > the following differences:
> > 
> > - omap_framebuffer_init() uses the drm_format_info passed down from either
> >   drm_internal_framebuffer_create(), or omap_fbdev_driver_fbdev_probe().
> >   In both cases the passed down format info matches what
> >   omap_framebuffer_init() would look up again.
> > 
> > - Skip the format == NULL check. format can't be NULL in any case, since
> >   that would have emitted a WARN already in drm_get_format_info() ->
> >   drm_format_info().
> > 
> > [1] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> 
> Yep, I have no issue with the patch as such. But if it's a fix, going
> into an rc, I think it's better if it's as small as possible, and do the
> cleanups/refactorings as a non-fix later.
> 
> The patch description here sounds more like it's just refactoring the
> code a bit, but if I understand correctly, it's fixing an issue that
> cause a WARN?

Yes, the patch description should've mentioned that it fixes the
!fb->format WARN in drm_framebuffer_init() and the resulting failure to
create the framebuffer for fbdev and other FB users. I will add this.

> So... Either we could 1) split the patch, have the WARN fix as a fix
> patch to the current rc, and the rest later in the next merge window, or
> 2) if you want to keep this as a single patch, I think it makes sense to
> change the subject and description to highlight the fix aspect.

Yes, the patch should go to 6.17-rc1, but I would prefer 2) above, since
patchset [1] requiring it was also added in the same -rc1 and this patch
has been also tested at least by the bug reporter.

> I think my comments apply to all patches in this series, as they're
> essentially doing the same thing...

I can also amend the commit log for the other patches according to the
above.

>  Tomi
> 
> >>  Tomi
> >>
> >>>  
> >>>  	ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
> >>>  	if (ret) {
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> >>> index 0873f953cf1d1..e6010302a22bd 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> >>> @@ -13,6 +13,7 @@ struct drm_connector;
> >>>  struct drm_device;
> >>>  struct drm_file;
> >>>  struct drm_framebuffer;
> >>> +struct drm_format_info;
> >>>  struct drm_gem_object;
> >>>  struct drm_mode_fb_cmd2;
> >>>  struct drm_plane_state;
> >>> @@ -23,6 +24,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >>>  		struct drm_file *file, const struct drm_format_info *info,
> >>>  		const struct drm_mode_fb_cmd2 *mode_cmd);
> >>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>> +		const struct drm_format_info *info,
> >>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
> >>>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
> >>>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> >>> index 7b63968906817..948af7ec1130b 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> >>> @@ -197,7 +197,10 @@ int omap_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
> >>>  		goto fail;
> >>>  	}
> >>>  
> >>> -	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
> >>> +	fb = omap_framebuffer_init(dev,
> >>> +				   drm_get_format_info(dev, mode_cmd.pixel_format,
> >>> +						       mode_cmd.modifier[0]),
> >>> +				   &mode_cmd, &bo);
> >>>  	if (IS_ERR(fb)) {
> >>>  		dev_err(dev->dev, "failed to allocate fb\n");
> >>>  		/* note: if fb creation failed, we can't rely on fb destroy
> >>
> 

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Tomi Valkeinen]

Hi,

On 05/08/2025 19:22, Imre Deak wrote:
> On Tue, Aug 05, 2025 at 06:43:04PM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 05/08/2025 17:49, Imre Deak wrote:
>>> Hi Tomi,
>>>
>>> On Tue, Aug 05, 2025 at 02:53:36PM +0300, Tomi Valkeinen wrote:
>>>> Hi Imre,
>>>>
>>>> On 28/07/2025 13:16, Imre Deak wrote:
>>>>> Plumb the format info from .fb_create() all the way to
>>>>> drm_helper_mode_fill_fb_struct() to avoid the redundant
>>>>> lookup.
>>>>>
>>>>> For the fbdev case a manual drm_get_format_info() lookup
>>>>> is needed.
>>>>>
>>>>> The patch is based on the driver parts of the patchset at Link:
>>>>> below, which missed converting the omap driver.
>>>>>
>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
>>>>> Reported-by: Mark Brown <broonie@kernel.org>
>>>>> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
>>>>> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
>>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
>>>>>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
>>>>>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
>>>>>  3 files changed, 16 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
>>>>> index 30c81e2e5d6b3..bb3105556f193 100644
>>>>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
>>>>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
>>>>> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>>>>>  		}
>>>>>  	}
>>>>>  
>>>>> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
>>>>> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
>>>>>  	if (IS_ERR(fb))
>>>>>  		goto error;
>>>>>  
>>>>> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>>>>>  }
>>>>>  
>>>>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>>> +		const struct drm_format_info *info,
>>>>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
>>>>>  {
>>>>> -	const struct drm_format_info *format = NULL;
>>>>>  	struct omap_framebuffer *omap_fb = NULL;
>>>>>  	struct drm_framebuffer *fb = NULL;
>>>>>  	unsigned int pitch = mode_cmd->pitches[0];
>>>>> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>>>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
>>>>>  			(char *)&mode_cmd->pixel_format);
>>>>>  
>>>>> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
>>>>> -				     mode_cmd->modifier[0]);
>>>>> -
>>>>>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>>>>>  		if (formats[i] == mode_cmd->pixel_format)
>>>>>  			break;
>>>>>  	}
>>>>>  
>>>>> -	if (!format || i == ARRAY_SIZE(formats)) {
>>>>> +	if (i == ARRAY_SIZE(formats)) {
>>>>>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
>>>>>  			(char *)&mode_cmd->pixel_format);
>>>>>  		ret = -EINVAL;
>>>>> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>>>  	}
>>>>>  
>>>>>  	fb = &omap_fb->base;
>>>>> -	omap_fb->format = format;
>>>>> +	omap_fb->format = info;
>>>>>  	mutex_init(&omap_fb->lock);
>>>>>  
>>>>>  	/*
>>>>> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>>>  	 * that the two planes of multiplane formats need the same number of
>>>>>  	 * bytes per pixel.
>>>>>  	 */
>>>>> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>>>>> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
>>>>>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
>>>>>  		ret = -EINVAL;
>>>>>  		goto fail;
>>>>>  	}
>>>>>  
>>>>> -	if (pitch % format->cpp[0]) {
>>>>> +	if (pitch % info->cpp[0]) {
>>>>>  		dev_dbg(dev->dev,
>>>>>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
>>>>> -			pitch, format->cpp[0]);
>>>>> +			pitch, info->cpp[0]);
>>>>>  		ret = -EINVAL;
>>>>>  		goto fail;
>>>>>  	}
>>>>>  
>>>>> -	for (i = 0; i < format->num_planes; i++) {
>>>>> +	for (i = 0; i < info->num_planes; i++) {
>>>>>  		struct plane *plane = &omap_fb->planes[i];
>>>>> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
>>>>> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
>>>>>  		unsigned int size;
>>>>>  
>>>>>  		size = pitch * mode_cmd->height / vsub;
>>>>> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>>>  		plane->dma_addr  = 0;
>>>>>  	}
>>>>>  
>>>>> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
>>>>> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>>>>
>>>> Isn't the fix really a one-liner, just passing "format" here instead of
>>>> NULL?
>>>
>>> That would fix the issue of fb->format being initialized to NULL yes.
>>>
>>> However, I think the change should be in sync with the conversion of the
>>> rest of the drivers in patchset [1]. IOW, what this patch is meant to
>>> fix is that [1] missed converting the OMAP driver similarly to the other
>>> drivers.
>>>
>>> I think the change is equivalent to the above one-liner you suggest with
>>> the following differences:
>>>
>>> - omap_framebuffer_init() uses the drm_format_info passed down from either
>>>   drm_internal_framebuffer_create(), or omap_fbdev_driver_fbdev_probe().
>>>   In both cases the passed down format info matches what
>>>   omap_framebuffer_init() would look up again.
>>>
>>> - Skip the format == NULL check. format can't be NULL in any case, since
>>>   that would have emitted a WARN already in drm_get_format_info() ->
>>>   drm_format_info().
>>>
>>> [1] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
>>
>> Yep, I have no issue with the patch as such. But if it's a fix, going
>> into an rc, I think it's better if it's as small as possible, and do the
>> cleanups/refactorings as a non-fix later.
>>
>> The patch description here sounds more like it's just refactoring the
>> code a bit, but if I understand correctly, it's fixing an issue that
>> cause a WARN?
> 
> Yes, the patch description should've mentioned that it fixes the
> !fb->format WARN in drm_framebuffer_init() and the resulting failure to
> create the framebuffer for fbdev and other FB users. I will add this.
> 
>> So... Either we could 1) split the patch, have the WARN fix as a fix
>> patch to the current rc, and the rest later in the next merge window, or
>> 2) if you want to keep this as a single patch, I think it makes sense to
>> change the subject and description to highlight the fix aspect.
> 
> Yes, the patch should go to 6.17-rc1, but I would prefer 2) above, since
> patchset [1] requiring it was also added in the same -rc1 and this patch
> has been also tested at least by the bug reporter.
> 
>> I think my comments apply to all patches in this series, as they're
>> essentially doing the same thing...
> 
> I can also amend the commit log for the other patches according to the
> above.

Alright. In any case, I don't think any of my concerns are big, but I
wouldn't mind a clarification in the description. If you'll be pushing
all these together:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

If you want me to merge the omap one separately, just say so.

 Tomi

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

On Tue, Aug 05, 2025 at 07:56:50PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 05/08/2025 19:22, Imre Deak wrote:
> > On Tue, Aug 05, 2025 at 06:43:04PM +0300, Tomi Valkeinen wrote:
> >> Hi,
> >>
> >> On 05/08/2025 17:49, Imre Deak wrote:
> >>> Hi Tomi,
> >>>
> >>> On Tue, Aug 05, 2025 at 02:53:36PM +0300, Tomi Valkeinen wrote:
> >>>> Hi Imre,
> >>>>
> >>>> On 28/07/2025 13:16, Imre Deak wrote:
> >>>>> Plumb the format info from .fb_create() all the way to
> >>>>> drm_helper_mode_fill_fb_struct() to avoid the redundant
> >>>>> lookup.
> >>>>>
> >>>>> For the fbdev case a manual drm_get_format_info() lookup
> >>>>> is needed.
> >>>>>
> >>>>> The patch is based on the driver parts of the patchset at Link:
> >>>>> below, which missed converting the omap driver.
> >>>>>
> >>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Cc: Maxime Ripard <mripard@kernel.org>
> >>>>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> >>>>> Reported-by: Mark Brown <broonie@kernel.org>
> >>>>> Closes: https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f84180b@sirena.org.uk
> >>>>> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> >>>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/omapdrm/omap_fb.c    | 23 ++++++++++-------------
> >>>>>  drivers/gpu/drm/omapdrm/omap_fb.h    |  2 ++
> >>>>>  drivers/gpu/drm/omapdrm/omap_fbdev.c |  5 ++++-
> >>>>>  3 files changed, 16 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> >>>>> index 30c81e2e5d6b3..bb3105556f193 100644
> >>>>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> >>>>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> >>>>> @@ -351,7 +351,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >>>>>  		}
> >>>>>  	}
> >>>>>  
> >>>>> -	fb = omap_framebuffer_init(dev, mode_cmd, bos);
> >>>>> +	fb = omap_framebuffer_init(dev, info, mode_cmd, bos);
> >>>>>  	if (IS_ERR(fb))
> >>>>>  		goto error;
> >>>>>  
> >>>>> @@ -365,9 +365,9 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
> >>>>>  }
> >>>>>  
> >>>>>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>>> +		const struct drm_format_info *info,
> >>>>>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
> >>>>>  {
> >>>>> -	const struct drm_format_info *format = NULL;
> >>>>>  	struct omap_framebuffer *omap_fb = NULL;
> >>>>>  	struct drm_framebuffer *fb = NULL;
> >>>>>  	unsigned int pitch = mode_cmd->pitches[0];
> >>>>> @@ -377,15 +377,12 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>>>  			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
> >>>>>  			(char *)&mode_cmd->pixel_format);
> >>>>>  
> >>>>> -	format = drm_get_format_info(dev, mode_cmd->pixel_format,
> >>>>> -				     mode_cmd->modifier[0]);
> >>>>> -
> >>>>>  	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> >>>>>  		if (formats[i] == mode_cmd->pixel_format)
> >>>>>  			break;
> >>>>>  	}
> >>>>>  
> >>>>> -	if (!format || i == ARRAY_SIZE(formats)) {
> >>>>> +	if (i == ARRAY_SIZE(formats)) {
> >>>>>  		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
> >>>>>  			(char *)&mode_cmd->pixel_format);
> >>>>>  		ret = -EINVAL;
> >>>>> @@ -399,7 +396,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>>>  	}
> >>>>>  
> >>>>>  	fb = &omap_fb->base;
> >>>>> -	omap_fb->format = format;
> >>>>> +	omap_fb->format = info;
> >>>>>  	mutex_init(&omap_fb->lock);
> >>>>>  
> >>>>>  	/*
> >>>>> @@ -407,23 +404,23 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>>>  	 * that the two planes of multiplane formats need the same number of
> >>>>>  	 * bytes per pixel.
> >>>>>  	 */
> >>>>> -	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> >>>>> +	if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> >>>>>  		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
> >>>>>  		ret = -EINVAL;
> >>>>>  		goto fail;
> >>>>>  	}
> >>>>>  
> >>>>> -	if (pitch % format->cpp[0]) {
> >>>>> +	if (pitch % info->cpp[0]) {
> >>>>>  		dev_dbg(dev->dev,
> >>>>>  			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> >>>>> -			pitch, format->cpp[0]);
> >>>>> +			pitch, info->cpp[0]);
> >>>>>  		ret = -EINVAL;
> >>>>>  		goto fail;
> >>>>>  	}
> >>>>>  
> >>>>> -	for (i = 0; i < format->num_planes; i++) {
> >>>>> +	for (i = 0; i < info->num_planes; i++) {
> >>>>>  		struct plane *plane = &omap_fb->planes[i];
> >>>>> -		unsigned int vsub = i == 0 ? 1 : format->vsub;
> >>>>> +		unsigned int vsub = i == 0 ? 1 : info->vsub;
> >>>>>  		unsigned int size;
> >>>>>  
> >>>>>  		size = pitch * mode_cmd->height / vsub;
> >>>>> @@ -440,7 +437,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>>>>  		plane->dma_addr  = 0;
> >>>>>  	}
> >>>>>  
> >>>>> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> >>>>> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
> >>>>
> >>>> Isn't the fix really a one-liner, just passing "format" here instead of
> >>>> NULL?
> >>>
> >>> That would fix the issue of fb->format being initialized to NULL yes.
> >>>
> >>> However, I think the change should be in sync with the conversion of the
> >>> rest of the drivers in patchset [1]. IOW, what this patch is meant to
> >>> fix is that [1] missed converting the OMAP driver similarly to the other
> >>> drivers.
> >>>
> >>> I think the change is equivalent to the above one-liner you suggest with
> >>> the following differences:
> >>>
> >>> - omap_framebuffer_init() uses the drm_format_info passed down from either
> >>>   drm_internal_framebuffer_create(), or omap_fbdev_driver_fbdev_probe().
> >>>   In both cases the passed down format info matches what
> >>>   omap_framebuffer_init() would look up again.
> >>>
> >>> - Skip the format == NULL check. format can't be NULL in any case, since
> >>>   that would have emitted a WARN already in drm_get_format_info() ->
> >>>   drm_format_info().
> >>>
> >>> [1] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> >>
> >> Yep, I have no issue with the patch as such. But if it's a fix, going
> >> into an rc, I think it's better if it's as small as possible, and do the
> >> cleanups/refactorings as a non-fix later.
> >>
> >> The patch description here sounds more like it's just refactoring the
> >> code a bit, but if I understand correctly, it's fixing an issue that
> >> cause a WARN?
> > 
> > Yes, the patch description should've mentioned that it fixes the
> > !fb->format WARN in drm_framebuffer_init() and the resulting failure to
> > create the framebuffer for fbdev and other FB users. I will add this.
> > 
> >> So... Either we could 1) split the patch, have the WARN fix as a fix
> >> patch to the current rc, and the rest later in the next merge window, or
> >> 2) if you want to keep this as a single patch, I think it makes sense to
> >> change the subject and description to highlight the fix aspect.
> > 
> > Yes, the patch should go to 6.17-rc1, but I would prefer 2) above, since
> > patchset [1] requiring it was also added in the same -rc1 and this patch
> > has been also tested at least by the bug reporter.
> > 
> >> I think my comments apply to all patches in this series, as they're
> >> essentially doing the same thing...
> > 
> > I can also amend the commit log for the other patches according to the
> > above.
> 
> Alright. In any case, I don't think any of my concerns are big, but I
> wouldn't mind a clarification in the description. If you'll be pushing
> all these together:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks.

> If you want me to merge the omap one separately, just say so.

I sent now the updated [1], which can be merged to drm-misc-next-fixes,
but I'm not sure if there's still a pull request to v6.17-rc1 from that.
So waiting now a suggestion on that.

[1] https://lore.kernel.org/all/20250805175752.690504-1-imre.deak@intel.com

> 
>  Tomi
> 

Re: [PATCH 1/3] drm/omap: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Tue, Aug 05, 2025 at 06:43:04PM +0300, Tomi Valkeinen wrote:

> The patch description here sounds more like it's just refactoring the
> code a bit, but if I understand correctly, it's fixing an issue that
> cause a WARN?

Yes, this has been causing a regression in my testing since before the
merge window.  I originally reported the issue on the 22nd, the week
before the merge window.  

> So... Either we could 1) split the patch, have the WARN fix as a fix
> patch to the current rc, and the rest later in the next merge window, or
> 2) if you want to keep this as a single patch, I think it makes sense to
> change the subject and description to highlight the fix aspect.

The offending code had only been in -next since the 17th (a week and a
half before the merge window) so I'm not sure there's huge concerns
about stability TBH.  A this point I'm more concerned that we're going
to get a -rc1 with breakage.
with brea

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Plumb the format info from .fb_create() all the way to
drm_helper_mode_fill_fb_struct() to avoid the redundant
lookup.

The patch is based on the driver parts of the patchset at Link:
below, which missed converting the nouveau driver.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++------
 drivers/gpu/drm/nouveau/nouveau_display.h | 3 +++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index e1e5421263103..805d0a87aa546 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -253,6 +253,7 @@ nouveau_check_bl_size(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
 
 int
 nouveau_framebuffer_new(struct drm_device *dev,
+			const struct drm_format_info *info,
 			const struct drm_mode_fb_cmd2 *mode_cmd,
 			struct drm_gem_object *gem,
 			struct drm_framebuffer **pfb)
@@ -260,7 +261,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nouveau_bo *nvbo = nouveau_gem_object(gem);
 	struct drm_framebuffer *fb;
-	const struct drm_format_info *info;
 	unsigned int height, i;
 	uint32_t tile_mode;
 	uint8_t kind;
@@ -295,9 +295,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
 		kind = nvbo->kind;
 	}
 
-	info = drm_get_format_info(dev, mode_cmd->pixel_format,
-				   mode_cmd->modifier[0]);
-
 	for (i = 0; i < info->num_planes; i++) {
 		height = drm_format_info_plane_height(info,
 						      mode_cmd->height,
@@ -321,7 +318,7 @@ nouveau_framebuffer_new(struct drm_device *dev,
 	if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
 		return -ENOMEM;
 
-	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
+	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
 	fb->obj[0] = gem;
 
 	ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs);
@@ -344,7 +341,7 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
 	if (!gem)
 		return ERR_PTR(-ENOENT);
 
-	ret = nouveau_framebuffer_new(dev, mode_cmd, gem, &fb);
+	ret = nouveau_framebuffer_new(dev, info, mode_cmd, gem, &fb);
 	if (ret == 0)
 		return fb;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
index e45f211501f61..470e0910d4845 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -8,8 +8,11 @@
 
 #include <drm/drm_framebuffer.h>
 
+struct drm_format_info;
+
 int
 nouveau_framebuffer_new(struct drm_device *dev,
+			const struct drm_format_info *info,
 			const struct drm_mode_fb_cmd2 *mode_cmd,
 			struct drm_gem_object *gem,
 			struct drm_framebuffer **pfb);
-- 
2.49.1

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Hi Lyude, Danilo, Dave, Simona,

could you please ack this patch? Note that it fixes commit 41ab92d35ccd
("drm: Make passing of format info to drm_helper_mode_fill_fb_struct()
mandatory") which is already part of Dave's 6.17-rc1 pull request at:

https://lore.kernel.org/all/CAPM=9tzVm80-v6_5nt6kko3nR+aQLZ7R98i419FV8f4-ayQWUw@mail.gmail.com

Alex already acked the patchset, but patch 1 and 2 are still missing an
ack/reviewed-by from maintainers.

Thanks,
Imre

On Mon, Jul 28, 2025 at 01:16:02PM +0300, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.
> 
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the nouveau driver.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++------
>  drivers/gpu/drm/nouveau/nouveau_display.h | 3 +++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index e1e5421263103..805d0a87aa546 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -253,6 +253,7 @@ nouveau_check_bl_size(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
>  
>  int
>  nouveau_framebuffer_new(struct drm_device *dev,
> +			const struct drm_format_info *info,
>  			const struct drm_mode_fb_cmd2 *mode_cmd,
>  			struct drm_gem_object *gem,
>  			struct drm_framebuffer **pfb)
> @@ -260,7 +261,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nouveau_bo *nvbo = nouveau_gem_object(gem);
>  	struct drm_framebuffer *fb;
> -	const struct drm_format_info *info;
>  	unsigned int height, i;
>  	uint32_t tile_mode;
>  	uint8_t kind;
> @@ -295,9 +295,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
>  		kind = nvbo->kind;
>  	}
>  
> -	info = drm_get_format_info(dev, mode_cmd->pixel_format,
> -				   mode_cmd->modifier[0]);
> -
>  	for (i = 0; i < info->num_planes; i++) {
>  		height = drm_format_info_plane_height(info,
>  						      mode_cmd->height,
> @@ -321,7 +318,7 @@ nouveau_framebuffer_new(struct drm_device *dev,
>  	if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
>  		return -ENOMEM;
>  
> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>  	fb->obj[0] = gem;
>  
>  	ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs);
> @@ -344,7 +341,7 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
>  	if (!gem)
>  		return ERR_PTR(-ENOENT);
>  
> -	ret = nouveau_framebuffer_new(dev, mode_cmd, gem, &fb);
> +	ret = nouveau_framebuffer_new(dev, info, mode_cmd, gem, &fb);
>  	if (ret == 0)
>  		return fb;
>  
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> index e45f211501f61..470e0910d4845 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> @@ -8,8 +8,11 @@
>  
>  #include <drm/drm_framebuffer.h>
>  
> +struct drm_format_info;
> +
>  int
>  nouveau_framebuffer_new(struct drm_device *dev,
> +			const struct drm_format_info *info,
>  			const struct drm_mode_fb_cmd2 *mode_cmd,
>  			struct drm_gem_object *gem,
>  			struct drm_framebuffer **pfb);
> -- 
> 2.49.1
> 

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Danilo Krummrich]

On 7/28/25 12:16 PM, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.
> 
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the nouveau driver.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Acked-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Danilo Krummrich]

On 8/1/25 11:08 AM, Danilo Krummrich wrote:
> On 7/28/25 12:16 PM, Imre Deak wrote:
>> Plumb the format info from .fb_create() all the way to
>> drm_helper_mode_fill_fb_struct() to avoid the redundant
>> lookup.
>>
>> The patch is based on the driver parts of the patchset at Link:
>> below, which missed converting the nouveau driver.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to 
>> drm_helper_mode_fill_fb_struct() mandatory")
>> Link: https://lore.kernel.org/all/20250701090722.13645-1- 
>> ville.syrjala@linux.intel.com
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Any concerns if I just pick this one? It's a bit annoying to not have this fixed
in drm-misc-fixes and -next. I'd hope for -rc2 getting backmerged.

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

On Tue, Aug 12, 2025 at 07:04:44PM +0200, Danilo Krummrich wrote:
> On 8/1/25 11:08 AM, Danilo Krummrich wrote:
> > On 7/28/25 12:16 PM, Imre Deak wrote:
> > > Plumb the format info from .fb_create() all the way to
> > > drm_helper_mode_fill_fb_struct() to avoid the redundant
> > > lookup.
> > > 
> > > The patch is based on the driver parts of the patchset at Link:
> > > below, which missed converting the nouveau driver.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Fixes: 41ab92d35ccd ("drm: Make passing of format info to
> > > drm_helper_mode_fill_fb_struct() mandatory")
> > > Link: https://lore.kernel.org/all/20250701090722.13645-1-
> > > ville.syrjala@linux.intel.com
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Acked-by: Danilo Krummrich <dakr@kernel.org>
> 
> Any concerns if I just pick this one? It's a bit annoying to not have this fixed
> in drm-misc-fixes and -next. I'd hope for -rc2 getting backmerged.

Yes, this got delayed and didn't get to -rc1, maybe I could've merged
the fixes already reviewed earlier.

The fix is in drm-misc-next-fixes atm and Thomas has sent already

https://lore.kernel.org/all/20250812064712.GA14554@2a02-2454-fd5e-fd00-2c49-c639-c55f-a125.dyn6.pyur.net

which should get pulled to drm-fixes and then to -rc2, AFAIU, not sure
if that's enough for you. I let the maintainers comment about
cherry-picking it if not, I suppose the concern could be the duplicated
commits that Linus would see.

--Imre

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Danilo Krummrich]

On 8/12/25 7:16 PM, Imre Deak wrote:
> The fix is in drm-misc-next-fixes atm and Thomas has sent already
> 
> https://lore.kernel.org/all/20250812064712.GA14554@2a02-2454-fd5e-fd00-2c49-c639-c55f-a125.dyn6.pyur.net

Ok, that's great! (No further action needed AFAIC. :)

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Thomas Zimmermann]

Hi

Am 12.08.25 um 19:24 schrieb Danilo Krummrich:
> On 8/12/25 7:16 PM, Imre Deak wrote:
>> The fix is in drm-misc-next-fixes atm and Thomas has sent already
>>
>> https://lore.kernel.org/all/20250812064712.GA14554@2a02-2454-fd5e-fd00-2c49-c639-c55f-a125.dyn6.pyur.net 
>>
>
> Ok, that's great! (No further action needed AFAIC. :)

Please don't cherry-pick across trees unless absolutely necessary. It's 
a hassle for later backporting and merging.

Best regards
Thomas


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

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Danilo Krummrich]

On Wed Aug 13, 2025 at 9:25 AM CEST, Thomas Zimmermann wrote:
> Hi
>
> Am 12.08.25 um 19:24 schrieb Danilo Krummrich:
>> On 8/12/25 7:16 PM, Imre Deak wrote:
>>> The fix is in drm-misc-next-fixes atm and Thomas has sent already
>>>
>>> https://lore.kernel.org/all/20250812064712.GA14554@2a02-2454-fd5e-fd00-2c49-c639-c55f-a125.dyn6.pyur.net 
>>>
>>
>> Ok, that's great! (No further action needed AFAIC. :)
>
> Please don't cherry-pick across trees unless absolutely necessary. It's 
> a hassle for later backporting and merging.

No worries, maintaining a couple trees myself I'm not willing to contribute to
such kind of mess. :)

I didn't know if this patch was picked up somewhere else already (since I was
asked for an ACK previously). Hence, I asked if this has happened and if not, I'd
have taken it thorugh -fixes myself, ideally getting the corresponding -rc
backmerged into -next.

Thanks,
Danilo

Re: [PATCH 2/3] drm/nouveau: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[James Jones]

Review-by: James Jones <jajones@nvidia.com>
Tested-by: James Jones <jajones@nvidia.com>

Tested on GB203, TU102, and NV50.

Thanks,
-James

On 7/28/25 03:16, Imre Deak wrote:
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant
> lookup.
> 
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the nouveau driver.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
> Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++------
>   drivers/gpu/drm/nouveau/nouveau_display.h | 3 +++
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index e1e5421263103..805d0a87aa546 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -253,6 +253,7 @@ nouveau_check_bl_size(struct nouveau_drm *drm, struct nouveau_bo *nvbo,
>   
>   int
>   nouveau_framebuffer_new(struct drm_device *dev,
> +			const struct drm_format_info *info,
>   			const struct drm_mode_fb_cmd2 *mode_cmd,
>   			struct drm_gem_object *gem,
>   			struct drm_framebuffer **pfb)
> @@ -260,7 +261,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
>   	struct nouveau_drm *drm = nouveau_drm(dev);
>   	struct nouveau_bo *nvbo = nouveau_gem_object(gem);
>   	struct drm_framebuffer *fb;
> -	const struct drm_format_info *info;
>   	unsigned int height, i;
>   	uint32_t tile_mode;
>   	uint8_t kind;
> @@ -295,9 +295,6 @@ nouveau_framebuffer_new(struct drm_device *dev,
>   		kind = nvbo->kind;
>   	}
>   
> -	info = drm_get_format_info(dev, mode_cmd->pixel_format,
> -				   mode_cmd->modifier[0]);
> -
>   	for (i = 0; i < info->num_planes; i++) {
>   		height = drm_format_info_plane_height(info,
>   						      mode_cmd->height,
> @@ -321,7 +318,7 @@ nouveau_framebuffer_new(struct drm_device *dev,
>   	if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
>   		return -ENOMEM;
>   
> -	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> +	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>   	fb->obj[0] = gem;
>   
>   	ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs);
> @@ -344,7 +341,7 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
>   	if (!gem)
>   		return ERR_PTR(-ENOENT);
>   
> -	ret = nouveau_framebuffer_new(dev, mode_cmd, gem, &fb);
> +	ret = nouveau_framebuffer_new(dev, info, mode_cmd, gem, &fb);
>   	if (ret == 0)
>   		return fb;
>   
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> index e45f211501f61..470e0910d4845 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> @@ -8,8 +8,11 @@
>   
>   #include <drm/drm_framebuffer.h>
>   
> +struct drm_format_info;
> +
>   int
>   nouveau_framebuffer_new(struct drm_device *dev,
> +			const struct drm_format_info *info,
>   			const struct drm_mode_fb_cmd2 *mode_cmd,
>   			struct drm_gem_object *gem,
>   			struct drm_framebuffer **pfb);

[PATCH 3/3] drm/radeon: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Imre Deak]

Plumb the format info from .fb_create() all the way to
drm_helper_mode_fill_fb_struct() to avoid the redundant
lookup.

For the fbdev case a manual drm_get_format_info() lookup
is needed.

The patch is based on the driver parts of the patchset at Link:
below, which missed converting the radeon driver.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: amd-gfx@lists.freedesktop.org
Fixes: 41ab92d35ccd ("drm: Make passing of format info to drm_helper_mode_fill_fb_struct() mandatory")
Link: https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/radeon/radeon_display.c |  5 +++--
 drivers/gpu/drm/radeon/radeon_fbdev.c   | 11 ++++++-----
 drivers/gpu/drm/radeon/radeon_mode.h    |  2 ++
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index b4bf5dfeea2dc..4dc77c398617a 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1297,12 +1297,13 @@ static const struct drm_framebuffer_funcs radeon_fb_funcs = {
 int
 radeon_framebuffer_init(struct drm_device *dev,
 			struct drm_framebuffer *fb,
+			const struct drm_format_info *info,
 			const struct drm_mode_fb_cmd2 *mode_cmd,
 			struct drm_gem_object *obj)
 {
 	int ret;
 	fb->obj[0] = obj;
-	drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
+	drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
 	ret = drm_framebuffer_init(dev, fb, &radeon_fb_funcs);
 	if (ret) {
 		fb->obj[0] = NULL;
@@ -1341,7 +1342,7 @@ radeon_user_framebuffer_create(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	ret = radeon_framebuffer_init(dev, fb, mode_cmd, obj);
+	ret = radeon_framebuffer_init(dev, fb, info, mode_cmd, obj);
 	if (ret) {
 		kfree(fb);
 		drm_gem_object_put(obj);
diff --git a/drivers/gpu/drm/radeon/radeon_fbdev.c b/drivers/gpu/drm/radeon/radeon_fbdev.c
index e3a481bbee7b6..dc81b0c2dbff3 100644
--- a/drivers/gpu/drm/radeon/radeon_fbdev.c
+++ b/drivers/gpu/drm/radeon/radeon_fbdev.c
@@ -53,10 +53,10 @@ static void radeon_fbdev_destroy_pinned_object(struct drm_gem_object *gobj)
 }
 
 static int radeon_fbdev_create_pinned_object(struct drm_fb_helper *fb_helper,
+					     const struct drm_format_info *info,
 					     struct drm_mode_fb_cmd2 *mode_cmd,
 					     struct drm_gem_object **gobj_p)
 {
-	const struct drm_format_info *info;
 	struct radeon_device *rdev = fb_helper->dev->dev_private;
 	struct drm_gem_object *gobj = NULL;
 	struct radeon_bo *rbo = NULL;
@@ -67,8 +67,6 @@ static int radeon_fbdev_create_pinned_object(struct drm_fb_helper *fb_helper,
 	int height = mode_cmd->height;
 	u32 cpp;
 
-	info = drm_get_format_info(rdev_to_drm(rdev), mode_cmd->pixel_format,
-				   mode_cmd->modifier[0]);
 	cpp = info->cpp[0];
 
 	/* need to align pitch with crtc limits */
@@ -206,6 +204,7 @@ int radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 				    struct drm_fb_helper_surface_size *sizes)
 {
 	struct radeon_device *rdev = fb_helper->dev->dev_private;
+	const struct drm_format_info *format_info;
 	struct drm_mode_fb_cmd2 mode_cmd = { };
 	struct fb_info *info;
 	struct drm_gem_object *gobj;
@@ -224,7 +223,9 @@ int radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
-	ret = radeon_fbdev_create_pinned_object(fb_helper, &mode_cmd, &gobj);
+	format_info = drm_get_format_info(rdev_to_drm(rdev), mode_cmd.pixel_format,
+					  mode_cmd.modifier[0]);
+	ret = radeon_fbdev_create_pinned_object(fb_helper, format_info, &mode_cmd, &gobj);
 	if (ret) {
 		DRM_ERROR("failed to create fbcon object %d\n", ret);
 		return ret;
@@ -236,7 +237,7 @@ int radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 		ret = -ENOMEM;
 		goto err_radeon_fbdev_destroy_pinned_object;
 	}
-	ret = radeon_framebuffer_init(rdev_to_drm(rdev), fb, &mode_cmd, gobj);
+	ret = radeon_framebuffer_init(rdev_to_drm(rdev), fb, format_info, &mode_cmd, gobj);
 	if (ret) {
 		DRM_ERROR("failed to initialize framebuffer %d\n", ret);
 		goto err_kfree;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 3102f6c2d0556..9e34da2cacef6 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -40,6 +40,7 @@
 
 struct drm_fb_helper;
 struct drm_fb_helper_surface_size;
+struct drm_format_info;
 
 struct edid;
 struct drm_edid;
@@ -890,6 +891,7 @@ extern void
 radeon_combios_encoder_dpms_scratch_regs(struct drm_encoder *encoder, bool on);
 int radeon_framebuffer_init(struct drm_device *dev,
 			     struct drm_framebuffer *rfb,
+			     const struct drm_format_info *info,
 			     const struct drm_mode_fb_cmd2 *mode_cmd,
 			     struct drm_gem_object *obj);
 
-- 
2.49.1

RE: [PATCH 3/3] drm/radeon: Pass along the format info from .fb_create() to drm_helper_mode_fill_fb_struct()

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Imre Deak <imre.deak@intel.com>
> Sent: Monday, July 28, 2025 6:16 AM
> To: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Thomas Zimmermann <tzimmermann@suse.de>;
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; amd-gfx@lists.freedesktop.org
> Subject: [PATCH 3/3] drm/radeon: Pass along the format info from .fb_create() to
> drm_helper_mode_fill_fb_struct()
>
> Plumb the format info from .fb_create() all the way to
> drm_helper_mode_fill_fb_struct() to avoid the redundant lookup.
>
> For the fbdev case a manual drm_get_format_info() lookup is needed.
>
> The patch is based on the driver parts of the patchset at Link:
> below, which missed converting the radeon driver.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: amd-gfx@lists.freedesktop.org
> Fixes: 41ab92d35ccd ("drm: Make passing of format info to
> drm_helper_mode_fill_fb_struct() mandatory")
> Link: https://lore.kernel.org/all/20250701090722.13645-1-
> ville.syrjala@linux.intel.com
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Series is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/radeon/radeon_display.c |  5 +++--
>  drivers/gpu/drm/radeon/radeon_fbdev.c   | 11 ++++++-----
>  drivers/gpu/drm/radeon/radeon_mode.h    |  2 ++
>  3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c
> b/drivers/gpu/drm/radeon/radeon_display.c
> index b4bf5dfeea2dc..4dc77c398617a 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -1297,12 +1297,13 @@ static const struct drm_framebuffer_funcs
> radeon_fb_funcs = {  int  radeon_framebuffer_init(struct drm_device *dev,
>                       struct drm_framebuffer *fb,
> +                     const struct drm_format_info *info,
>                       const struct drm_mode_fb_cmd2 *mode_cmd,
>                       struct drm_gem_object *obj)
>  {
>       int ret;
>       fb->obj[0] = obj;
> -     drm_helper_mode_fill_fb_struct(dev, fb, NULL, mode_cmd);
> +     drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
>       ret = drm_framebuffer_init(dev, fb, &radeon_fb_funcs);
>       if (ret) {
>               fb->obj[0] = NULL;
> @@ -1341,7 +1342,7 @@ radeon_user_framebuffer_create(struct drm_device
> *dev,
>               return ERR_PTR(-ENOMEM);
>       }
>
> -     ret = radeon_framebuffer_init(dev, fb, mode_cmd, obj);
> +     ret = radeon_framebuffer_init(dev, fb, info, mode_cmd, obj);
>       if (ret) {
>               kfree(fb);
>               drm_gem_object_put(obj);
> diff --git a/drivers/gpu/drm/radeon/radeon_fbdev.c
> b/drivers/gpu/drm/radeon/radeon_fbdev.c
> index e3a481bbee7b6..dc81b0c2dbff3 100644
> --- a/drivers/gpu/drm/radeon/radeon_fbdev.c
> +++ b/drivers/gpu/drm/radeon/radeon_fbdev.c
> @@ -53,10 +53,10 @@ static void radeon_fbdev_destroy_pinned_object(struct
> drm_gem_object *gobj)  }
>
>  static int radeon_fbdev_create_pinned_object(struct drm_fb_helper *fb_helper,
> +                                          const struct drm_format_info *info,
>                                            struct drm_mode_fb_cmd2 *mode_cmd,
>                                            struct drm_gem_object **gobj_p)  {
> -     const struct drm_format_info *info;
>       struct radeon_device *rdev = fb_helper->dev->dev_private;
>       struct drm_gem_object *gobj = NULL;
>       struct radeon_bo *rbo = NULL;
> @@ -67,8 +67,6 @@ static int radeon_fbdev_create_pinned_object(struct
> drm_fb_helper *fb_helper,
>       int height = mode_cmd->height;
>       u32 cpp;
>
> -     info = drm_get_format_info(rdev_to_drm(rdev), mode_cmd->pixel_format,
> -                                mode_cmd->modifier[0]);
>       cpp = info->cpp[0];
>
>       /* need to align pitch with crtc limits */ @@ -206,6 +204,7 @@ int
> radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
>                                   struct drm_fb_helper_surface_size *sizes)  {
>       struct radeon_device *rdev = fb_helper->dev->dev_private;
> +     const struct drm_format_info *format_info;
>       struct drm_mode_fb_cmd2 mode_cmd = { };
>       struct fb_info *info;
>       struct drm_gem_object *gobj;
> @@ -224,7 +223,9 @@ int radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper
> *fb_helper,
>       mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes-
> >surface_bpp,
>                                                         sizes->surface_depth);
>
> -     ret = radeon_fbdev_create_pinned_object(fb_helper, &mode_cmd, &gobj);
> +     format_info = drm_get_format_info(rdev_to_drm(rdev),
> mode_cmd.pixel_format,
> +                                       mode_cmd.modifier[0]);
> +     ret = radeon_fbdev_create_pinned_object(fb_helper, format_info,
> +&mode_cmd, &gobj);
>       if (ret) {
>               DRM_ERROR("failed to create fbcon object %d\n", ret);
>               return ret;
> @@ -236,7 +237,7 @@ int radeon_fbdev_driver_fbdev_probe(struct drm_fb_helper
> *fb_helper,
>               ret = -ENOMEM;
>               goto err_radeon_fbdev_destroy_pinned_object;
>       }
> -     ret = radeon_framebuffer_init(rdev_to_drm(rdev), fb, &mode_cmd, gobj);
> +     ret = radeon_framebuffer_init(rdev_to_drm(rdev), fb, format_info,
> +&mode_cmd, gobj);
>       if (ret) {
>               DRM_ERROR("failed to initialize framebuffer %d\n", ret);
>               goto err_kfree;
> diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
> b/drivers/gpu/drm/radeon/radeon_mode.h
> index 3102f6c2d0556..9e34da2cacef6 100644
> --- a/drivers/gpu/drm/radeon/radeon_mode.h
> +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> @@ -40,6 +40,7 @@
>
>  struct drm_fb_helper;
>  struct drm_fb_helper_surface_size;
> +struct drm_format_info;
>
>  struct edid;
>  struct drm_edid;
> @@ -890,6 +891,7 @@ extern void
>  radeon_combios_encoder_dpms_scratch_regs(struct drm_encoder *encoder, bool
> on);  int radeon_framebuffer_init(struct drm_device *dev,
>                            struct drm_framebuffer *rfb,
> +                          const struct drm_format_info *info,
>                            const struct drm_mode_fb_cmd2 *mode_cmd,
>                            struct drm_gem_object *obj);
>
> --
> 2.49.1

[PATCH 0/3] drm: Fix initializing format info in drm_framebuffer

[Naresh Kamboju]

> This patchset fixes initializing the format info in drm_framebuffer by
> drm_helper_mode_fill_fb_struct() in the omap, nouveau, radeon drivers
> after patch [1] in patchset [2], the latter one missing the conversion
> of these 3 drivers. This patchset converts the 3 drivers similarly to
> the other drivers converted by [2].

> This patchset was only compile tested.

> [1] https://lore.kernel.org/all/20250701090722.13645-20-ville.syrjala@linux.intel.com
> [2] https://lore.kernel.org/all/20250701090722.13645-1-ville.syrjala@linux.intel.com

Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>

Test job,
 - https://lkft.validation.linaro.org/scheduler/job/8377982