[Intel-xe] [PATCH 01/10] fixup! drm/xe/display: Implement display support

[Rodrigo Vivi]

On Tue, Oct 03, 2023 at 05:34:48PM +0300, Jani Nikula 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.
> 
> v2: define local has_display() to make this a bit cleaner
> 
> 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 | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
> index 07898e0e175e..68729997e1fe 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"		/* FIXME: HAS_DISPLAY() depends on this */

should we then add a FIXME tag to the commit subject as well?
just to ensure we don't miss this here?

>  #include "intel_acpi.h"
>  #include "intel_audio.h"
>  #include "intel_bw.h"
> @@ -32,6 +33,11 @@
>  
>  /* Xe device functions */
>  
> +static bool has_display(struct xe_device *xe)
> +{
> +	return HAS_DISPLAY(xe);
> +}
> +
>  /**
>   * xe_display_driver_probe_defer - Detect if we need to wait for other drivers
>   *				   early on
> @@ -316,7 +322,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 +352,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 +398,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 +409,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 +430,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))

my first reaction to this probe function when I bump into it during
rebase is that it should be simply refactor to something like


xe_display_probe()
{
	if(!xe->info.enable_display)
		return;

	intel_display_device_probe(xe);
}

but then I put the pipe_mask check and the goto back to avoid
disruption, but without clearly understanding on why we have
that to start with.

My thoughts was on maybe it was a redundant check to see if
display init setup went well, but now I see it was more about
has_display...

But now I wonder if we cannot simply use the HAS_DISPLAY()
directly in the first line of this function and avoid everything
else?

>  		return;
>  
>  no_display:
> -- 
> 2.39.2
>