Re: [Intel-gfx] [PATCH 1/2] drm/i915: Reduce INTEL_DISPLAY_ENABLED to just removing the outputs

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

On Thu, 10 Sep 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 10 Sep 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Having a mode where the display hardware is present but we try
>> to pretend it isn't just leads to massive headaches when trying
>> to reason what the fallout might be from skipping some random
>> bits of programming.
>>
>> Let's just neuter INTEL_DISPLAY_ENABLED so that we treat the
>> hardware as fully present, except we just don't register any
>> outputs. That's still rather sketchy if the outputs are already
>> enabled when the driver is loaded. I think the simplest solution
>> would be to probe everything as normal and just return
>> disconnected" from all .detect() hooks. That would avoid anything
>> automagically enabling those outputs, but the driver could then
>> shut things down using the normal codepaths.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> I agree with the reasoning and the patches. It will probably conflict
> with someone else's unspecified notion of what "display disable" should
> actually mean. But at least this approach is internally consistent.
>
> Would be great if we could hide the outputs from userspace afterwards,
> but that's probably not trivial.
>
> Both patches,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Patch 1 in [1] is follow-up to this, patches 2-3 should probably have
been sent separately as related but independent.

BR,
Jani.


[1] https://patchwork.freedesktop.org/series/81541/



>
>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c    | 2 +-
>>  drivers/gpu/drm/i915/display/intel_display.c | 8 ++++----
>>  drivers/gpu/drm/i915/display/intel_fbdev.c   | 3 +--
>>  drivers/gpu/drm/i915/display/intel_gmbus.c   | 2 +-
>>  drivers/gpu/drm/i915/i915_drv.c              | 4 ++--
>>  5 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index a0a41ec5c341..c110cd9e8a73 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -2133,7 +2133,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>  
>>  	INIT_LIST_HEAD(&dev_priv->vbt.display_devices);
>>  
>> -	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) {
>> +	if (!HAS_DISPLAY(dev_priv)) {
>>  		drm_dbg_kms(&dev_priv->drm,
>>  			    "Skipping VBT init due to disabled display.\n");
>>  		return;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index ec148a8da2c2..bacaf713eed4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -17882,7 +17882,7 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
>>  	if (i915_inject_probe_failure(i915))
>>  		return -ENODEV;
>>  
>> -	if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) {
>> +	if (HAS_DISPLAY(i915)) {
>>  		ret = drm_vblank_init(&i915->drm,
>>  				      INTEL_NUM_PIPES(i915));
>>  		if (ret)
>> @@ -17956,7 +17956,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915)
>>  		    INTEL_NUM_PIPES(i915),
>>  		    INTEL_NUM_PIPES(i915) > 1 ? "s" : "");
>>  
>> -	if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) {
>> +	if (HAS_DISPLAY(i915)) {
>>  		for_each_pipe(i915, pipe) {
>>  			ret = intel_crtc_init(i915, pipe);
>>  			if (ret) {
>> @@ -18045,7 +18045,7 @@ int intel_modeset_init(struct drm_i915_private *i915)
>>  
>>  	intel_overlay_setup(i915);
>>  
>> -	if (!HAS_DISPLAY(i915) || !INTEL_DISPLAY_ENABLED(i915))
>> +	if (!HAS_DISPLAY(i915))
>>  		return 0;
>>  
>>  	ret = intel_fbdev_init(&i915->drm);
>> @@ -19018,7 +19018,7 @@ intel_display_capture_error_state(struct drm_i915_private *dev_priv)
>>  
>>  	BUILD_BUG_ON(ARRAY_SIZE(transcoders) != ARRAY_SIZE(error->transcoder));
>>  
>> -	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv))
>> +	if (!HAS_DISPLAY(dev_priv))
>>  		return NULL;
>>  
>>  	error = kzalloc(sizeof(*error), GFP_ATOMIC);
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index bd39eb6a21b8..842c04e63214 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -451,8 +451,7 @@ int intel_fbdev_init(struct drm_device *dev)
>>  	struct intel_fbdev *ifbdev;
>>  	int ret;
>>  
>> -	if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv) ||
>> -			!INTEL_DISPLAY_ENABLED(dev_priv)))
>> +	if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
>>  		return -ENODEV;
>>  
>>  	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index a8d119b6b45c..e6b8d6dfb598 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -834,7 +834,7 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  	unsigned int pin;
>>  	int ret;
>>  
>> -	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv))
>> +	if (!HAS_DISPLAY(dev_priv))
>>  		return 0;
>>  
>>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index d66fe09d337e..9b35af2cf225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -693,7 +693,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>>  		drm_err(&dev_priv->drm,
>>  			"Failed to register driver for userspace access!\n");
>>  
>> -	if (HAS_DISPLAY(dev_priv) && INTEL_DISPLAY_ENABLED(dev_priv)) {
>> +	if (HAS_DISPLAY(dev_priv)) {
>>  		/* Must be done after probing outputs */
>>  		intel_opregion_register(dev_priv);
>>  		acpi_video_register();
>> @@ -716,7 +716,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>>  	 * We need to coordinate the hotplugs with the asynchronous fbdev
>>  	 * configuration, for which we use the fbdev->async_cookie.
>>  	 */
>> -	if (HAS_DISPLAY(dev_priv) && INTEL_DISPLAY_ENABLED(dev_priv))
>> +	if (HAS_DISPLAY(dev_priv))
>>  		drm_kms_helper_poll_init(dev);
>>  
>>  	intel_power_domains_enable(dev_priv);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx