Re: [PATCH] Revert "drm/amdgpu: Verify bo size can fit framebuffer size on init."

[Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>]

On Thu, Apr 29, 2021 at 11:27 PM Mark Yacoub <markyacoub@chromium.org> wrote:
>
> On Thu, Apr 29, 2021 at 4:50 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > This reverts commit f258907fdd835e1aed6d666b00cdd0f186676b7c.
> >
> > Same problem as "drm/amdgpu: Verify bo size can fit framebuffer size",
> > but because it gets checked earlier it now only triggers on the
> > modifiers case.
> >
> > There are a couple of reasons why the DRM core BO size check won't
> > work for AMDGPU, especially around DCC planes.
> >
> Can you tell us more about those reasons? Last time this was reverted
> due to a failure on ubuntu was due to a userspace bug that was fixed.
> So I'm thinking we might wanna fix what broke instead of removing the
> check.

Agree on having the check in the end, just wasn't sure if fixes (or
when I started looking at it, I thought stable) was the right place
given some of the tiling complexity.

So the core problems:

1) In the format structs we don't do set any of the tilesize / blocks
etc. to avoid having format arrays per modifier/GPU
2) The pitch on the main plane is pixel_pitch * bytes_per_pixel even
for tiled ...
3) The pitch for the DCC planes is really the pixel pitch of the main
surface that would be covered by it ...

1 is changeable by refactoring but sadly 2 and 3 are hard to change by
now (would need to bump the modifier version). And all 3 mean that the
default computation in the core drm helper is not the right check for
BO size.

> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > ---
> >
> > For -fixes. Might have some conflicts with
> > "drm/amdgpu: Ensure that the modifier requested is supported by plane"
> > for amd-staging-drm-next
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 68 ++++-----------------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c      |  4 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h    |  8 ---
> >  3 files changed, 15 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index 9a2f811450ed..cbe050436c7b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -870,62 +870,17 @@ static int amdgpu_display_get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb
> >         return r;
> >  }
> >
> > -int amdgpu_display_gem_fb_init(struct drm_device *dev,
> > -                              struct amdgpu_framebuffer *rfb,
> > -                              const struct drm_mode_fb_cmd2 *mode_cmd,
> > -                              struct drm_gem_object *obj)
> > -{
> > -       int ret;
> > -
> > -       rfb->base.obj[0] = obj;
> > -       drm_helper_mode_fill_fb_struct(dev, &rfb->base, mode_cmd);
> > -       ret = drm_framebuffer_init(dev, &rfb->base, &amdgpu_fb_funcs);
> > -       if (ret)
> > -               goto err;
> > -
> > -       ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
> > -       if (ret)
> > -               goto err;
> > -
> > -       return 0;
> > -err:
> > -       drm_err(dev, "Failed to init gem fb: %d\n", ret);
> > -       rfb->base.obj[0] = NULL;
> > -       return ret;
> > -}
> > -
> > -int amdgpu_display_gem_fb_verify_and_init(
> > -       struct drm_device *dev, struct amdgpu_framebuffer *rfb,
> > -       struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> > -       struct drm_gem_object *obj)
> > -{
> > -       int ret;
> > -
> > -       rfb->base.obj[0] = obj;
> > -
> > -       /* Verify that bo size can fit the fb size. */
> > -       ret = drm_gem_fb_init_with_funcs(dev, &rfb->base, file_priv, mode_cmd,
> > -                                        &amdgpu_fb_funcs);
> > -       if (ret)
> > -               goto err;
> > -
> > -       ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
> > -       if (ret)
> > -               goto err;
> > -
> > -       return 0;
> > -err:
> > -       drm_err(dev, "Failed to verify and init gem fb: %d\n", ret);
> > -       rfb->base.obj[0] = NULL;
> > -       return ret;
> > -}
> > -
> >  int amdgpu_display_framebuffer_init(struct drm_device *dev,
> >                                     struct amdgpu_framebuffer *rfb,
> >                                     const struct drm_mode_fb_cmd2 *mode_cmd,
> >                                     struct drm_gem_object *obj)
> >  {
> >         int ret, i;
> > +       rfb->base.obj[0] = obj;
> > +       drm_helper_mode_fill_fb_struct(dev, &rfb->base, mode_cmd);
> > +       ret = drm_framebuffer_init(dev, &rfb->base, &amdgpu_fb_funcs);
> > +       if (ret)
> > +               goto fail;
> >
> >         /*
> >          * This needs to happen before modifier conversion as that might change
> > @@ -936,13 +891,13 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
> >                         drm_dbg_kms(dev, "Plane 0 and %d have different BOs: %u vs. %u\n",
> >                                     i, mode_cmd->handles[0], mode_cmd->handles[i]);
> >                         ret = -EINVAL;
> > -                       return ret;
> > +                       goto fail;
> >                 }
> >         }
> >
> >         ret = amdgpu_display_get_fb_info(rfb, &rfb->tiling_flags, &rfb->tmz_surface);
> >         if (ret)
> > -               return ret;
> > +               goto fail;
> >
> >         if (dev->mode_config.allow_fb_modifiers &&
> >             !(rfb->base.flags & DRM_MODE_FB_MODIFIERS)) {
> > @@ -950,7 +905,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
> >                 if (ret) {
> >                         drm_dbg_kms(dev, "Failed to convert tiling flags 0x%llX to a modifier",
> >                                     rfb->tiling_flags);
> > -                       return ret;
> > +                       goto fail;
> >                 }
> >         }
> >
> > @@ -961,6 +916,10 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
> >         }
> >
> >         return 0;
> > +
> > +fail:
> > +       rfb->base.obj[0] = NULL;
> > +       return ret;
> >  }
> >
> >  struct drm_framebuffer *
> > @@ -995,8 +954,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
> >                 return ERR_PTR(-ENOMEM);
> >         }
> >
> > -       ret = amdgpu_display_gem_fb_verify_and_init(dev, amdgpu_fb, file_priv,
> > -                                                   mode_cmd, obj);
> > +       ret = amdgpu_display_framebuffer_init(dev, amdgpu_fb, mode_cmd, obj);
> >         if (ret) {
> >                 kfree(amdgpu_fb);
> >                 drm_gem_object_put(obj);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > index 4c5c19820d37..24010cacf7d0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > @@ -232,8 +232,8 @@ static int amdgpufb_create(struct drm_fb_helper *helper,
> >                 goto out;
> >         }
> >
> > -       ret = amdgpu_display_gem_fb_init(adev_to_drm(adev), &rfbdev->rfb,
> > -                                        &mode_cmd, gobj);
> > +       ret = amdgpu_display_framebuffer_init(adev_to_drm(adev), &rfbdev->rfb,
> > +                                             &mode_cmd, gobj);
> >         if (ret) {
> >                 DRM_ERROR("failed to initialize framebuffer %d\n", ret);
> >                 goto out;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > index cb0b581bbce7..319cb19e1b99 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > @@ -602,14 +602,6 @@ int amdgpu_display_get_crtc_scanoutpos(struct drm_device *dev,
> >                         int *hpos, ktime_t *stime, ktime_t *etime,
> >                         const struct drm_display_mode *mode);
> >
> > -int amdgpu_display_gem_fb_init(struct drm_device *dev,
> > -                              struct amdgpu_framebuffer *rfb,
> > -                              const struct drm_mode_fb_cmd2 *mode_cmd,
> > -                              struct drm_gem_object *obj);
> > -int amdgpu_display_gem_fb_verify_and_init(
> > -       struct drm_device *dev, struct amdgpu_framebuffer *rfb,
> > -       struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> > -       struct drm_gem_object *obj);
> >  int amdgpu_display_framebuffer_init(struct drm_device *dev,
> >                                     struct amdgpu_framebuffer *rfb,
> >                                     const struct drm_mode_fb_cmd2 *mode_cmd,
> > --
> > 2.31.1
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx