* [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
@ 2015-11-11 14:36 Maarten Lankhorst
2015-11-12 13:37 ` [Intel-gfx] " Ander Conselvan De Oliveira
0 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2015-11-11 14:36 UTC (permalink / raw)
To: intel-gfx; +Cc: v4.2?, Tomas M., stable
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-11 14:36 [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case Maarten Lankhorst
@ 2015-11-12 13:37 ` Ander Conselvan De Oliveira
2015-11-12 13:58 ` Maarten Lankhorst
0 siblings, 1 reply; 12+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-11-12 13:37 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx; +Cc: Tomas M., stable
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);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-12 13:37 ` [Intel-gfx] " Ander Conselvan De Oliveira
@ 2015-11-12 13:58 ` Maarten Lankhorst
2015-11-13 13:28 ` Ander Conselvan De Oliveira
0 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2015-11-12 13:58 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, intel-gfx; +Cc: Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-12 13:58 ` Maarten Lankhorst
@ 2015-11-13 13:28 ` Ander Conselvan De Oliveira
2015-11-23 9:25 ` Maarten Lankhorst
0 siblings, 1 reply; 12+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-11-13 13:28 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx; +Cc: Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-13 13:28 ` Ander Conselvan De Oliveira
@ 2015-11-23 9:25 ` Maarten Lankhorst
0 siblings, 0 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2015-11-23 9:25 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, intel-gfx; +Cc: Tomas M., stable
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
@ 2015-11-23 9:25 ` Maarten Lankhorst
0 siblings, 0 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2015-11-23 9:25 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, intel-gfx; +Cc: Tomas M., stable
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-23 9:25 ` Maarten Lankhorst
(?)
@ 2015-12-01 9:07 ` Jani Nikula
2015-12-09 13:25 ` Tomas M
-1 siblings, 1 reply; 12+ messages in thread
From: Jani Nikula @ 2015-12-01 9:07 UTC (permalink / raw)
To: Maarten Lankhorst, Ander Conselvan De Oliveira, intel-gfx
Cc: Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-12-01 9:07 ` [Intel-gfx] " Jani Nikula
@ 2015-12-09 13:25 ` Tomas M
2015-12-09 14:22 ` [Intel-gfx] " Jani Nikula
0 siblings, 1 reply; 12+ messages in thread
From: Tomas M @ 2015-12-09 13:25 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, stable
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-12-09 13:25 ` Tomas M
@ 2015-12-09 14:22 ` Jani Nikula
0 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2015-12-09 14:22 UTC (permalink / raw)
To: Tomas M; +Cc: Maarten Lankhorst, Ander Conselvan De Oliveira, intel-gfx, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-11-23 9:25 ` Maarten Lankhorst
(?)
(?)
@ 2015-12-10 8:33 ` Daniel Vetter
2015-12-10 12:47 ` Jani Nikula
-1 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2015-12-10 8:33 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Ander Conselvan De Oliveira, intel-gfx, Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
2015-12-10 8:33 ` Daniel Vetter
@ 2015-12-10 12:47 ` Jani Nikula
0 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2015-12-10 12:47 UTC (permalink / raw)
To: Daniel Vetter, Maarten Lankhorst; +Cc: intel-gfx, Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] drm/i915: Do a better job at disabling primary plane in the noatomic case.
@ 2015-12-10 12:47 ` Jani Nikula
0 siblings, 0 replies; 12+ messages in thread
From: Jani Nikula @ 2015-12-10 12:47 UTC (permalink / raw)
To: Daniel Vetter, Maarten Lankhorst; +Cc: intel-gfx, Tomas M., stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-12-10 12:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-11 14:36 [PATCH] drm/i915: Do a better job at disabling primary plane in the noatomic case Maarten Lankhorst
2015-11-12 13:37 ` [Intel-gfx] " Ander Conselvan De Oliveira
2015-11-12 13:58 ` Maarten Lankhorst
2015-11-13 13:28 ` Ander Conselvan De Oliveira
2015-11-23 9:25 ` [PATCH v2] " Maarten Lankhorst
2015-11-23 9:25 ` Maarten Lankhorst
2015-12-01 9:07 ` [Intel-gfx] " Jani Nikula
2015-12-09 13:25 ` Tomas M
2015-12-09 14:22 ` [Intel-gfx] " Jani Nikula
2015-12-10 8:33 ` Daniel Vetter
2015-12-10 12:47 ` Jani Nikula
2015-12-10 12:47 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.