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.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?
>
> 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.valkei...@ideasonboard.com>
If you want me to merge the omap one separately, just say so.
Tomi
