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

[Imre Deak <imre.deak@intel.com>]

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