[PATCH 1/2] Revert "drm/core: Preserve the fb id on close."

[David Herrmann]

Hi

On Fri, Oct 2, 2015 at 1:40 PM, Vincent Abriou <vincent.abriou at st.com> wrote:
> This reverts commit 73f7570bc6c853ca1fad24f9d31815b20e405354.
>
> This patch need to be revert because it breaks middlewares way of working.
> As example, modetest and weston, only relies on drmModeRmFB to close
> CRTC or planes.
> Keeping this patch will induce weird behavior like a plane displayed
> with modetest will be visible on weston and vice-versa.
>
> We can overcome this by updating the middleware (successfully tested
> with modetest) to force them to disable CRTC and plane using
> drmModeSetCrtc or drmModeSetPlane.
> But it is a long to way and for short term, it is not acceptable to break
> ABI.

Can you please provide some real scenario that breaks? Using
'modetest' is not enough to argue for a revert. It is a testing tool
to exercise DRM internals, of course it is possible to show the new
behavior by using it.

Furthermore, using drmModeRmFB() should not be enough to reset a
plane. Can you back your claim that weston relies on this behavior?
Reading weston code, I see an explicit plane-disable on each render
pass. I cannot see how it relies on planes to be disabled after
removing the FB (and it really cannot do that..). Regarding modetest:
this is not a runtime tool. It does not do dynamic adjustments to
runtime changes. Hence, it obviously relies on close(2) to clean
things up..

I'd really appreciate if you could elaborate why *exactly* you think
this is needed.

Thanks
David

>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Vincent Abriou <vincent.abriou at st.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e600a5f..626b0a5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3320,6 +3320,9 @@ int drm_mode_rmfb(struct drm_device *dev,
>         if (!found)
>                 goto fail_lookup;
>
> +       /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
> +       __drm_framebuffer_unregister(dev, fb);
> +
>         list_del_init(&fb->filp_head);
>         mutex_unlock(&dev->mode_config.fb_lock);
>         mutex_unlock(&file_priv->fbs_lock);
> @@ -3491,6 +3494,7 @@ out_err1:
>   */
>  void drm_fb_release(struct drm_file *priv)
>  {
> +       struct drm_device *dev = priv->minor->dev;
>         struct drm_framebuffer *fb, *tfb;
>
>         /*
> @@ -3504,9 +3508,15 @@ void drm_fb_release(struct drm_file *priv)
>          * at it any more.
>          */
>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> +
> +               mutex_lock(&dev->mode_config.fb_lock);
> +               /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
> +               __drm_framebuffer_unregister(dev, fb);
> +               mutex_unlock(&dev->mode_config.fb_lock);
> +
>                 list_del_init(&fb->filp_head);
>
> -               /* This drops the fpriv->fbs reference. */
> +               /* This will also drop the fpriv->fbs reference. */
>                 drm_framebuffer_unreference(fb);
>         }
>  }
> --
> 1.9.1
>