[Intel-gfx] [PATCH 02/20] drm/i915: Set PCH as NOP when display is disabled

[Souza, Jose]

On Thu, 2018-08-09 at 11:16 +0300, Jani Nikula wrote:
> On Wed, 08 Aug 2018, José Roberto de Souza <jose.souza at 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 at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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);
> 
>