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

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

On Tue, 03 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 03 Oct 2023, Jani Nikula <jani.nikula@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.

I've sent a series superseeding this one, and addressing some of the
issues listed above [1].

BR,
Jani.


[1] https://patchwork.freedesktop.org/series/124561/


>
>
> BR,
> Jani.
>
>
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@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