From: Jani Nikula <jani.nikula@intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: intel-xe@lists.freedesktop.org, rodrigo.vivi@intel.com
Subject: Re: [Intel-xe] [PATCH 09/10] fixup! drm/xe/display: Implement display support
Date: Wed, 04 Oct 2023 20:32:33 +0300 [thread overview]
Message-ID: <87wmw2cmz2.fsf@intel.com> (raw)
In-Reply-To: <xqsumuzekbwjzjntiehzb7kcz6jw7wfe7tem6upe2mnmysf2fk@ar7o3jhlgbel>
On Wed, 04 Oct 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Oct 03, 2023 at 05:34:56PM +0300, Jani Nikula wrote:
>>Let the display code decide whether display hardware is there or
>>not. Remove has_display member from struct xe_device_desc, and
>>enable_display from struct xe_device.
>
> that doesn't seem good and the best example would be the current
> situation for LNL where you go from working to crashing.
Bugs aside, the single point of truth on whether display is there should
be the display code, and not duplicated. Same with i915, i915_pci.c has
no info on display.
>
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>> drivers/gpu/drm/xe/xe_device_types.h | 2 --
>> drivers/gpu/drm/xe/xe_display.c | 51 ++++++++++++----------------
>> drivers/gpu/drm/xe/xe_pci.c | 13 -------
>> 3 files changed, 22 insertions(+), 44 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>index 62e1a875980b..01ab0b3d1954 100644
>>--- a/drivers/gpu/drm/xe/xe_device_types.h
>>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>>@@ -236,8 +236,6 @@ struct xe_device {
>> u8 has_llc:1;
>> /** @has_range_tlb_invalidation: Has range based TLB invalidations */
>> u8 has_range_tlb_invalidation:1;
>>- /** @enable_display: display enabled */
>>- u8 enable_display:1;
>>
>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> struct {
>>diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>>index 75054f78d7ae..70e7afc0a890 100644
>>--- a/drivers/gpu/drm/xe/xe_display.c
>>+++ b/drivers/gpu/drm/xe/xe_display.c
>>@@ -57,7 +57,7 @@ static void xe_display_last_close(struct drm_device *dev)
>> {
>> struct xe_device *xe = to_xe_device(dev);
>>
>>- if (xe->info.enable_display)
>>+ if (has_display(xe))
>
> these are not the same thing. enable_display here is "do I even want to
> touch display?". It's not about having it or not.
>
> If you can have the i915-semantics and retain what we can do in xe that
> we can't in i915, then I'm ok with it, otherwise nak:
>
> 1) have a platform flag that tells me "do I want to enable it?"
> so we can decide on bring up if the platform is ready to have
> that enabled or not. This is the info.enable_display
>
> 2) have a module param (current solution, per module) or something
> else (future solution?, per device) that allows me to override the
> enable_display in runtime
Like I said elsewhere, what's currently in tree is broken. Please don't
expect me to fix it all up and cater for all the needs while doing so.
BR,
Jani.
> Note that pipe_mask being 0 still enables a bunch of things on the
> display side, still touches somes registers and is controlled on the
> i915-display side, not in xe. That was the case last time I checked a
> few months ago, please correct me if it's not anymore.
>
> Lucas De Marchi
>
>> intel_fbdev_restore_mode(to_xe_device(dev));
>> }
>>
>>@@ -138,7 +138,7 @@ static void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
>> {
>> struct xe_device *xe = to_xe_device(dev);
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_power_domains_cleanup(xe);
>>@@ -148,7 +148,7 @@ int xe_display_init_nommio(struct xe_device *xe)
>> {
>> int err;
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return 0;
>>
>> /* Fake uncore lock */
>>@@ -168,7 +168,7 @@ static void xe_display_fini_noirq(struct drm_device *dev, void *dummy)
>> {
>> struct xe_device *xe = to_xe_device(dev);
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_display_driver_remove_noirq(xe);
>>@@ -179,7 +179,7 @@ int xe_display_init_noirq(struct xe_device *xe)
>> {
>> int err;
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return 0;
>>
>> intel_display_driver_early_probe(xe);
>>@@ -208,7 +208,7 @@ static void xe_display_fini_noaccel(struct drm_device *dev, void *dummy)
>> {
>> struct xe_device *xe = to_xe_device(dev);
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_display_driver_remove_nogem(xe);
>>@@ -218,7 +218,7 @@ int xe_display_init_noaccel(struct xe_device *xe)
>> {
>> int err;
>>
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return 0;
>>
>> err = intel_display_driver_probe_nogem(xe);
>>@@ -230,7 +230,7 @@ int xe_display_init_noaccel(struct xe_device *xe)
>>
>> int xe_display_init(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return 0;
>>
>> return intel_display_driver_probe(xe);
>>@@ -238,7 +238,7 @@ int xe_display_init(struct xe_device *xe)
>>
>> void xe_display_fini(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> /* poll work can call into fbdev, hence clean that up afterwards */
>>@@ -251,7 +251,7 @@ void xe_display_fini(struct xe_device *xe)
>>
>> void xe_display_register(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_display_driver_register(xe);
>>@@ -261,7 +261,7 @@ void xe_display_register(struct xe_device *xe)
>>
>> void xe_display_unregister(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_unregister_dsm_handler();
>>@@ -271,7 +271,7 @@ void xe_display_unregister(struct xe_device *xe)
>>
>> void xe_display_driver_remove(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_display_driver_remove(xe);
>>@@ -281,7 +281,7 @@ void xe_display_driver_remove(struct xe_device *xe)
>>
>> void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> if (master_ctl & DISPLAY_IRQ)
>>@@ -290,7 +290,7 @@ void xe_display_irq_handler(struct xe_device *xe, u32 master_ctl)
>>
>> void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> if (gu_misc_iir & GU_MISC_GSE)
>>@@ -299,7 +299,7 @@ void xe_display_irq_enable(struct xe_device *xe, u32 gu_misc_iir)
>>
>> void xe_display_irq_reset(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> gen11_display_irq_reset(xe);
>>@@ -307,7 +307,7 @@ void xe_display_irq_reset(struct xe_device *xe)
>>
>> void xe_display_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> if (gt->info.id == XE_GT0)
>>@@ -341,7 +341,7 @@ static bool suspend_to_idle(void)
>> void xe_display_pm_suspend(struct xe_device *xe)
>> {
>> bool s2idle = suspend_to_idle();
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> /*
>>@@ -370,7 +370,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
>> void xe_display_pm_suspend_late(struct xe_device *xe)
>> {
>> bool s2idle = suspend_to_idle();
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_power_domains_suspend(xe, s2idle);
>>@@ -380,7 +380,7 @@ void xe_display_pm_suspend_late(struct xe_device *xe)
>>
>> void xe_display_pm_resume_early(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_display_power_resume_early(xe);
>>@@ -390,7 +390,7 @@ void xe_display_pm_resume_early(struct xe_device *xe)
>>
>> void xe_display_pm_resume(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>+ if (!has_display(xe))
>> return;
>>
>> intel_dmc_resume(xe);
>>@@ -418,15 +418,8 @@ void xe_display_pm_resume(struct xe_device *xe)
>>
>> void xe_display_probe(struct xe_device *xe)
>> {
>>- if (!xe->info.enable_display)
>>- goto no_display;
>>-
>> intel_display_device_probe(xe);
>>
>>- if (has_display(xe))
>>- return;
>>-
>>-no_display:
>>- xe->info.enable_display = false;
>>- unset_display_features(xe);
>>+ if (!has_display(xe))
>>+ unset_display_features(xe);
>> }
>>diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>index 8ee430c6f8b1..cbbfe75eb2d2 100644
>>--- a/drivers/gpu/drm/xe/xe_pci.c
>>+++ b/drivers/gpu/drm/xe/xe_pci.c
>>@@ -56,7 +56,6 @@ struct xe_device_desc {
>>
>> u8 require_force_probe:1;
>> u8 is_dgfx:1;
>>- u8 has_display:1;
>>
>> u8 has_llc:1;
>> };
>>@@ -207,7 +206,6 @@ static const struct xe_device_desc tgl_desc = {
>> .graphics = &graphics_xelp,
>> .media = &media_xem,
>> PLATFORM(XE_TIGERLAKE),
>>- .has_display = true,
>> .has_llc = true,
>> .require_force_probe = true,
>> };
>>@@ -216,7 +214,6 @@ static const struct xe_device_desc rkl_desc = {
>> .graphics = &graphics_xelp,
>> .media = &media_xem,
>> PLATFORM(XE_ROCKETLAKE),
>>- .has_display = true,
>> .has_llc = true,
>> .require_force_probe = true,
>> };
>>@@ -225,7 +222,6 @@ static const struct xe_device_desc adl_s_desc = {
>> .graphics = &graphics_xelp,
>> .media = &media_xem,
>> PLATFORM(XE_ALDERLAKE_S),
>>- .has_display = true,
>> .has_llc = true,
>> .require_force_probe = true,
>> };
>>@@ -236,7 +232,6 @@ static const struct xe_device_desc adl_p_desc = {
>> .graphics = &graphics_xelp,
>> .media = &media_xem,
>> PLATFORM(XE_ALDERLAKE_P),
>>- .has_display = true,
>> .has_llc = true,
>> .require_force_probe = true,
>> .subplatforms = (const struct xe_subplatform_desc[]) {
>>@@ -249,7 +244,6 @@ static const struct xe_device_desc adl_n_desc = {
>> .graphics = &graphics_xelp,
>> .media = &media_xem,
>> PLATFORM(XE_ALDERLAKE_N),
>>- .has_display = true,
>> .has_llc = true,
>> .require_force_probe = true,
>> };
>>@@ -262,7 +256,6 @@ static const struct xe_device_desc dg1_desc = {
>> .media = &media_xem,
>> DGFX_FEATURES,
>> PLATFORM(XE_DG1),
>>- .has_display = true,
>> .require_force_probe = true,
>> };
>>
>>@@ -286,7 +279,6 @@ static const struct xe_device_desc ats_m_desc = {
>> .require_force_probe = true,
>>
>> DG2_FEATURES,
>>- .has_display = false,
>> };
>>
>> static const struct xe_device_desc dg2_desc = {
>>@@ -295,14 +287,12 @@ static const struct xe_device_desc dg2_desc = {
>> .require_force_probe = true,
>>
>> DG2_FEATURES,
>>- .has_display = true,
>> };
>>
>> static const struct xe_device_desc pvc_desc = {
>> .graphics = &graphics_xehpc,
>> DGFX_FEATURES,
>> PLATFORM(XE_PVC),
>>- .has_display = false,
>> .require_force_probe = true,
>> };
>>
>>@@ -310,7 +300,6 @@ static const struct xe_device_desc mtl_desc = {
>> /* .graphics and .media determined via GMD_ID */
>> .require_force_probe = true,
>> PLATFORM(XE_METEORLAKE),
>>- .has_display = true,
>> };
>>
>> static const struct xe_device_desc lnl_desc = {
>>@@ -574,8 +563,6 @@ static int xe_info_init(struct xe_device *xe,
>> xe->info.has_flat_ccs = graphics_desc->has_flat_ccs;
>> xe->info.has_range_tlb_invalidation = graphics_desc->has_range_tlb_invalidation;
>>
>>- xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
>>- desc->has_display;
>> /*
>> * All platforms have at least one primary GT. Any platform with media
>> * version 13 or higher has an additional dedicated media GT. And
>>--
>>2.39.2
>>
--
Jani Nikula, Intel
next prev parent reply other threads:[~2023-10-04 17:32 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-03 14:34 [Intel-xe] [PATCH 00/10] xe/display: clarify display configuration Jani Nikula
2023-10-03 14:34 ` [Intel-xe] [PATCH 01/10] fixup! drm/xe/display: Implement display support Jani Nikula
2023-10-04 14:02 ` Rodrigo Vivi
2023-10-04 14:23 ` Jani Nikula
2023-10-04 14:41 ` Rodrigo Vivi
2023-10-04 14:25 ` Jani Nikula
2023-10-03 14:34 ` [Intel-xe] [PATCH 02/10] " Jani Nikula
2023-10-04 14:04 ` Rodrigo Vivi
2023-10-03 14:34 ` [Intel-xe] [PATCH 03/10] " Jani Nikula
2023-10-04 14:04 ` Rodrigo Vivi
2023-10-03 14:34 ` [Intel-xe] [PATCH 04/10] fixup! drm/xe: Introduce Xe assert macros Jani Nikula
2023-10-04 14:05 ` Rodrigo Vivi
2023-10-03 14:34 ` [Intel-xe] [PATCH 05/10] fixup! drm/xe/display: Implement display support Jani Nikula
2023-10-04 14:05 ` Rodrigo Vivi
2023-10-03 14:34 ` [Intel-xe] [PATCH 06/10] " Jani Nikula
2023-10-04 14:05 ` Rodrigo Vivi
2023-10-04 16:19 ` Lucas De Marchi
2023-10-04 16:31 ` Jani Nikula
2023-10-03 14:34 ` [Intel-xe] [PATCH 07/10] " Jani Nikula
2023-10-04 14:06 ` Rodrigo Vivi
2023-10-03 14:34 ` [Intel-xe] [PATCH 08/10] " Jani Nikula
2023-10-04 14:23 ` Rodrigo Vivi
2023-10-04 14:37 ` Jani Nikula
2023-10-04 16:14 ` Rodrigo Vivi
2023-10-04 16:41 ` Lucas De Marchi
2023-10-04 17:11 ` Jani Nikula
2023-10-04 18:03 ` Lucas De Marchi
2023-10-03 14:34 ` [Intel-xe] [PATCH 09/10] " Jani Nikula
2023-10-04 14:21 ` Rodrigo Vivi
2023-10-04 16:28 ` Lucas De Marchi
2023-10-04 17:32 ` Jani Nikula [this message]
2023-10-03 14:34 ` [Intel-xe] [PATCH 10/10] " Jani Nikula
2023-10-04 14:09 ` Rodrigo Vivi
2023-10-03 14:48 ` [Intel-xe] ✓ CI.Patch_applied: success for xe/display: clarify display configuration Patchwork
2023-10-03 14:48 ` [Intel-xe] ✗ CI.checkpatch: warning " Patchwork
2023-10-03 14:49 ` [Intel-xe] ✓ CI.KUnit: success " Patchwork
2023-10-03 14:56 ` [Intel-xe] ✓ CI.Build: " Patchwork
2023-10-03 14:57 ` [Intel-xe] ✓ CI.Hooks: " Patchwork
2023-10-03 14:58 ` [Intel-xe] ✓ CI.checksparse: " Patchwork
2023-10-03 15:35 ` [Intel-xe] ✓ CI.BAT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wmw2cmz2.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox