| Message ID | 1507789821-10235-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
HI, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > Of Mika Kahola > Sent: torstai 12. lokakuuta 2017 9.30 > To: intel-gfx@lists.freedesktop.org > Cc: ville.syrjala@intel.linux.com; Vivi, Rodrigo <rodrigo.vivi@intel.com> > Subject: [Intel-gfx] [PATCH] drm/i915: Cache max number of pipes > > CI system spotted an error on CFL system when running IGT tests with display > disabled. In this case, the 'INTEL_INFO(dev_priv)->num_pipes' > is set to 0. This will cause that we prematurely return from 'get_saved_enc()'. > > To fix this issue, the patch introduces a 'max_pipes' variable which caches > the maximum number of available pipes. > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103206 > Signed-off-by: Mika Kahola <mika.kahola@intel.com> This was also tested successfully on try-bot fixing issue seen, so maybe we can say tested-by: try-bot? > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_audio.c | 2 +- > drivers/gpu/drm/i915/intel_device_info.c | 2 ++ > 3 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h index 770305b..6229ff8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -868,6 +868,7 @@ struct intel_device_info { > u8 num_pipes; > u8 num_sprites[I915_MAX_PIPES]; > u8 num_scalers[I915_MAX_PIPES]; > + u8 max_pipes; > > unsigned int page_sizes; /* page sizes supported by the HW */ > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > index 0ddba16..147bd3d 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -754,7 +754,7 @@ static struct intel_encoder *get_saved_enc(struct > drm_i915_private *dev_priv, { > struct intel_encoder *encoder; > > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > + if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->max_pipes)) > return NULL; > > /* MST */ > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index 875d428..ba1a807 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -376,6 +376,8 @@ void intel_device_info_runtime_init(struct > drm_i915_private *dev_priv) > info->num_sprites[pipe] = 1; > } > > + info->max_pipes = info->num_pipes; > + > if (i915_modparams.disable_display) { > DRM_INFO("Display disabled (module parameter)\n"); > info->num_pipes = 0; > -- > 2.7.4 Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
On Thu, 12 Oct 2017, Mika Kahola <mika.kahola@intel.com> wrote: > CI system spotted an error on CFL system when running IGT tests with > display disabled. In this case, the 'INTEL_INFO(dev_priv)->num_pipes' > is set to 0. This will cause that we prematurely return from > 'get_saved_enc()'. > > To fix this issue, the patch introduces a 'max_pipes' variable which caches > the maximum number of available pipes. This is not the right fix. The audio component init should be skipped and thus all component calls blocked when num_pipes == 0. It's just that the num_pipes = 0 happens way too late when the display is fused off or disabled via modparam. This makes me wonder how many things we actually screw up because intel_device_info_runtime_init() happens too late. > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103206 If this patch fixes the bug, we use Bugzilla: tag. Reference: is for other references, e.g. the referenced bug is related to the patch at hand somehow, but this does not fix the bug. BR, Jani. > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_audio.c | 2 +- > drivers/gpu/drm/i915/intel_device_info.c | 2 ++ > 3 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 770305b..6229ff8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -868,6 +868,7 @@ struct intel_device_info { > u8 num_pipes; > u8 num_sprites[I915_MAX_PIPES]; > u8 num_scalers[I915_MAX_PIPES]; > + u8 max_pipes; > > unsigned int page_sizes; /* page sizes supported by the HW */ > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index 0ddba16..147bd3d 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -754,7 +754,7 @@ static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv, > { > struct intel_encoder *encoder; > > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > + if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->max_pipes)) > return NULL; > > /* MST */ > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 875d428..ba1a807 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -376,6 +376,8 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) > info->num_sprites[pipe] = 1; > } > > + info->max_pipes = info->num_pipes; > + > if (i915_modparams.disable_display) { > DRM_INFO("Display disabled (module parameter)\n"); > info->num_pipes = 0;
On Thu, 12 Oct 2017, "Saarinen, Jani" <jani.saarinen@intel.com> wrote: > This was also tested successfully on try-bot fixing issue seen, so > maybe we can say tested-by: try-bot? Sure, but nacked-by: yours truly I'm afraid! ;) BR, Jani.
On Thu, 2017-10-12 at 09:55 +0300, Jani Nikula wrote: > On Thu, 12 Oct 2017, Mika Kahola <mika.kahola@intel.com> wrote: > > > > CI system spotted an error on CFL system when running IGT tests > > with > > display disabled. In this case, the 'INTEL_INFO(dev_priv)- > > >num_pipes' > > is set to 0. This will cause that we prematurely return from > > 'get_saved_enc()'. > > > > To fix this issue, the patch introduces a 'max_pipes' variable > > which caches > > the maximum number of available pipes. > This is not the right fix. The audio component init should be skipped > and thus all component calls blocked when num_pipes == 0. It's just > that > the num_pipes = 0 happens way too late when the display is fused off > or > disabled via modparam. > > This makes me wonder how many things we actually screw up because > intel_device_info_runtime_init() happens too Right. So instead of doing this we shouldn't initialize the audio at all in case of disabled display? num_pipes variable just gets updated too late and audio part has already started to initialize itself. > late. > > > > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103206 > If this patch fixes the bug, we use Bugzilla: tag. Reference: is for > other references, e.g. the referenced bug is related to the patch at > hand somehow, but this does not fix the bug. > > BR, > Jani. > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_audio.c | 2 +- > > drivers/gpu/drm/i915/intel_device_info.c | 2 ++ > > 3 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 770305b..6229ff8 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -868,6 +868,7 @@ struct intel_device_info { > > u8 num_pipes; > > u8 num_sprites[I915_MAX_PIPES]; > > u8 num_scalers[I915_MAX_PIPES]; > > + u8 max_pipes; > > > > unsigned int page_sizes; /* page sizes supported by the HW > > */ > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > > b/drivers/gpu/drm/i915/intel_audio.c > > index 0ddba16..147bd3d 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -754,7 +754,7 @@ static struct intel_encoder > > *get_saved_enc(struct drm_i915_private *dev_priv, > > { > > struct intel_encoder *encoder; > > > > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > > + if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->max_pipes)) > > return NULL; > > > > /* MST */ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > b/drivers/gpu/drm/i915/intel_device_info.c > > index 875d428..ba1a807 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > @@ -376,6 +376,8 @@ void intel_device_info_runtime_init(struct > > drm_i915_private *dev_priv) > > info->num_sprites[pipe] = 1; > > } > > > > + info->max_pipes = info->num_pipes; > > + > > if (i915_modparams.disable_display) { > > DRM_INFO("Display disabled (module parameter)\n"); > > info->num_pipes = 0;
On Mon, 16 Oct 2017, Mika Kahola <mika.kahola@intel.com> wrote: > On Thu, 2017-10-12 at 09:55 +0300, Jani Nikula wrote: >> On Thu, 12 Oct 2017, Mika Kahola <mika.kahola@intel.com> wrote: >> > >> > CI system spotted an error on CFL system when running IGT tests >> > with >> > display disabled. In this case, the 'INTEL_INFO(dev_priv)- >> > >num_pipes' >> > is set to 0. This will cause that we prematurely return from >> > 'get_saved_enc()'. >> > >> > To fix this issue, the patch introduces a 'max_pipes' variable >> > which caches >> > the maximum number of available pipes. >> This is not the right fix. The audio component init should be skipped >> and thus all component calls blocked when num_pipes == 0. It's just >> that >> the num_pipes = 0 happens way too late when the display is fused off >> or >> disabled via modparam. >> >> This makes me wonder how many things we actually screw up because >> intel_device_info_runtime_init() happens too > Right. So instead of doing this we shouldn't initialize the audio at > all in case of disabled display? num_pipes variable just gets updated > too late and audio part has already started to initialize itself. Correct. BR, Jani. > >> late. >> >> > >> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103206 >> If this patch fixes the bug, we use Bugzilla: tag. Reference: is for >> other references, e.g. the referenced bug is related to the patch at >> hand somehow, but this does not fix the bug. >> >> BR, >> Jani. >> >> >> > >> > Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_drv.h | 1 + >> > drivers/gpu/drm/i915/intel_audio.c | 2 +- >> > drivers/gpu/drm/i915/intel_device_info.c | 2 ++ >> > 3 files changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > b/drivers/gpu/drm/i915/i915_drv.h >> > index 770305b..6229ff8 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > @@ -868,6 +868,7 @@ struct intel_device_info { >> > u8 num_pipes; >> > u8 num_sprites[I915_MAX_PIPES]; >> > u8 num_scalers[I915_MAX_PIPES]; >> > + u8 max_pipes; >> > >> > unsigned int page_sizes; /* page sizes supported by the HW >> > */ >> > >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c >> > b/drivers/gpu/drm/i915/intel_audio.c >> > index 0ddba16..147bd3d 100644 >> > --- a/drivers/gpu/drm/i915/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/intel_audio.c >> > @@ -754,7 +754,7 @@ static struct intel_encoder >> > *get_saved_enc(struct drm_i915_private *dev_priv, >> > { >> > struct intel_encoder *encoder; >> > >> > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) >> > + if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->max_pipes)) >> > return NULL; >> > >> > /* MST */ >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c >> > b/drivers/gpu/drm/i915/intel_device_info.c >> > index 875d428..ba1a807 100644 >> > --- a/drivers/gpu/drm/i915/intel_device_info.c >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c >> > @@ -376,6 +376,8 @@ void intel_device_info_runtime_init(struct >> > drm_i915_private *dev_priv) >> > info->num_sprites[pipe] = 1; >> > } >> > >> > + info->max_pipes = info->num_pipes; >> > + >> > if (i915_modparams.disable_display) { >> > DRM_INFO("Display disabled (module parameter)\n"); >> > info->num_pipes = 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 770305b..6229ff8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -868,6 +868,7 @@ struct intel_device_info { u8 num_pipes; u8 num_sprites[I915_MAX_PIPES]; u8 num_scalers[I915_MAX_PIPES]; + u8 max_pipes; unsigned int page_sizes; /* page sizes supported by the HW */ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0ddba16..147bd3d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -754,7 +754,7 @@ static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv, { struct intel_encoder *encoder; - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) + if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->max_pipes)) return NULL; /* MST */ diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 875d428..ba1a807 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -376,6 +376,8 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) info->num_sprites[pipe] = 1; } + info->max_pipes = info->num_pipes; + if (i915_modparams.disable_display) { DRM_INFO("Display disabled (module parameter)\n"); info->num_pipes = 0;
CI system spotted an error on CFL system when running IGT tests with display disabled. In this case, the 'INTEL_INFO(dev_priv)->num_pipes' is set to 0. This will cause that we prematurely return from 'get_saved_enc()'. To fix this issue, the patch introduces a 'max_pipes' variable which caches the maximum number of available pipes. Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103206 Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 2 +- drivers/gpu/drm/i915/intel_device_info.c | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-)