[Intel-gfx] [PATCH 01/16] drm/i915: Properly set PCH as NOP when display is disabled

[Lucas De Marchi]

On Sat, Nov 03, 2018 at 11:41:10PM +0200, Jani Nikula wrote:
> On Fri, 12 Oct 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.
> 
> I'm growing this nagging feeling that this may be the wrong direction.
> 
> It's just that disabling an existing display and not having display are
> two very different things. Even if our goal here is use the former to
> test the latter, I think it's more subtle than this. You already saw
> some of this in patch 15 of this series. You can't just pretend there's
> no display if you also want to gain benefits.

I'm tending to agree with this. If you want to *disable* display you
will need to do something with the HW because it's there in an unknown
state. If you don't have the HW, you can't do that because you will not
have the registers you need.

> I'm also not sure if we need to go as far as Chris is suggesting in [1],
> but assuming disable display means num_pipes = 0 from start prevents us

Could we have both approaches? I think they differ mainly on the
initialization part, but it is useful to just shutdown the display when
we don't need it.

Lucas De Marchi

> from doing that later on, because we'll start making assumptions. Like
> you already do in this series.
> 
> Again, there's a bunch of useful stuff in the series that could be
> merged with a different ordering. Including patches 3-5 that look
> superficially good but do need an in-depth review. The init sequences
> are hard, and we should add more asserts and comments on the ordering,
> because we tend to forget why things are the way they are.
> 
> 
> BR,
> Jani.
> 
> [1] https://patchwork.freedesktop.org/series/51000/
> 
> >
> > 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 baac35f698f9..e3efc3dd8a30 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1649,6 +1649,11 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	memcpy(device_info, match_info, sizeof(*device_info));
> >  	device_info->device_id = pdev->device;
> >  
> > +	if (i915_modparams.disable_display) {
> > +		DRM_INFO("Display disabled (module parameter)\n");
> > +		device_info->num_pipes = 0;
> > +	}
> > +
> >  	BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >  		     BITS_PER_TYPE(device_info->platform_mask));
> >  	BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index 03df4e33763d..69be3f211737 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -775,12 +775,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);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx