[PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Mika Kuoppala]

CI/Bat got following (shortened) trace on byt and also
on bsw:

------------[ cut here ]-----------
Unclaimed register detected before reading register 0x186500
Call Trace:
 __unclaimed_reg_debug+0x68/0x80 [i915]
vlv_read32+0x2de/0x370 [i915]
intel_set_memory_cxsr+0x87/0x1a0 [i915]
intel_pre_plane_update+0xb3/0xf0 [i915]
intel_atomic_commit+0x3b5/0x17c0 [i915]
...
---[ end trace 6387a0ad001bb39f ]---

Fix this by limiting pre plane update only to active crtcs.

References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index aa24f79d85bf..a134a698d97d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		intel_pre_plane_update(intel_crtc);
-
 		if (crtc_state->active) {
+			intel_pre_plane_update(intel_crtc);
 			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
-- 
2.5.0

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

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Ville Syrjälä]

On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
> CI/Bat got following (shortened) trace on byt and also
> on bsw:
> 
> ------------[ cut here ]-----------
> Unclaimed register detected before reading register 0x186500
> Call Trace:
>  __unclaimed_reg_debug+0x68/0x80 [i915]
> vlv_read32+0x2de/0x370 [i915]
> intel_set_memory_cxsr+0x87/0x1a0 [i915]
> intel_pre_plane_update+0xb3/0xf0 [i915]
> intel_atomic_commit+0x3b5/0x17c0 [i915]
> ...
> ---[ end trace 6387a0ad001bb39f ]---
> 
> Fix this by limiting pre plane update only to active crtcs.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index aa24f79d85bf..a134a698d97d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> -		intel_pre_plane_update(intel_crtc);
> -
>  		if (crtc_state->active) {
> +			intel_pre_plane_update(intel_crtc);

I think you'll want to deal with the other one too (the one in the crtc
enable/plane update path). Actually I think the plane update stuff
should be split into a separate loop from the crtc_enable stuff, but
that's a separate topic.

Hmm. And there's a post_plane_update there that looks a bit too
lonely as well. Something really should be done to make this code
less convoluted. A modeset/plane update shouldn't be this hard to
get right.

>  			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
>  			dev_priv->display.crtc_disable(crtc);
>  			intel_crtc->active = false;
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 8fb2feecca499d11e104264071ac55e273e23af5 drm-intel-nightly: 2016y-01m-14d-13h-06m-44s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2) UNSTABLE
                dmesg-warn -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (snb-x220t)

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:101  dwarn:3   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:132  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1192/

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

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Matt Roper]

On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
> CI/Bat got following (shortened) trace on byt and also
> on bsw:
> 
> ------------[ cut here ]-----------
> Unclaimed register detected before reading register 0x186500
> Call Trace:
>  __unclaimed_reg_debug+0x68/0x80 [i915]
> vlv_read32+0x2de/0x370 [i915]
> intel_set_memory_cxsr+0x87/0x1a0 [i915]
> intel_pre_plane_update+0xb3/0xf0 [i915]
> intel_atomic_commit+0x3b5/0x17c0 [i915]
> ...
> ---[ end trace 6387a0ad001bb39f ]---
> 
> Fix this by limiting pre plane update only to active crtcs.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index aa24f79d85bf..a134a698d97d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> -		intel_pre_plane_update(intel_crtc);
> -
>  		if (crtc_state->active) {
> +			intel_pre_plane_update(intel_crtc);

Won't this change prevent us from setting up watermarks before turning
on the CRTC in a disabled->enabled transition?


Matt

>  			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
>  			dev_priv->display.crtc_disable(crtc);
>  			intel_crtc->active = false;
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Maarten Lankhorst]

Op 14-01-16 om 17:52 schreef Ville Syrjälä:
> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
>> CI/Bat got following (shortened) trace on byt and also
>> on bsw:
>>
>> ------------[ cut here ]-----------
>> Unclaimed register detected before reading register 0x186500
>> Call Trace:
>>  __unclaimed_reg_debug+0x68/0x80 [i915]
>> vlv_read32+0x2de/0x370 [i915]
>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
>> intel_pre_plane_update+0xb3/0xf0 [i915]
>> intel_atomic_commit+0x3b5/0x17c0 [i915]
>> ...
>> ---[ end trace 6387a0ad001bb39f ]---
>>
>> Fix this by limiting pre plane update only to active crtcs.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index aa24f79d85bf..a134a698d97d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  		if (!needs_modeset(crtc->state))
>>  			continue;
>>  
>> -		intel_pre_plane_update(intel_crtc);
>> -
>>  		if (crtc_state->active) {
>> +			intel_pre_plane_update(intel_crtc);
> I think you'll want to deal with the other one too (the one in the crtc
> enable/plane update path). Actually I think the plane update stuff
> should be split into a separate loop from the crtc_enable stuff, but
> that's a separate topic.
>
> Hmm. And there's a post_plane_update there that looks a bit too
> lonely as well. Something really should be done to make this code
> less convoluted. A modeset/plane update shouldn't be this hard to
> get right.
>
>
I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?

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

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Mika Kuoppala]

Maarten Lankhorst <maarten.lankhorst@linux.intel.com> writes:

> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
>> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
>>> CI/Bat got following (shortened) trace on byt and also
>>> on bsw:
>>>
>>> ------------[ cut here ]-----------
>>> Unclaimed register detected before reading register 0x186500
>>> Call Trace:
>>>  __unclaimed_reg_debug+0x68/0x80 [i915]
>>> vlv_read32+0x2de/0x370 [i915]
>>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
>>> intel_pre_plane_update+0xb3/0xf0 [i915]
>>> intel_atomic_commit+0x3b5/0x17c0 [i915]
>>> ...
>>> ---[ end trace 6387a0ad001bb39f ]---
>>>
>>> Fix this by limiting pre plane update only to active crtcs.
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index aa24f79d85bf..a134a698d97d 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>  		if (!needs_modeset(crtc->state))
>>>  			continue;
>>>  
>>> -		intel_pre_plane_update(intel_crtc);
>>> -
>>>  		if (crtc_state->active) {
>>> +			intel_pre_plane_update(intel_crtc);
>> I think you'll want to deal with the other one too (the one in the crtc
>> enable/plane update path). Actually I think the plane update stuff
>> should be split into a separate loop from the crtc_enable stuff, but
>> that's a separate topic.
>>
>> Hmm. And there's a post_plane_update there that looks a bit too
>> lonely as well. Something really should be done to make this code
>> less convoluted. A modeset/plane update shouldn't be this hard to
>> get right.
>>
>>
> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?
>

intel_set_memory_cxsr() did the access with all the power wells off.

So perhaps my commit message was off. Don't know if crtc
state is what matters, but evidence points to the powers
being off regardless.

-Mika

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

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Ville Syrjälä]

On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote:
> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
> > On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
> >> CI/Bat got following (shortened) trace on byt and also
> >> on bsw:
> >>
> >> ------------[ cut here ]-----------
> >> Unclaimed register detected before reading register 0x186500
> >> Call Trace:
> >>  __unclaimed_reg_debug+0x68/0x80 [i915]
> >> vlv_read32+0x2de/0x370 [i915]
> >> intel_set_memory_cxsr+0x87/0x1a0 [i915]
> >> intel_pre_plane_update+0xb3/0xf0 [i915]
> >> intel_atomic_commit+0x3b5/0x17c0 [i915]
> >> ...
> >> ---[ end trace 6387a0ad001bb39f ]---
> >>
> >> Fix this by limiting pre plane update only to active crtcs.
> >>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index aa24f79d85bf..a134a698d97d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>  		if (!needs_modeset(crtc->state))
> >>  			continue;
> >>  
> >> -		intel_pre_plane_update(intel_crtc);
> >> -
> >>  		if (crtc_state->active) {
> >> +			intel_pre_plane_update(intel_crtc);
> > I think you'll want to deal with the other one too (the one in the crtc
> > enable/plane update path). Actually I think the plane update stuff
> > should be split into a separate loop from the crtc_enable stuff, but
> > that's a separate topic.
> >
> > Hmm. And there's a post_plane_update there that looks a bit too
> > lonely as well. Something really should be done to make this code
> > less convoluted. A modeset/plane update shouldn't be this hard to
> > get right.
> >
> >
> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?

Staring at the code a bit. I would assume it's due to
intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true'
when the plane gets either turned off or on.

This sort of stuff makes me wish again that we had a separate atomic state
for the disable case. Using the same state flag for both the disable
and enable phases of the operation is very confusing. Would be even
more confusing if we required that flag to have different values for the
disable and enable phases.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Maarten Lankhorst]

Op 19-01-16 om 20:22 schreef Ville Syrjälä:
> On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote:
>> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
>>> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
>>>> CI/Bat got following (shortened) trace on byt and also
>>>> on bsw:
>>>>
>>>> ------------[ cut here ]-----------
>>>> Unclaimed register detected before reading register 0x186500
>>>> Call Trace:
>>>>  __unclaimed_reg_debug+0x68/0x80 [i915]
>>>> vlv_read32+0x2de/0x370 [i915]
>>>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
>>>> intel_pre_plane_update+0xb3/0xf0 [i915]
>>>> intel_atomic_commit+0x3b5/0x17c0 [i915]
>>>> ...
>>>> ---[ end trace 6387a0ad001bb39f ]---
>>>>
>>>> Fix this by limiting pre plane update only to active crtcs.
>>>>
>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index aa24f79d85bf..a134a698d97d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>  		if (!needs_modeset(crtc->state))
>>>>  			continue;
>>>>  
>>>> -		intel_pre_plane_update(intel_crtc);
>>>> -
>>>>  		if (crtc_state->active) {
>>>> +			intel_pre_plane_update(intel_crtc);
>>> I think you'll want to deal with the other one too (the one in the crtc
>>> enable/plane update path). Actually I think the plane update stuff
>>> should be split into a separate loop from the crtc_enable stuff, but
>>> that's a separate topic.
>>>
>>> Hmm. And there's a post_plane_update there that looks a bit too
>>> lonely as well. Something really should be done to make this code
>>> less convoluted. A modeset/plane update shouldn't be this hard to
>>> get right.
>>>
>>>
>> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?
> Staring at the code a bit. I would assume it's due to
> intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true'
> when the plane gets either turned off or on.
>
> This sort of stuff makes me wish again that we had a separate atomic state
> for the disable case. Using the same state flag for both the disable
> and enable phases of the operation is very confusing. Would be even
> more confusing if we required that flag to have different values for the
> disable and enable phases.
But cxsr_allowed still needs to be false. Maybe this?

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5abe97c1a944..bfdf02f7436b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4830,7 +4830,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 
 	if (pipe_config->disable_cxsr) {
 		crtc->wm.cxsr_allowed = false;
-		intel_set_memory_cxsr(dev_priv, false);
+		if (!needs_modeset(&pipe_config->base))
+			intel_set_memory_cxsr(dev_priv, false);
 	}
 
 	/*

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

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Ville Syrjälä]

On Wed, Jan 20, 2016 at 08:18:03AM +0100, Maarten Lankhorst wrote:
> Op 19-01-16 om 20:22 schreef Ville Syrjälä:
> > On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote:
> >> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
> >>> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
> >>>> CI/Bat got following (shortened) trace on byt and also
> >>>> on bsw:
> >>>>
> >>>> ------------[ cut here ]-----------
> >>>> Unclaimed register detected before reading register 0x186500
> >>>> Call Trace:
> >>>>  __unclaimed_reg_debug+0x68/0x80 [i915]
> >>>> vlv_read32+0x2de/0x370 [i915]
> >>>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
> >>>> intel_pre_plane_update+0xb3/0xf0 [i915]
> >>>> intel_atomic_commit+0x3b5/0x17c0 [i915]
> >>>> ...
> >>>> ---[ end trace 6387a0ad001bb39f ]---
> >>>>
> >>>> Fix this by limiting pre plane update only to active crtcs.
> >>>>
> >>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >>>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index aa24f79d85bf..a134a698d97d 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>>>  		if (!needs_modeset(crtc->state))
> >>>>  			continue;
> >>>>  
> >>>> -		intel_pre_plane_update(intel_crtc);
> >>>> -
> >>>>  		if (crtc_state->active) {
> >>>> +			intel_pre_plane_update(intel_crtc);
> >>> I think you'll want to deal with the other one too (the one in the crtc
> >>> enable/plane update path). Actually I think the plane update stuff
> >>> should be split into a separate loop from the crtc_enable stuff, but
> >>> that's a separate topic.
> >>>
> >>> Hmm. And there's a post_plane_update there that looks a bit too
> >>> lonely as well. Something really should be done to make this code
> >>> less convoluted. A modeset/plane update shouldn't be this hard to
> >>> get right.
> >>>
> >>>
> >> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?
> > Staring at the code a bit. I would assume it's due to
> > intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true'
> > when the plane gets either turned off or on.
> >
> > This sort of stuff makes me wish again that we had a separate atomic state
> > for the disable case. Using the same state flag for both the disable
> > and enable phases of the operation is very confusing. Would be even
> > more confusing if we required that flag to have different values for the
> > disable and enable phases.
> But cxsr_allowed still needs to be false. Maybe this?
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5abe97c1a944..bfdf02f7436b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4830,7 +4830,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  
>  	if (pipe_config->disable_cxsr) {
>  		crtc->wm.cxsr_allowed = false;
> -		intel_set_memory_cxsr(dev_priv, false);
> +		if (!needs_modeset(&pipe_config->base))
> +			intel_set_memory_cxsr(dev_priv, false);

Hmm. Yeah, I suppose that could work. I fear thinking about it is going
to give me a headache. Would really need nice two-stage wm programming
for gmch platforms as well to handle it properly.

>  	}
>  
>  	/*

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Maarten Lankhorst]

Op 20-01-16 om 12:11 schreef Ville Syrjälä:
> On Wed, Jan 20, 2016 at 08:18:03AM +0100, Maarten Lankhorst wrote:
>> Op 19-01-16 om 20:22 schreef Ville Syrjälä:
>>> On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote:
>>>> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
>>>>> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
>>>>>> CI/Bat got following (shortened) trace on byt and also
>>>>>> on bsw:
>>>>>>
>>>>>> ------------[ cut here ]-----------
>>>>>> Unclaimed register detected before reading register 0x186500
>>>>>> Call Trace:
>>>>>>  __unclaimed_reg_debug+0x68/0x80 [i915]
>>>>>> vlv_read32+0x2de/0x370 [i915]
>>>>>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
>>>>>> intel_pre_plane_update+0xb3/0xf0 [i915]
>>>>>> intel_atomic_commit+0x3b5/0x17c0 [i915]
>>>>>> ...
>>>>>> ---[ end trace 6387a0ad001bb39f ]---
>>>>>>
>>>>>> Fix this by limiting pre plane update only to active crtcs.
>>>>>>
>>>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index aa24f79d85bf..a134a698d97d 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>>>  		if (!needs_modeset(crtc->state))
>>>>>>  			continue;
>>>>>>  
>>>>>> -		intel_pre_plane_update(intel_crtc);
>>>>>> -
>>>>>>  		if (crtc_state->active) {
>>>>>> +			intel_pre_plane_update(intel_crtc);
>>>>> I think you'll want to deal with the other one too (the one in the crtc
>>>>> enable/plane update path). Actually I think the plane update stuff
>>>>> should be split into a separate loop from the crtc_enable stuff, but
>>>>> that's a separate topic.
>>>>>
>>>>> Hmm. And there's a post_plane_update there that looks a bit too
>>>>> lonely as well. Something really should be done to make this code
>>>>> less convoluted. A modeset/plane update shouldn't be this hard to
>>>>> get right.
>>>>>
>>>>>
>>>> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?
>>> Staring at the code a bit. I would assume it's due to
>>> intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true'
>>> when the plane gets either turned off or on.
>>>
>>> This sort of stuff makes me wish again that we had a separate atomic state
>>> for the disable case. Using the same state flag for both the disable
>>> and enable phases of the operation is very confusing. Would be even
>>> more confusing if we required that flag to have different values for the
>>> disable and enable phases.
>> But cxsr_allowed still needs to be false. Maybe this?
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5abe97c1a944..bfdf02f7436b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4830,7 +4830,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>>  
>>  	if (pipe_config->disable_cxsr) {
>>  		crtc->wm.cxsr_allowed = false;
>> -		intel_set_memory_cxsr(dev_priv, false);
>> +		if (!needs_modeset(&pipe_config->base))
>> +			intel_set_memory_cxsr(dev_priv, false);
> Hmm. Yeah, I suppose that could work. I fear thinking about it is going
> to give me a headache. Would really need nice two-stage wm programming
> for gmch platforms as well to handle it properly.
>
Indeed, that would help a lot. Hopefully the ilk two-stage wm patch gets reapplied soon so we could convert gmch as well after..

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