Hi
On Fri, Oct 2, 2015 at 1:40 PM, Vincent Abriou <vincent.abriou@xxxxxx> 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@xxxxxxxxxxxxxxx>
> Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Vincent Abriou <vincent.abriou@xxxxxx>
> ---
> 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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
