Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Maarten Lankhorst]

Op 16-02-17 om 10:45 schreef Jani Nikula:
> On Wed, 15 Feb 2017, Sinclair Yeh  wrote:
>> On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
>>> On Wed, 25 Jan 2017, Maarten Lankhorst  
>>> wrote:
 This was somehow lost between v3 and the merged version in Maarten's
 patch merged as:

 commit f2d580b9a8149735cbc4b59c4a8df60173658140
 Author: Maarten Lankhorst 
 Date:   Wed May 4 14:38:26 2016 +0200

 drm/core: Do not preserve framebuffer on rmfb, v4.

 This introduces a slight behavioral change to rmfb. Instead of
 disabling a crtc when the primary plane is disabled, we try to
 preserve it.

 Apart from old versions of the vmwgfx xorg driver, there is
 nothing depending on rmfb disabling a crtc. Since vmwgfx is
 a legacy driver we can safely only disable the plane with atomic.

 If this commit is rejected by the driver then we will still fall
 back to the old behavior and turn off the crtc.

 v2:
 - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
 - Add WARN_ON when atomic_remove_fb fails.
 - Always call drm_atomic_state_put.
 v3:
 - Use drm_drv_uses_atomic_modeset
 - Handle the case where the first plane-disable-only commit fails
   with -EINVAL. Some drivers do not support this, fall back to
   disabling all crtc's in this case.

 Signed-off-by: Daniel Vetter 
 Signed-off-by: Daniel Vetter 
 Signed-off-by: Maarten Lankhorst 
>>> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
>>> for the patch.
>> I verified yesterday that this patch will cause a regression for vmwgfx
>> multi-mon.  Can we hold off on this?
> Turns out it fails some of our tests too. Maybe three weeks old CI
> results are not to be trusted, the base moves too fast. *shrug*.
>
> I've dropped the patch, and asked Dave not to pull. Let's go back to the
> drawing board...
Yeah it's unfortunate. We tend to trigger a lot of bugs in other parts of the 
code with this change. The reload tests are fixed by fixing 
drm_atomic_helper_disable_all.
I haven't seen the crc bugs before, would be interesting to know why other 
tests a're suddenly failing.

~Maarten
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Jani Nikula]

On Wed, 15 Feb 2017, Sinclair Yeh  wrote:
> On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
>> On Wed, 25 Jan 2017, Maarten Lankhorst  
>> wrote:
>> > This was somehow lost between v3 and the merged version in Maarten's
>> > patch merged as:
>> >
>> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
>> > Author: Maarten Lankhorst 
>> > Date:   Wed May 4 14:38:26 2016 +0200
>> >
>> > drm/core: Do not preserve framebuffer on rmfb, v4.
>> >
>> > This introduces a slight behavioral change to rmfb. Instead of
>> > disabling a crtc when the primary plane is disabled, we try to
>> > preserve it.
>> >
>> > Apart from old versions of the vmwgfx xorg driver, there is
>> > nothing depending on rmfb disabling a crtc. Since vmwgfx is
>> > a legacy driver we can safely only disable the plane with atomic.
>> >
>> > If this commit is rejected by the driver then we will still fall
>> > back to the old behavior and turn off the crtc.
>> >
>> > v2:
>> > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
>> > - Add WARN_ON when atomic_remove_fb fails.
>> > - Always call drm_atomic_state_put.
>> > v3:
>> > - Use drm_drv_uses_atomic_modeset
>> > - Handle the case where the first plane-disable-only commit fails
>> >   with -EINVAL. Some drivers do not support this, fall back to
>> >   disabling all crtc's in this case.
>> >
>> > Signed-off-by: Daniel Vetter 
>> > Signed-off-by: Daniel Vetter 
>> > Signed-off-by: Maarten Lankhorst 
>> 
>> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
>> for the patch.
>
> I verified yesterday that this patch will cause a regression for vmwgfx
> multi-mon.  Can we hold off on this?

Turns out it fails some of our tests too. Maybe three weeks old CI
results are not to be trusted, the base moves too fast. *shrug*.

I've dropped the patch, and asked Dave not to pull. Let's go back to the
drawing board...

BR,
Jani.


>
>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > ---
>> >  drivers/gpu/drm/drm_atomic.c| 106 
>> > 
>> >  drivers/gpu/drm/drm_crtc_internal.h |   1 +
>> >  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
>> >  3 files changed, 114 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > index 723392fc98c8..c79ab8048435 100644
>> > --- a/drivers/gpu/drm/drm_atomic.c
>> > +++ b/drivers/gpu/drm/drm_atomic.c
>> > @@ -2058,6 +2058,112 @@ static void complete_crtc_signaling(struct 
>> > drm_device *dev,
>> >kfree(fence_state);
>> >  }
>> >  
>> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>> > +{
>> > +  struct drm_modeset_acquire_ctx ctx;
>> > +  struct drm_device *dev = fb->dev;
>> > +  struct drm_atomic_state *state;
>> > +  struct drm_plane *plane;
>> > +  struct drm_connector *conn;
>> > +  struct drm_connector_state *conn_state;
>> > +  int i, ret = 0;
>> > +  unsigned plane_mask, disable_crtcs = false;
>> > +
>> > +  state = drm_atomic_state_alloc(dev);
>> > +  if (!state)
>> > +  return -ENOMEM;
>> > +
>> > +  drm_modeset_acquire_init(, 0);
>> > +  state->acquire_ctx = 
>> > +
>> > +retry:
>> > +  plane_mask = 0;
>> > +  ret = drm_modeset_lock_all_ctx(dev, );
>> > +  if (ret)
>> > +  goto unlock;
>> > +
>> > +  drm_for_each_plane(plane, dev) {
>> > +  struct drm_plane_state *plane_state;
>> > +
>> > +  if (plane->state->fb != fb)
>> > +  continue;
>> > +
>> > +  plane_state = drm_atomic_get_plane_state(state, plane);
>> > +  if (IS_ERR(plane_state)) {
>> > +  ret = PTR_ERR(plane_state);
>> > +  goto unlock;
>> > +  }
>> > +
>> > +  /*
>> > +   * Some drivers do not support keeping crtc active with the
>> > +   * primary plane disabled. If we fail to commit with -EINVAL
>> > +   * then we will try to perform the same commit but with all
>> > +   * crtc's disabled for primary planes as well.
>> > +   */
>> > +  if (disable_crtcs && plane_state->crtc->primary == plane) {
>> > +  struct drm_crtc_state *crtc_state;
>> > +
>> > +  crtc_state = drm_atomic_get_existing_crtc_state(state, 
>> > plane_state->crtc);
>> > +
>> > +  ret = drm_atomic_add_affected_connectors(state, 
>> > plane_state->crtc);
>> > +  if (ret)
>> > +  goto unlock;
>> > +
>> > +  crtc_state->active = false;
>> > +  ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
>> > +  if (ret)
>> > +  goto unlock;
>> > +  }
>> > +
>> > +  drm_atomic_set_fb_for_plane(plane_state, NULL);
>> > +  ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
>> > +  if (ret)
>> > + 

Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Sinclair Yeh]

On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
> On Wed, 25 Jan 2017, Maarten Lankhorst  
> wrote:
> > This was somehow lost between v3 and the merged version in Maarten's
> > patch merged as:
> >
> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
> > Author: Maarten Lankhorst 
> > Date:   Wed May 4 14:38:26 2016 +0200
> >
> > drm/core: Do not preserve framebuffer on rmfb, v4.
> >
> > This introduces a slight behavioral change to rmfb. Instead of
> > disabling a crtc when the primary plane is disabled, we try to
> > preserve it.
> >
> > Apart from old versions of the vmwgfx xorg driver, there is
> > nothing depending on rmfb disabling a crtc. Since vmwgfx is
> > a legacy driver we can safely only disable the plane with atomic.
> >
> > If this commit is rejected by the driver then we will still fall
> > back to the old behavior and turn off the crtc.
> >
> > v2:
> > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
> > - Add WARN_ON when atomic_remove_fb fails.
> > - Always call drm_atomic_state_put.
> > v3:
> > - Use drm_drv_uses_atomic_modeset
> > - Handle the case where the first plane-disable-only commit fails
> >   with -EINVAL. Some drivers do not support this, fall back to
> >   disabling all crtc's in this case.
> >
> > Signed-off-by: Daniel Vetter 
> > Signed-off-by: Daniel Vetter 
> > Signed-off-by: Maarten Lankhorst 
> 
> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
> for the patch.

I verified yesterday that this patch will cause a regression for vmwgfx
multi-mon.  Can we hold off on this?



> 
> BR,
> Jani.
> 
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 106 
> > 
> >  drivers/gpu/drm/drm_crtc_internal.h |   1 +
> >  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
> >  3 files changed, 114 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 723392fc98c8..c79ab8048435 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -2058,6 +2058,112 @@ static void complete_crtc_signaling(struct 
> > drm_device *dev,
> > kfree(fence_state);
> >  }
> >  
> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
> > +{
> > +   struct drm_modeset_acquire_ctx ctx;
> > +   struct drm_device *dev = fb->dev;
> > +   struct drm_atomic_state *state;
> > +   struct drm_plane *plane;
> > +   struct drm_connector *conn;
> > +   struct drm_connector_state *conn_state;
> > +   int i, ret = 0;
> > +   unsigned plane_mask, disable_crtcs = false;
> > +
> > +   state = drm_atomic_state_alloc(dev);
> > +   if (!state)
> > +   return -ENOMEM;
> > +
> > +   drm_modeset_acquire_init(, 0);
> > +   state->acquire_ctx = 
> > +
> > +retry:
> > +   plane_mask = 0;
> > +   ret = drm_modeset_lock_all_ctx(dev, );
> > +   if (ret)
> > +   goto unlock;
> > +
> > +   drm_for_each_plane(plane, dev) {
> > +   struct drm_plane_state *plane_state;
> > +
> > +   if (plane->state->fb != fb)
> > +   continue;
> > +
> > +   plane_state = drm_atomic_get_plane_state(state, plane);
> > +   if (IS_ERR(plane_state)) {
> > +   ret = PTR_ERR(plane_state);
> > +   goto unlock;
> > +   }
> > +
> > +   /*
> > +* Some drivers do not support keeping crtc active with the
> > +* primary plane disabled. If we fail to commit with -EINVAL
> > +* then we will try to perform the same commit but with all
> > +* crtc's disabled for primary planes as well.
> > +*/
> > +   if (disable_crtcs && plane_state->crtc->primary == plane) {
> > +   struct drm_crtc_state *crtc_state;
> > +
> > +   crtc_state = drm_atomic_get_existing_crtc_state(state, 
> > plane_state->crtc);
> > +
> > +   ret = drm_atomic_add_affected_connectors(state, 
> > plane_state->crtc);
> > +   if (ret)
> > +   goto unlock;
> > +
> > +   crtc_state->active = false;
> > +   ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> > +   if (ret)
> > +   goto unlock;
> > +   }
> > +
> > +   drm_atomic_set_fb_for_plane(plane_state, NULL);
> > +   ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> > +   if (ret)
> > +   goto unlock;
> > +
> > +   plane_mask |= BIT(drm_plane_index(plane));
> > +
> > +   plane->old_fb = plane->fb;
> > +   }
> > +
> > +   /* This list is only not empty when disable_crtcs is set. */
> > +   for_each_connector_in_state(state, conn, conn_state, i) {
> > +   ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> > +
> > 

Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Jani Nikula]

On Wed, 25 Jan 2017, Maarten Lankhorst  
wrote:
> This was somehow lost between v3 and the merged version in Maarten's
> patch merged as:
>
> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> Author: Maarten Lankhorst 
> Date:   Wed May 4 14:38:26 2016 +0200
>
> drm/core: Do not preserve framebuffer on rmfb, v4.
>
> This introduces a slight behavioral change to rmfb. Instead of
> disabling a crtc when the primary plane is disabled, we try to
> preserve it.
>
> Apart from old versions of the vmwgfx xorg driver, there is
> nothing depending on rmfb disabling a crtc. Since vmwgfx is
> a legacy driver we can safely only disable the plane with atomic.
>
> If this commit is rejected by the driver then we will still fall
> back to the old behavior and turn off the crtc.
>
> v2:
> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
> - Add WARN_ON when atomic_remove_fb fails.
> - Always call drm_atomic_state_put.
> v3:
> - Use drm_drv_uses_atomic_modeset
> - Handle the case where the first plane-disable-only commit fails
>   with -EINVAL. Some drivers do not support this, fall back to
>   disabling all crtc's in this case.
>
> Signed-off-by: Daniel Vetter 
> Signed-off-by: Daniel Vetter 
> Signed-off-by: Maarten Lankhorst 

Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
for the patch.

BR,
Jani.


> ---
>  drivers/gpu/drm/drm_atomic.c| 106 
> 
>  drivers/gpu/drm/drm_crtc_internal.h |   1 +
>  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
>  3 files changed, 114 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 723392fc98c8..c79ab8048435 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2058,6 +2058,112 @@ static void complete_crtc_signaling(struct drm_device 
> *dev,
>   kfree(fence_state);
>  }
>  
> +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
> +{
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_device *dev = fb->dev;
> + struct drm_atomic_state *state;
> + struct drm_plane *plane;
> + struct drm_connector *conn;
> + struct drm_connector_state *conn_state;
> + int i, ret = 0;
> + unsigned plane_mask, disable_crtcs = false;
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state)
> + return -ENOMEM;
> +
> + drm_modeset_acquire_init(, 0);
> + state->acquire_ctx = 
> +
> +retry:
> + plane_mask = 0;
> + ret = drm_modeset_lock_all_ctx(dev, );
> + if (ret)
> + goto unlock;
> +
> + drm_for_each_plane(plane, dev) {
> + struct drm_plane_state *plane_state;
> +
> + if (plane->state->fb != fb)
> + continue;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto unlock;
> + }
> +
> + /*
> +  * Some drivers do not support keeping crtc active with the
> +  * primary plane disabled. If we fail to commit with -EINVAL
> +  * then we will try to perform the same commit but with all
> +  * crtc's disabled for primary planes as well.
> +  */
> + if (disable_crtcs && plane_state->crtc->primary == plane) {
> + struct drm_crtc_state *crtc_state;
> +
> + crtc_state = drm_atomic_get_existing_crtc_state(state, 
> plane_state->crtc);
> +
> + ret = drm_atomic_add_affected_connectors(state, 
> plane_state->crtc);
> + if (ret)
> + goto unlock;
> +
> + crtc_state->active = false;
> + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> + if (ret)
> + goto unlock;
> + }
> +
> + drm_atomic_set_fb_for_plane(plane_state, NULL);
> + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + if (ret)
> + goto unlock;
> +
> + plane_mask |= BIT(drm_plane_index(plane));
> +
> + plane->old_fb = plane->fb;
> + }
> +
> + /* This list is only not empty when disable_crtcs is set. */
> + for_each_connector_in_state(state, conn, conn_state, i) {
> + ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +
> + if (ret)
> + goto unlock;
> + }
> +
> + if (plane_mask)
> + ret = drm_atomic_commit(state);
> +
> +unlock:
> + if (plane_mask)
> + drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff();
> +