diff mbox series

[1/2] drm/i915: Reduce INTEL_DISPLAY_ENABLED to just removing the outputs

Message ID 20200909213824.12390-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Reduce INTEL_DISPLAY_ENABLED to just removing the outputs | expand

Commit Message

Ville Syrjälä Sept. 9, 2020, 9:38 p.m. UTC 
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>
---
 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(-)

Comments

Jani Nikula Sept. 10, 2020, 8:04 a.m. UTC | #1 
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>


> ---
>  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 Sept. 10, 2020, 9:54 a.m. UTC | #2 
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);
diff mbox series

Patch

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);