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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Oct 04, 2023 at 05:23:14PM +0300, Jani Nikula wrote:
> On Wed, 04 Oct 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > 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@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@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?
> 
> The display probe may end up detecting there's no display
> i.e. HAS_DISPLAY() may change depending on probe. And if there's no
> display, you need to drop DRIVER_MODESET and DRIVER_ATOMIC from
> driver_features.
> 
> Or are you suggesting to use HAS_DISPLAY() directly instead of the
> has_display() wrapper I cooked up?

this was my thought, but I confess I didn't think deep about the
implications above.

Feel free to go with this as is.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> BR,
> Jani.
> 
> 
> 
> >
> >>  		return;
> >>  
> >>  no_display:
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel