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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 12 Oct 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.

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 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
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@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 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx