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

Hi

On Fri, Oct 2, 2015 at 1:40 PM, Vincent Abriou vincent.abriou@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@linux.intel.com Cc: David Herrmann dh.herrmann@gmail.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Vincent Abriou vincent.abriou@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