| Message ID | 1428345029-13880-1-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 '*'
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
> -----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
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
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
> -----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
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;
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(-)