[Intel-xe] [PATCH 1/3] fixup! drm/xe/display: Implement display support

[Jani Nikula]

On Tue, 03 Oct 2023, Jani Nikula <jani.nikula at intel.com> wrote:
> We have an abstraction for "has display", and it's
> HAS_DISPLAY(). Unfortunately, it requires access to
> DISPLAY_RUNTIME_INFO(), so include compat-i915-headers/i915_drv.h too,
> although it's a bit meh.
>
> Looking at this makes me think there's a bunch of confusion in:
>
> - the pipe_mask or now HAS_DISPLAY() checks
> - the global enable_display checks
> - the xe->info.enable_display checks
> - redefinition of INTEL_DISPLAY_ENABLED()
>
> I really don't understand this, but it all looks very suspicious. This
> change leaves all that in place, unmodified.

To elaborate, this statement in xe_pci.c conflates three different
concepts into one, and it just does not work like this:

	xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
				  enable_display &&
				  desc->has_display;

CONFIG_DRM_XE_DISPLAY describes whether display support has been built
into the driver. CONFIG_DRM_XE_DISPLAY=n is fine if there is no display
hardware. Otherwise, the display hardware will be left in whatever state
the BIOS/GOP left it, e.g. consuming a bunch of power which is not
desirable. (Even with CONFIG_DRM_XE_DISPLAY=n we might want to include
enough display code to probe and warn about this scenario.)

enable_display is a module parameter apparently intended to disable
display dynamically. Similar to the above, if CONFIG_DRM_XE_DISPLAY=y
but enable_display==false, the display hardware will be left in whatever
state the BIOS/GOP left it. In i915, the module parameter is
i915.disable_display, and it does run a bunch of display code to take
over the hardware, put it to sleep, and keep all connectors
disconnected. If you have display hardware, this is the sensible thing
to do.

desc->has_display means that we've probed the hardware and found it's
not there, and we shouldn't touch it.


BR,
Jani.


>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_display.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
> index 07898e0e175e..c783573585d7 100644
> --- a/drivers/gpu/drm/xe/xe_display.c
> +++ b/drivers/gpu/drm/xe/xe_display.c
> @@ -15,6 +15,7 @@
>  #include <drm/xe_drm.h>
>  
>  #include "soc/intel_dram.h"
> +#include "i915_drv.h"
>  #include "intel_acpi.h"
>  #include "intel_audio.h"
>  #include "intel_bw.h"
> @@ -316,7 +317,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
>  	struct drm_device *dev = &xe->drm;
>  	struct intel_encoder *encoder;
>  
> -	if (xe->info.display_runtime.pipe_mask)
> +	if (HAS_DISPLAY(xe))
>  		return;
>  
>  	drm_modeset_lock_all(dev);
> @@ -346,7 +347,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
>  	 * properly.
>  	 */
>  	intel_power_domains_disable(xe);
> -	if (xe->info.display_runtime.pipe_mask)
> +	if (HAS_DISPLAY(xe))
>  		drm_kms_helper_poll_disable(&xe->drm);
>  
>  	intel_display_driver_suspend(xe);
> @@ -392,7 +393,7 @@ void xe_display_pm_resume(struct xe_device *xe)
>  
>  	intel_dmc_resume(xe);
>  
> -	if (xe->info.display_runtime.pipe_mask)
> +	if (HAS_DISPLAY(xe))
>  		drm_mode_config_reset(&xe->drm);
>  
>  	intel_display_driver_init_hw(xe);
> @@ -403,7 +404,7 @@ void xe_display_pm_resume(struct xe_device *xe)
>  	intel_display_driver_resume(xe);
>  
>  	intel_hpd_poll_disable(xe);
> -	if (xe->info.display_runtime.pipe_mask)
> +	if (HAS_DISPLAY(xe))
>  		drm_kms_helper_poll_enable(&xe->drm);
>  
>  	intel_opregion_resume(xe);
> @@ -424,7 +425,7 @@ void xe_display_probe(struct xe_device *xe)
>  
>  	intel_display_device_probe(xe);
>  
> -	if (xe->info.display_runtime.pipe_mask)
> +	if (HAS_DISPLAY(xe))
>  		return;
>  
>  no_display:

-- 
Jani Nikula, Intel