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.syrj...@linux.intel.com>
>>> Cc: Tomi Valkeinen <tomi.valkei...@ideasonboard.com>
>>> Cc: Thomas Zimmermann <tzimmerm...@suse.de>
>>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>>> Cc: Maxime Ripard <mrip...@kernel.org>
>>> Fixes: 41ab92d35ccd ("drm: Make passing of format info to
>>> drm_helper_mode_fill_fb_struct() mandatory")
>>> Reported-by: Mark Brown <broo...@kernel.org>
>>> Closes:
>>> https://lore.kernel.org/all/98b3a62c-91ff-4f91-a58b-e1265f841...@sirena.org.uk
>>> Link:
>>> https://lore.kernel.org/all/20250701090722.13645-1-ville.syrj...@linux.intel.com
>>> Signed-off-by: Imre Deak <imre.d...@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.syrj...@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
>>
