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

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

[Maarten Lankhorst]

From: Maarten Lankhorst <dev@lankhorst.se>

Add null check to make cursor magic work.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/xe/display/xe_fb_pin.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
index 3422942a99518..ac0d4474cd880 100644
--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
@@ -308,6 +308,7 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state)
 
 void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state)
 {
-	__xe_unpin_fb_vma(old_plane_state->ggtt_vma);
+	if (old_plane_state->ggtt_vma)
+		__xe_unpin_fb_vma(old_plane_state->ggtt_vma);
 	old_plane_state->ggtt_vma = NULL;
 }
-- 
2.39.2

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

[Jani Nikula]

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.

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:
-- 
2.39.2

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

[Jani Nikula]

We have an abstraction for "display runtime info", and it's
DISPLAY_RUNTIME_INFO(). Use it.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
index 7c825f129166..f739858d17e2 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
@@ -90,9 +90,9 @@ static inline struct drm_i915_private *kdev_to_i915(struct device *kdev)
 #define IP_VER(ver, rel)                ((ver) << 8 | (rel))
 
 #define INTEL_DISPLAY_ENABLED(xe) (HAS_DISPLAY((xe)) && !intel_opregion_headless_sku((xe)))
-#define DISPLAY_VER(xe) ((xe)->info.display_runtime.ip.ver)
-#define DISPLAY_VER_FULL(xe) IP_VER((xe)->info.display_runtime.ip.ver, \
-				    (xe)->info.display_runtime.ip.rel)
+#define DISPLAY_VER(i915)	(DISPLAY_RUNTIME_INFO(i915)->ip.ver)
+#define DISPLAY_VER_FULL(i915)	IP_VER(DISPLAY_RUNTIME_INFO(i915)->ip.ver, \
+				       DISPLAY_RUNTIME_INFO(i915)->ip.rel)
 
 #define IS_DISPLAY_VER(xe, first, last) ((DISPLAY_VER(xe) >= first && DISPLAY_VER(xe) <= last))
 #define IS_GRAPHICS_VER(xe, first, last) \
-- 
2.39.2

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

[Jani Nikula]

The display device and runtime info have been moved as part of struct
intel_display, and the ones in struct xe_device are stale.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++--
 drivers/gpu/drm/xe/xe_device_types.h              | 2 --
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
index f739858d17e2..e7c9b4ea2153 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
@@ -206,9 +206,10 @@ static inline void intel_runtime_pm_put(struct xe_runtime_pm *pm, bool wakeref)
 
 #define intel_step_name xe_step_name
 #define pdev_to_i915 pdev_to_xe_device
-#define DISPLAY_INFO(xe)		((xe)->info.display)
 #define RUNTIME_INFO(xe)		(&(xe)->info.i915_runtime)
-#define DISPLAY_RUNTIME_INFO(xe)	(&(xe)->info.display_runtime)
+
+#define DISPLAY_INFO(xe)		((xe)->display.info.__device_info)
+#define DISPLAY_RUNTIME_INFO(xe)	(&(xe)->display.info.__runtime_info)
 
 #define FORCEWAKE_ALL XE_FORCEWAKE_ALL
 #define HPD_STORM_DEFAULT_THRESHOLD 50
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 0717839ae964..62e1a875980b 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -240,8 +240,6 @@ struct xe_device {
 		u8 enable_display:1;
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
-		const struct intel_display_device_info *display;
-		struct intel_display_runtime_info display_runtime;
 		struct {
 			u32 rawclk_freq;
 		} i915_runtime;
-- 
2.39.2

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

[Jani Nikula]

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.


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

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

[Jani Nikula]

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