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

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

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?


BR,
Jani.



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

-- 
Jani Nikula, Intel