[PATCH] drm/i915: reset drm state backpointer in crtc_state

[PATCH] drm/i915: reset drm state backpointer in crtc_state

[Chandra Konduru]

At end of intel_crtc_set_config, reset crtc_state's
drm_state back pointer to null.

Signed-off-by: Chandra Konduru <chandra.konduru@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 c84926b..f9c2e4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12451,8 +12451,10 @@ fail:
 	}
 
 out_config:
-	if (state)
+	if (state) {
 		drm_atomic_state_free(state);
+		to_intel_crtc(set->crtc)->config->base.state = NULL;
+	}
 
 	intel_set_config_free(config);
 	return ret;
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6132
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  272/272              272/272
ILK                 -1              302/302              301/302
SNB                                  303/303              303/303
IVB                 -1              338/338              337/338
BYT                 -1              287/287              286/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_pipe_crc_basic@bad-nb-words-3      PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:drm_edid_block_valid[drm]]*ERROR*EDID_checksum_is_invalid,remainder_is@EDID checksum is .* remainder is
*IVB  igt@gem_pwrite_pread@snooped-pwrite-blt-cpu_mmap-performance      PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
*BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(19)      FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[Jani Nikula]

On Mon, 06 Apr 2015, Chandra Konduru <chandra.konduru@intel.com> wrote:
> At end of intel_crtc_set_config, reset crtc_state's
> drm_state back pointer to null.

This does not tell me anything that reading the patch already
didn't. Please explain *why* this is needed in the commit message. What
breaks without it? If this fixes a regression, please indicate which
commit regressed.

BR,
Jani.

>
> Signed-off-by: Chandra Konduru <chandra.konduru@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 c84926b..f9c2e4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12451,8 +12451,10 @@ fail:
>  	}
>  
>  out_config:
> -	if (state)
> +	if (state) {
>  		drm_atomic_state_free(state);
> +		to_intel_crtc(set->crtc)->config->base.state = NULL;
> +	}
>  
>  	intel_set_config_free(config);
>  	return ret;
> -- 
> 1.7.9.5
>
> _______________________________________________
> 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

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Tuesday, April 07, 2015 2:02 AM
> To: Konduru, Chandra; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: reset drm state backpointer in
> crtc_state
> 
> On Mon, 06 Apr 2015, Chandra Konduru <chandra.konduru@intel.com> wrote:
> > At end of intel_crtc_set_config, reset crtc_state's drm_state back
> > pointer to null.
> 
> This does not tell me anything that reading the patch already didn't. Please
> explain *why* this is needed in the commit message. What breaks without it? If
> this fixes a regression, please indicate which commit regressed.

Once atomic transaction is done, live crtc_state (i.e., intel_crtc->config) is 
carrying back pointer to drm_atomic_state which is freed. When a new 
non-atomic transaction is made (crtc_disable triggered off from set_mode), 
this stale pointer is carried into that transaction. 
Depending on timing, this causes issue to scaler feature that I am working 
if panel fit to be disabled during crtc_disable.

It has potential to cause issues to wm work that Matt is doing, but not sure.

> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@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 c84926b..f9c2e4d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12451,8 +12451,10 @@ fail:
> >  	}
> >
> >  out_config:
> > -	if (state)
> > +	if (state) {
> >  		drm_atomic_state_free(state);
> > +		to_intel_crtc(set->crtc)->config->base.state = NULL;
> > +	}
> >
> >  	intel_set_config_free(config);
> >  	return ret;
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > 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

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[Daniel Vetter]

On Tue, Apr 07, 2015 at 06:48:15PM +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > Sent: Tuesday, April 07, 2015 2:02 AM
> > To: Konduru, Chandra; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: reset drm state backpointer in
> > crtc_state
> > 
> > On Mon, 06 Apr 2015, Chandra Konduru <chandra.konduru@intel.com> wrote:
> > > At end of intel_crtc_set_config, reset crtc_state's drm_state back
> > > pointer to null.
> > 
> > This does not tell me anything that reading the patch already didn't. Please
> > explain *why* this is needed in the commit message. What breaks without it? If
> > this fixes a regression, please indicate which commit regressed.
> 
> Once atomic transaction is done, live crtc_state (i.e., intel_crtc->config) is 
> carrying back pointer to drm_atomic_state which is freed. When a new 
> non-atomic transaction is made (crtc_disable triggered off from set_mode), 
> this stale pointer is carried into that transaction. 
> Depending on timing, this causes issue to scaler feature that I am working 
> if panel fit to be disabled during crtc_disable.

You shouldn't ever be chasing the state backpointer of a committed state.
It's ok to clean it up, but chasing that pointer itself is a bug. Why does
the scaler code look at that pointer?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[Daniel Vetter]

On Mon, Apr 06, 2015 at 11:30:29AM -0700, Chandra Konduru wrote:
> At end of intel_crtc_set_config, reset crtc_state's
> drm_state back pointer to null.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@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 c84926b..f9c2e4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12451,8 +12451,10 @@ fail:
>  	}
>  
>  out_config:
> -	if (state)
> +	if (state) {
>  		drm_atomic_state_free(state);
> +		to_intel_crtc(set->crtc)->config->base.state = NULL;

This should be done in intel_crtc_set_state instead of just in this case
here.
-Daniel

> +	}
>  
>  	intel_set_config_free(config);
>  	return ret;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: reset drm state backpointer in crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, April 08, 2015 1:20 AM
> To: Konduru, Chandra
> Cc: Jani Nikula; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: reset drm state backpointer in
> crtc_state
> 
> On Tue, Apr 07, 2015 at 06:48:15PM +0000, Konduru, Chandra wrote:
> >
> > > -----Original Message-----
> > > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > > Sent: Tuesday, April 07, 2015 2:02 AM
> > > To: Konduru, Chandra; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: reset drm state
> > > backpointer in crtc_state
> > >
> > > On Mon, 06 Apr 2015, Chandra Konduru <chandra.konduru@intel.com>
> wrote:
> > > > At end of intel_crtc_set_config, reset crtc_state's drm_state back
> > > > pointer to null.
> > >
> > > This does not tell me anything that reading the patch already
> > > didn't. Please explain *why* this is needed in the commit message.
> > > What breaks without it? If this fixes a regression, please indicate which
> commit regressed.
> >
> > Once atomic transaction is done, live crtc_state (i.e.,
> > intel_crtc->config) is carrying back pointer to drm_atomic_state which
> > is freed. When a new non-atomic transaction is made (crtc_disable
> > triggered off from set_mode), this stale pointer is carried into that transaction.
> > Depending on timing, this causes issue to scaler feature that I am
> > working if panel fit to be disabled during crtc_disable.
> 
> You shouldn't ever be chasing the state backpointer of a committed state.
> It's ok to clean it up, but chasing that pointer itself is a bug. Why does the scaler
> code look at that pointer?
While setting up scalers, it looks at that pointer to know other scaler users.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx