| Message ID | 20180809001606.26876-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] drm: Let userspace check if driver supports modeset | expand |
On Wed, 08 Aug 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > num_pipes is set to 0 if disable_display is set inside > intel_device_info_runtime_init() but when that happen PCH will > already be set in intel_detect_pch(). > > i915_driver_load() > i915_driver_init_early() > ... > intel_detect_pch() > ... > ... > i915_driver_init_hw() > intel_device_info_runtime_init() > > So now setting num_pipes = 0 earlier to avoid this problem. Okay, this gets confusing. There are other paths in intel_device_info_runtime_init() that set num_pipes = 0 and depend on PCH having been detected. :( > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 5 +++++ > drivers/gpu/drm/i915/intel_device_info.c | 8 ++------ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9dce55182c3a..7952f5877402 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -917,6 +917,11 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > if (ret < 0) > goto err_workqueues; > > + if (i915_modparams.disable_display) { > + DRM_INFO("Display disabled (module parameter)\n"); > + device_info->num_pipes = 0; > + } > + Please look at the function as a whole, and note that this feels like a random thing to add in the middle. Needs to be stowed away somewhere deeper. Overall, I think we need to be more accurate about the relationship of num_pipes = 0 and PCH_NOP. BR, Jani. > /* This must be called before any calls to HAS_PCH_* */ > intel_detect_pch(dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 0ef0c6448d53..67102b481c8f 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -776,12 +776,8 @@ void intel_device_info_runtime_init(struct intel_device_info *info) > info->num_sprites[pipe] = 1; > } > > - if (i915_modparams.disable_display) { > - DRM_INFO("Display disabled (module parameter)\n"); > - info->num_pipes = 0; > - } else if (info->num_pipes > 0 && > - (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) && > - HAS_PCH_SPLIT(dev_priv)) { > + if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) && > + HAS_PCH_SPLIT(dev_priv)) { > u32 fuse_strap = I915_READ(FUSE_STRAP); > u32 sfuse_strap = I915_READ(SFUSE_STRAP);
Quoting José Roberto de Souza (2018-08-09 01:15:48) > num_pipes is set to 0 if disable_display is set inside > intel_device_info_runtime_init() but when that happen PCH will > already be set in intel_detect_pch(). One major thing missed is that if you disable the displays via modparam, you need to reap all the BIOS enabled displays and stolen memory that conflict with our usage. (We ignore the conflict so that means the BIOS can write into memory we are using elsewhere.) The same bug exists for outputs we don't recover from the BIOS, which is a regression from circa 3.2. -Chris
On Thu, 2018-08-09 at 11:16 +0300, Jani Nikula wrote: > On Wed, 08 Aug 2018, José Roberto de Souza <jose.souza@intel.com> > wrote: > > num_pipes is set to 0 if disable_display is set inside > > intel_device_info_runtime_init() but when that happen PCH will > > already be set in intel_detect_pch(). > > > > i915_driver_load() > > i915_driver_init_early() > > ... > > intel_detect_pch() > > ... > > ... > > i915_driver_init_hw() > > intel_device_info_runtime_init() > > > > So now setting num_pipes = 0 earlier to avoid this problem. > > Okay, this gets confusing. There are other paths in > intel_device_info_runtime_init() that set num_pipes = 0 and depend on > PCH having been detected. :( > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 5 +++++ > > drivers/gpu/drm/i915/intel_device_info.c | 8 ++------ > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 9dce55182c3a..7952f5877402 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -917,6 +917,11 @@ static int i915_driver_init_early(struct > > drm_i915_private *dev_priv, > > if (ret < 0) > > goto err_workqueues; > > > > + if (i915_modparams.disable_display) { > > + DRM_INFO("Display disabled (module parameter)\n"); > > + device_info->num_pipes = 0; > > + } > > + > > Please look at the function as a whole, and note that this feels like > a > random thing to add in the middle. Needs to be stowed away somewhere > deeper. I can move it to right after: /* Setup the write-once "constant" device info */ device_info = mkwrite_device_info(dev_priv); memcpy(device_info, match_info, sizeof(*device_info)); device_info->device_id = dev_priv->drm.pdev->device; > > Overall, I think we need to be more accurate about the relationship > of > num_pipes = 0 and PCH_NOP. The path in intel_device_info_runtime_init() that now sets num_pipes = 0 is when the display(I guess it is the whole GPU) is fused off so user can't use it at all. The other path changing num_pipes is one for IVB there is disables the last pipe. I guess with this changes we have a good relationship between num_pipes and PCH_NOP or do you have another suggestion. > > > BR, > Jani. > > > /* This must be called before any calls to HAS_PCH_* */ > > intel_detect_pch(dev_priv); > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > b/drivers/gpu/drm/i915/intel_device_info.c > > index 0ef0c6448d53..67102b481c8f 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > @@ -776,12 +776,8 @@ void intel_device_info_runtime_init(struct > > intel_device_info *info) > > info->num_sprites[pipe] = 1; > > } > > > > - if (i915_modparams.disable_display) { > > - DRM_INFO("Display disabled (module parameter)\n"); > > - info->num_pipes = 0; > > - } else if (info->num_pipes > 0 && > > - (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) && > > - HAS_PCH_SPLIT(dev_priv)) { > > + if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || > > IS_GEN8(dev_priv)) && > > + HAS_PCH_SPLIT(dev_priv)) { > > u32 fuse_strap = I915_READ(FUSE_STRAP); > > u32 sfuse_strap = I915_READ(SFUSE_STRAP); > >
On Thu, 2018-08-09 at 09:36 +0100, Chris Wilson wrote: > Quoting José Roberto de Souza (2018-08-09 01:15:48) > > num_pipes is set to 0 if disable_display is set inside > > intel_device_info_runtime_init() but when that happen PCH will > > already be set in intel_detect_pch(). > > One major thing missed is that if you disable the displays via > modparam, > you need to reap all the BIOS enabled displays and stolen memory that > conflict with our usage. (We ignore the conflict so that means the > BIOS > can write into memory we are using elsewhere.) The same bug exists > for > outputs we don't recover from the BIOS, which is a regression from > circa 3.2. I have a working in progress patch on top of this ones that complete power down all power wells, fixing this issue but for now you are right. > -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9dce55182c3a..7952f5877402 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -917,6 +917,11 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, if (ret < 0) goto err_workqueues; + if (i915_modparams.disable_display) { + DRM_INFO("Display disabled (module parameter)\n"); + device_info->num_pipes = 0; + } + /* This must be called before any calls to HAS_PCH_* */ intel_detect_pch(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 0ef0c6448d53..67102b481c8f 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -776,12 +776,8 @@ void intel_device_info_runtime_init(struct intel_device_info *info) info->num_sprites[pipe] = 1; } - if (i915_modparams.disable_display) { - DRM_INFO("Display disabled (module parameter)\n"); - info->num_pipes = 0; - } else if (info->num_pipes > 0 && - (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) && - HAS_PCH_SPLIT(dev_priv)) { + if (info->num_pipes > 0 && (IS_GEN7(dev_priv) || IS_GEN8(dev_priv)) && + HAS_PCH_SPLIT(dev_priv)) { u32 fuse_strap = I915_READ(FUSE_STRAP); u32 sfuse_strap = I915_READ(SFUSE_STRAP);
num_pipes is set to 0 if disable_display is set inside intel_device_info_runtime_init() but when that happen PCH will already be set in intel_detect_pch(). i915_driver_load() i915_driver_init_early() ... intel_detect_pch() ... ... i915_driver_init_hw() intel_device_info_runtime_init() So now setting num_pipes = 0 earlier to avoid this problem. Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/intel_device_info.c | 8 ++------ 2 files changed, 7 insertions(+), 6 deletions(-)