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

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

[Vincent Abriou]

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.


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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] Revert "drm/core: Preserve the framebuffer after removing it."

[Vincent Abriou]

This reverts commit 13803132818cf8084d169617be060fd8e3411a98.

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.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 626b0a5..9b9c4b4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3327,7 +3327,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
-	drm_framebuffer_unreference(fb);
+	drm_framebuffer_remove(fb);
 
 	return 0;
 
@@ -3517,7 +3517,7 @@ void drm_fb_release(struct drm_file *priv)
 		list_del_init(&fb->filp_head);
 
 		/* This will also drop the fpriv->fbs reference. */
-		drm_framebuffer_unreference(fb);
+		drm_framebuffer_remove(fb);
 	}
 }
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Vincent ABRIOU]

Hi David,

On 10/02/2015 01:49 PM, David Herrmann wrote:
> 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.
>

The scenario is the following (only considering weston):
Weston is started and a video or whatever is displayed in a plane. Then 
user want to stop weston. He expects to have nothing displayed on the 
screen but neither the crtc nor the plane are disabled. They are still 
displayed but the plane is frozen. If user restart weston he will see a 
black screen with the frozen plane then the crtc is displayed.

When weston is stop user expect to have everything closed at least to 
save power consumption.

> Furthermore, using drmModeRmFB() should not be enough to reset a
> plane. Can you back your claim that weston relies on this behavior?

I agree with you the drmModeRmFB is not enough to reset crtc or plane 
but at least for the crtc weston is not disabling the mode when weston 
is stopped. We can surely update weston to change this behavior but how 
many other middleware have been inspired from modetest?

> 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..
>

Modetest fails to properly close crtc. Same behavior than weston. 
Modetest does not give the right example on how crtc and plane should be 
release. We must surely clean modetest first before doing anything else.

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

First have been disturb by this patch. Then I understood that it could 
solve flickering issue while booting the kernel. My first reaction was 
"Ok we just need to update modetest and weston... cool". But we don't 
know if we will not break anything on other middleware.

In my opinion, this is too risky for the moment.

BR
Vincent
>
> 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
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel