[PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Maarten Lankhorst]

When disable_noatomic is called plane_mask is not reliable yet,
and plane_state->visible = true even after disabling the primary plane.

Fix this by unsetting plane->visible if it was visible, and calling
disable_planes with the primary plane as mask.

The other planes are already disabled in intel_sanitize_crtc, so
they don't have to be handled here.

Cc: stable@vger.kernel.org #v4.3, v4.2?
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493213b7..bc3282ab5ed2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		WARN_ON(intel_crtc->unpin_work);
 
 		intel_pre_disable_primary(crtc);
+
+		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
+		to_intel_plane_state(crtc->primary->state)->visible = false;
 	}
 
-	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
 	dev_priv->display.crtc_disable(crtc);
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Ander Conselvan De Oliveira]

On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
> When disable_noatomic is called plane_mask is not reliable yet,
> and plane_state->visible = true even after disabling the primary plane.

So the stale value of plane_state->visible causes a subsequent modeset to enable
the primary again?

> Fix this by unsetting plane->visible if it was visible, and calling
> disable_planes with the primary plane as mask.
> 
> The other planes are already disabled in intel_sanitize_crtc, so
> they don't have to be handled here.
> 
> Cc: stable@vger.kernel.org #v4.3, v4.2?
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..bc3282ab5ed2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
> *crtc)
>  		WARN_ON(intel_crtc->unpin_work);
>  
>  		intel_pre_disable_primary(crtc);
> +
> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc
> ->primary));
> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>  	}
>  
> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);

Can't we just make plane_mask reliable? We know whether the primary plane is
enabled and all others were disabled prior to that. Or did I miss something.

Ander

>  	dev_priv->display.crtc_disable(crtc);
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);

Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Maarten Lankhorst]

Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>> When disable_noatomic is called plane_mask is not reliable yet,
>> and plane_state->visible = true even after disabling the primary plane.
> So the stale value of plane_state->visible causes a subsequent modeset to enable
> the primary again?
Probably not because it would get recalculated in calc_changes, but it should really be set to false afterwards.

in intel_plane_atomic_calc_changes:
if (!was_crtc_enabled && WARN_ON(was_visible))
was_visible = false;

and in intel_sanitize_crtc when intel_check_plane_mapping fails, it gets set to true to disable it.

This is really a special case and it's for disable_noatomic only. As the name says it can't rely on the atomic state.

>> Fix this by unsetting plane->visible if it was visible, and calling
>> disable_planes with the primary plane as mask.
>>
>> The other planes are already disabled in intel_sanitize_crtc, so
>> they don't have to be handled here.
>>
>> Cc: stable@vger.kernel.org #v4.3, v4.2?
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
>> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index b5f7493213b7..bc3282ab5ed2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
>> *crtc)
>>  		WARN_ON(intel_crtc->unpin_work);
>>  
>>  		intel_pre_disable_primary(crtc);
>> +
>> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc
>> ->primary));
>> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>>  	}
>>  
>> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
> Can't we just make plane_mask reliable? We know whether the primary plane is
> enabled and all others were disabled prior to that. Or did I miss something.
>
In the case of hardware readout you can't. The plane_mask gets set only if we can preserve the initial framebuffer
else the primary plane gets disabled.

~Maarten

Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Ander Conselvan De Oliveira]

On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
> > On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
> > > When disable_noatomic is called plane_mask is not reliable yet,
> > > and plane_state->visible = true even after disabling the primary plane.
> > So the stale value of plane_state->visible causes a subsequent modeset to
> > enable
> > the primary again?
> Probably not because it would get recalculated in calc_changes, but it should
> really be set to false afterwards.

So basically I didn't understand how the wrong value of plane_state->visible
causes the bug that was mentioned. I think a brief explanation in the commit
message would be good.

> 
> in intel_plane_atomic_calc_changes:
> if (!was_crtc_enabled && WARN_ON(was_visible))
> was_visible = false;
> 
> and in intel_sanitize_crtc when intel_check_plane_mapping fails, it gets set
> to true to disable it.
> 
> This is really a special case and it's for disable_noatomic only. As the name
> says it can't rely on the atomic state.
> 
> > > Fix this by unsetting plane->visible if it was visible, and calling
> > > disable_planes with the primary plane as mask.
> > > 
> > > The other planes are already disabled in intel_sanitize_crtc, so
> > > they don't have to be handled here.
> > > 
> > > Cc: stable@vger.kernel.org #v4.3, v4.2?
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
> > > Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index b5f7493213b7..bc3282ab5ed2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct
> > > drm_crtc
> > > *crtc)
> > >  		WARN_ON(intel_crtc->unpin_work);
> > >  
> > >  		intel_pre_disable_primary(crtc);
> > > +
> > > +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc
> > > ->primary));
> > > +		to_intel_plane_state(crtc->primary->state)->visible =
> > > false;
> > >  	}
> > >  
> > > -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
> > Can't we just make plane_mask reliable? We know whether the primary plane is
> > enabled and all others were disabled prior to that. Or did I miss something.
> > 
> In the case of hardware readout you can't. The plane_mask gets set only if we
> can preserve the initial framebuffer
> else the primary plane gets disabled.

What I meant to say is that before a call to disable_noatomic but after we
disabled all other planes, plane_mask could have the appropriate value. As in we
could set plane_mask to 0 or drm_plane_index(primary) in readout_plane_state. It
would have to be update in disable_noatomic() though, the same way as 'visible'
is.

Anyway, I think the code is fine either way.

Ander

[PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Maarten Lankhorst]

Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>>>> When disable_noatomic is called plane_mask is not reliable yet,
>>>> and plane_state->visible = true even after disabling the primary plane.
>>> So the stale value of plane_state->visible causes a subsequent modeset to
>>> enable
>>> the primary again?
>> Probably not because it would get recalculated in calc_changes, but it should
>> really be set to false afterwards.
> So basically I didn't understand how the wrong value of plane_state->visible
> causes the bug that was mentioned. I think a brief explanation in the commit
> message would be good.
Well, apply with git am --scissors. Same patch but mentioning this.

Is this better?
---->8-----

When disable_noatomic is called plane_mask is not correct yet,
and plane_state->visible = true is left as true after disabling
the primary plane.

Other planes are already disabled as part of crtc sanitization, only the
primary is left active. But the plane_mask is not updated here. It gets updated
during fb takeover in modeset_gem_init, or set to the new value on resume.

This means that to disable the primary plane 1 << drm_plane_index(primary)
needs to be used.

Afterwards because the crtc is no longer active it's forbidden to keep
plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
triggers. There are other code points that rely on accurate plane_state->visible
too, so make sure the bool is cleared.


The other planes are already disabled in intel_sanitize_crtc, so
they don't have to be handled here.

Cc: stable@vger.kernel.org #v4.3, v4.2?
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493213b7..bc3282ab5ed2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		WARN_ON(intel_crtc->unpin_work);
 
 		intel_pre_disable_primary(crtc);
+
+		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
+		to_intel_plane_state(crtc->primary->state)->visible = false;
 	}
 
-	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
 	dev_priv->display.crtc_disable(crtc);
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
-- 2.1.0

[PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Maarten Lankhorst]

Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>>>> When disable_noatomic is called plane_mask is not reliable yet,
>>>> and plane_state->visible = true even after disabling the primary plane.
>>> So the stale value of plane_state->visible causes a subsequent modeset to
>>> enable
>>> the primary again?
>> Probably not because it would get recalculated in calc_changes, but it should
>> really be set to false afterwards.
> So basically I didn't understand how the wrong value of plane_state->visible
> causes the bug that was mentioned. I think a brief explanation in the commit
> message would be good.
Well, apply with git am --scissors. Same patch but mentioning this.

Is this better?
---->8-----

When disable_noatomic is called plane_mask is not correct yet,
and plane_state->visible = true is left as true after disabling
the primary plane.

Other planes are already disabled as part of crtc sanitization, only the
primary is left active. But the plane_mask is not updated here. It gets updated
during fb takeover in modeset_gem_init, or set to the new value on resume.

This means that to disable the primary plane 1 << drm_plane_index(primary)
needs to be used.

Afterwards because the crtc is no longer active it's forbidden to keep
plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
triggers. There are other code points that rely on accurate plane_state->visible
too, so make sure the bool is cleared.


The other planes are already disabled in intel_sanitize_crtc, so
they don't have to be handled here.

Cc: stable@vger.kernel.org #v4.3, v4.2?
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493213b7..bc3282ab5ed2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		WARN_ON(intel_crtc->unpin_work);
 
 		intel_pre_disable_primary(crtc);
+
+		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
+		to_intel_plane_state(crtc->primary->state)->visible = false;
 	}
 
-	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
 	dev_priv->display.crtc_disable(crtc);
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
-- 2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Jani Nikula]

On Mon, 23 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
>> On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>>> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>>>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>>>>> When disable_noatomic is called plane_mask is not reliable yet,
>>>>> and plane_state->visible = true even after disabling the primary plane.
>>>> So the stale value of plane_state->visible causes a subsequent modeset to
>>>> enable
>>>> the primary again?
>>> Probably not because it would get recalculated in calc_changes, but it should
>>> really be set to false afterwards.
>> So basically I didn't understand how the wrong value of plane_state->visible
>> causes the bug that was mentioned. I think a brief explanation in the commit
>> message would be good.
> Well, apply with git am --scissors. Same patch but mentioning this.
>
> Is this better?

Ander, ping?

BR,
Jani.

> ---->8-----
>
> When disable_noatomic is called plane_mask is not correct yet,
> and plane_state->visible = true is left as true after disabling
> the primary plane.
>
> Other planes are already disabled as part of crtc sanitization, only the
> primary is left active. But the plane_mask is not updated here. It gets updated
> during fb takeover in modeset_gem_init, or set to the new value on resume.
>
> This means that to disable the primary plane 1 << drm_plane_index(primary)
> needs to be used.
>
> Afterwards because the crtc is no longer active it's forbidden to keep
> plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
> triggers. There are other code points that rely on accurate plane_state->visible
> too, so make sure the bool is cleared.
>
>
> The other planes are already disabled in intel_sanitize_crtc, so
> they don't have to be handled here.
>
> Cc: stable@vger.kernel.org #v4.3, v4.2?
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..bc3282ab5ed2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  		WARN_ON(intel_crtc->unpin_work);
>  
>  		intel_pre_disable_primary(crtc);
> +
> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>  	}
>  
> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>  	dev_priv->display.crtc_disable(crtc);
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> -- 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Tomas M]

[-- Attachment #1.1: Type: text/plain, Size: 3601 bytes --]

Hello,

Is there any special reason this has not been merged yet?

Regards

Tomas

On Tue, Dec 1, 2015 at 6:07 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Mon, 23 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> wrote:
> > Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
> >> On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
> >>> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
> >>>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
> >>>>> When disable_noatomic is called plane_mask is not reliable yet,
> >>>>> and plane_state->visible = true even after disabling the primary
> plane.
> >>>> So the stale value of plane_state->visible causes a subsequent
> modeset to
> >>>> enable
> >>>> the primary again?
> >>> Probably not because it would get recalculated in calc_changes, but it
> should
> >>> really be set to false afterwards.
> >> So basically I didn't understand how the wrong value of
> plane_state->visible
> >> causes the bug that was mentioned. I think a brief explanation in the
> commit
> >> message would be good.
> > Well, apply with git am --scissors. Same patch but mentioning this.
> >
> > Is this better?
>
> Ander, ping?
>
> BR,
> Jani.
>
> > ---->8-----
> >
> > When disable_noatomic is called plane_mask is not correct yet,
> > and plane_state->visible = true is left as true after disabling
> > the primary plane.
> >
> > Other planes are already disabled as part of crtc sanitization, only the
> > primary is left active. But the plane_mask is not updated here. It gets
> updated
> > during fb takeover in modeset_gem_init, or set to the new value on
> resume.
> >
> > This means that to disable the primary plane 1 <<
> drm_plane_index(primary)
> > needs to be used.
> >
> > Afterwards because the crtc is no longer active it's forbidden to keep
> > plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
> > triggers. There are other code points that rely on accurate
> plane_state->visible
> > too, so make sure the bool is cleared.
> >
> >
> > The other planes are already disabled in intel_sanitize_crtc, so
> > they don't have to be handled here.
> >
> > Cc: stable@vger.kernel.org #v4.3, v4.2?
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
> > Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index b5f7493213b7..bc3282ab5ed2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct
> drm_crtc *crtc)
> >               WARN_ON(intel_crtc->unpin_work);
> >
> >               intel_pre_disable_primary(crtc);
> > +
> > +             intel_crtc_disable_planes(crtc, 1 <<
> drm_plane_index(crtc->primary));
> > +             to_intel_plane_state(crtc->primary->state)->visible =
> false;
> >       }
> >
> > -     intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
> >       dev_priv->display.crtc_disable(crtc);
> >       intel_crtc->active = false;
> >       intel_update_watermarks(crtc);
> > -- 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

[-- Attachment #1.2: Type: text/html, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Jani Nikula]

On Wed, 09 Dec 2015, Tomas M <tmezzadra@gmail.com> wrote:
> Hello,
>
> Is there any special reason this has not been merged yet?

No special reason. There's just the ordinary reason there is no review
on the patch yet.

BR,
Jani.



>
> Regards
>
> Tomas
>
> On Tue, Dec 1, 2015 at 6:07 AM, Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>
>> On Mon, 23 Nov 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> wrote:
>> > Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
>> >> On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>> >>> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>> >>>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>> >>>>> When disable_noatomic is called plane_mask is not reliable yet,
>> >>>>> and plane_state->visible = true even after disabling the primary
>> plane.
>> >>>> So the stale value of plane_state->visible causes a subsequent
>> modeset to
>> >>>> enable
>> >>>> the primary again?
>> >>> Probably not because it would get recalculated in calc_changes, but it
>> should
>> >>> really be set to false afterwards.
>> >> So basically I didn't understand how the wrong value of
>> plane_state->visible
>> >> causes the bug that was mentioned. I think a brief explanation in the
>> commit
>> >> message would be good.
>> > Well, apply with git am --scissors. Same patch but mentioning this.
>> >
>> > Is this better?
>>
>> Ander, ping?
>>
>> BR,
>> Jani.
>>
>> > ---->8-----
>> >
>> > When disable_noatomic is called plane_mask is not correct yet,
>> > and plane_state->visible = true is left as true after disabling
>> > the primary plane.
>> >
>> > Other planes are already disabled as part of crtc sanitization, only the
>> > primary is left active. But the plane_mask is not updated here. It gets
>> updated
>> > during fb takeover in modeset_gem_init, or set to the new value on
>> resume.
>> >
>> > This means that to disable the primary plane 1 <<
>> drm_plane_index(primary)
>> > needs to be used.
>> >
>> > Afterwards because the crtc is no longer active it's forbidden to keep
>> > plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
>> > triggers. There are other code points that rely on accurate
>> plane_state->visible
>> > too, so make sure the bool is cleared.
>> >
>> >
>> > The other planes are already disabled in intel_sanitize_crtc, so
>> > they don't have to be handled here.
>> >
>> > Cc: stable@vger.kernel.org #v4.3, v4.2?
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
>> > Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
>> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> > index b5f7493213b7..bc3282ab5ed2 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct
>> drm_crtc *crtc)
>> >               WARN_ON(intel_crtc->unpin_work);
>> >
>> >               intel_pre_disable_primary(crtc);
>> > +
>> > +             intel_crtc_disable_planes(crtc, 1 <<
>> drm_plane_index(crtc->primary));
>> > +             to_intel_plane_state(crtc->primary->state)->visible =
>> false;
>> >       }
>> >
>> > -     intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>> >       dev_priv->display.crtc_disable(crtc);
>> >       intel_crtc->active = false;
>> >       intel_update_watermarks(crtc);
>> > -- 2.1.0
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Daniel Vetter]

On Mon, Nov 23, 2015 at 10:25:28AM +0100, Maarten Lankhorst wrote:
> Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
> > On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
> >> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
> >>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
> >>>> When disable_noatomic is called plane_mask is not reliable yet,
> >>>> and plane_state->visible = true even after disabling the primary plane.
> >>> So the stale value of plane_state->visible causes a subsequent modeset to
> >>> enable
> >>> the primary again?
> >> Probably not because it would get recalculated in calc_changes, but it should
> >> really be set to false afterwards.
> > So basically I didn't understand how the wrong value of plane_state->visible
> > causes the bug that was mentioned. I think a brief explanation in the commit
> > message would be good.
> Well, apply with git am --scissors. Same patch but mentioning this.
> 
> Is this better?
> ---->8-----
> 
> When disable_noatomic is called plane_mask is not correct yet,
> and plane_state->visible = true is left as true after disabling
> the primary plane.
> 
> Other planes are already disabled as part of crtc sanitization, only the
> primary is left active. But the plane_mask is not updated here. It gets updated
> during fb takeover in modeset_gem_init, or set to the new value on resume.
> 
> This means that to disable the primary plane 1 << drm_plane_index(primary)
> needs to be used.
> 
> Afterwards because the crtc is no longer active it's forbidden to keep
> plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
> triggers. There are other code points that rely on accurate plane_state->visible
> too, so make sure the bool is cleared.

Oh dear is this a mess :(

As a bugfix this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But we need to bring a _lot_ more sanity to this in -next. Would probably
be better if we keep the primary plane state all consistent, and then
clear both visible and plane_mask when fb reconstruction fails.

Also, some WARN_ON to bitch about inconsistency between plane_mask and
->visible in the disable code paths would be good I think.
-Daniel

> 
> 
> The other planes are already disabled in intel_sanitize_crtc, so
> they don't have to be handled here.
> 
> Cc: stable@vger.kernel.org #v4.3, v4.2?
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5f7493213b7..bc3282ab5ed2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  		WARN_ON(intel_crtc->unpin_work);
>  
>  		intel_pre_disable_primary(crtc);
> +
> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>  	}
>  
> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>  	dev_priv->display.crtc_disable(crtc);
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> -- 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Jani Nikula]

On Thu, 10 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Nov 23, 2015 at 10:25:28AM +0100, Maarten Lankhorst wrote:
>> Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
>> > On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>> >> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>> >>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>> >>>> When disable_noatomic is called plane_mask is not reliable yet,
>> >>>> and plane_state->visible = true even after disabling the primary plane.
>> >>> So the stale value of plane_state->visible causes a subsequent modeset to
>> >>> enable
>> >>> the primary again?
>> >> Probably not because it would get recalculated in calc_changes, but it should
>> >> really be set to false afterwards.
>> > So basically I didn't understand how the wrong value of plane_state->visible
>> > causes the bug that was mentioned. I think a brief explanation in the commit
>> > message would be good.
>> Well, apply with git am --scissors. Same patch but mentioning this.
>> 
>> Is this better?
>> ---->8-----
>> 
>> When disable_noatomic is called plane_mask is not correct yet,
>> and plane_state->visible = true is left as true after disabling
>> the primary plane.
>> 
>> Other planes are already disabled as part of crtc sanitization, only the
>> primary is left active. But the plane_mask is not updated here. It gets updated
>> during fb takeover in modeset_gem_init, or set to the new value on resume.
>> 
>> This means that to disable the primary plane 1 << drm_plane_index(primary)
>> needs to be used.
>> 
>> Afterwards because the crtc is no longer active it's forbidden to keep
>> plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
>> triggers. There are other code points that rely on accurate plane_state->visible
>> too, so make sure the bool is cleared.
>
> Oh dear is this a mess :(
>
> As a bugfix this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.

>
> But we need to bring a _lot_ more sanity to this in -next. Would probably
> be better if we keep the primary plane state all consistent, and then
> clear both visible and plane_mask when fb reconstruction fails.
>
> Also, some WARN_ON to bitch about inconsistency between plane_mask and
> ->visible in the disable code paths would be good I think.
> -Daniel
>
>> 
>> 
>> The other planes are already disabled in intel_sanitize_crtc, so
>> they don't have to be handled here.
>> 
>> Cc: stable@vger.kernel.org #v4.3, v4.2?
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
>> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b5f7493213b7..bc3282ab5ed2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>>  		WARN_ON(intel_crtc->unpin_work);
>>  
>>  		intel_pre_disable_primary(crtc);
>> +
>> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
>> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>>  	}
>>  
>> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>>  	dev_priv->display.crtc_disable(crtc);
>>  	intel_crtc->active = false;
>>  	intel_update_watermarks(crtc);
>> -- 2.1.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.

[Jani Nikula]

On Thu, 10 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Nov 23, 2015 at 10:25:28AM +0100, Maarten Lankhorst wrote:
>> Op 13-11-15 om 14:28 schreef Ander Conselvan De Oliveira:
>> > On Thu, 2015-11-12 at 14:58 +0100, Maarten Lankhorst wrote:
>> >> Op 12-11-15 om 14:37 schreef Ander Conselvan De Oliveira:
>> >>> On Wed, 2015-11-11 at 15:36 +0100, Maarten Lankhorst wrote:
>> >>>> When disable_noatomic is called plane_mask is not reliable yet,
>> >>>> and plane_state->visible = true even after disabling the primary plane.
>> >>> So the stale value of plane_state->visible causes a subsequent modeset to
>> >>> enable
>> >>> the primary again?
>> >> Probably not because it would get recalculated in calc_changes, but it should
>> >> really be set to false afterwards.
>> > So basically I didn't understand how the wrong value of plane_state->visible
>> > causes the bug that was mentioned. I think a brief explanation in the commit
>> > message would be good.
>> Well, apply with git am --scissors. Same patch but mentioning this.
>> 
>> Is this better?
>> ---->8-----
>> 
>> When disable_noatomic is called plane_mask is not correct yet,
>> and plane_state->visible = true is left as true after disabling
>> the primary plane.
>> 
>> Other planes are already disabled as part of crtc sanitization, only the
>> primary is left active. But the plane_mask is not updated here. It gets updated
>> during fb takeover in modeset_gem_init, or set to the new value on resume.
>> 
>> This means that to disable the primary plane 1 << drm_plane_index(primary)
>> needs to be used.
>> 
>> Afterwards because the crtc is no longer active it's forbidden to keep
>> plane_state->visible set, or a WARN_ON in intel_plane_atomic_calc_changes
>> triggers. There are other code points that rely on accurate plane_state->visible
>> too, so make sure the bool is cleared.
>
> Oh dear is this a mess :(
>
> As a bugfix this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.

>
> But we need to bring a _lot_ more sanity to this in -next. Would probably
> be better if we keep the primary plane state all consistent, and then
> clear both visible and plane_mask when fb reconstruction fails.
>
> Also, some WARN_ON to bitch about inconsistency between plane_mask and
> ->visible in the disable code paths would be good I think.
> -Daniel
>
>> 
>> 
>> The other planes are already disabled in intel_sanitize_crtc, so
>> they don't have to be handled here.
>> 
>> Cc: stable@vger.kernel.org #v4.3, v4.2?
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92655
>> Tested-by: Tomas Mezzadra <tmezzadra@gmail.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b5f7493213b7..bc3282ab5ed2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6267,9 +6267,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>>  		WARN_ON(intel_crtc->unpin_work);
>>  
>>  		intel_pre_disable_primary(crtc);
>> +
>> +		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
>> +		to_intel_plane_state(crtc->primary->state)->visible = false;
>>  	}
>>  
>> -	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>>  	dev_priv->display.crtc_disable(crtc);
>>  	intel_crtc->active = false;
>>  	intel_update_watermarks(crtc);
>> -- 2.1.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx