From: Jani Nikula <jani.nikula@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>, intel-gfx@lists.freedesktop.org
Cc: matthew.d.roper@intel.com
Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init
Date: Fri, 02 Jun 2023 13:04:06 +0300 [thread overview]
Message-ID: <87bkhyfaeh.fsf@intel.com> (raw)
In-Reply-To: <20230601212535.675751-1-matthew.d.roper@intel.com>
On Thu, 01 Jun 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> Moving display-specific runtime info initialization into display/ makes
> the display code more self-contained and also makes it easier to call
> from the Xe driver.
I like the direction here. A few minor issues, comments inline.
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> .../drm/i915/display/intel_display_device.c | 124 +++++++++++++++++
> .../drm/i915/display/intel_display_device.h | 1 +
> drivers/gpu/drm/i915/intel_device_info.c | 130 +-----------------
> 3 files changed, 128 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 464df1764a86..8d379da877dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -7,6 +7,8 @@
> #include <drm/drm_color_mgmt.h>
> #include <linux/pci.h>
>
> +#include "display/intel_de.h"
> +#include "display/intel_display.h"
The display/ prefix should be dropped.
> #include "i915_drv.h"
> #include "i915_reg.h"
> #include "intel_display_device.h"
> @@ -778,3 +780,125 @@ intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
>
> return &no_display;
> }
> +
> +void intel_display_device_info_runtime_init(struct drm_i915_private *i915)
> +{
> + struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(i915);
> + enum pipe pipe;
> +
> + /* Wa_14011765242: adl-s A0,A1 */
> + if (IS_ADLS_DISPLAY_STEP(i915, STEP_A0, STEP_A2))
> + for_each_pipe(i915, pipe)
> + display_runtime->num_scalers[pipe] = 0;
> + else if (DISPLAY_VER(i915) >= 11) {
> + for_each_pipe(i915, pipe)
> + display_runtime->num_scalers[pipe] = 2;
> + } else if (DISPLAY_VER(i915) >= 9) {
> + display_runtime->num_scalers[PIPE_A] = 2;
> + display_runtime->num_scalers[PIPE_B] = 2;
> + display_runtime->num_scalers[PIPE_C] = 1;
> + }
> +
> + if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
> + for_each_pipe(i915, pipe)
> + display_runtime->num_sprites[pipe] = 4;
> + else if (DISPLAY_VER(i915) >= 11)
> + for_each_pipe(i915, pipe)
> + display_runtime->num_sprites[pipe] = 6;
> + else if (DISPLAY_VER(i915) == 10)
> + for_each_pipe(i915, pipe)
> + display_runtime->num_sprites[pipe] = 3;
> + else if (IS_BROXTON(i915)) {
> + /*
> + * Skylake and Broxton currently don't expose the topmost plane as its
> + * use is exclusive with the legacy cursor and we only want to expose
> + * one of those, not both. Until we can safely expose the topmost plane
> + * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> + * we don't expose the topmost plane at all to prevent ABI breakage
> + * down the line.
> + */
> +
> + display_runtime->num_sprites[PIPE_A] = 2;
> + display_runtime->num_sprites[PIPE_B] = 2;
> + display_runtime->num_sprites[PIPE_C] = 1;
> + } else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> + for_each_pipe(i915, pipe)
> + display_runtime->num_sprites[pipe] = 2;
> + } else if (DISPLAY_VER(i915) >= 5 || IS_G4X(i915)) {
> + for_each_pipe(i915, pipe)
> + display_runtime->num_sprites[pipe] = 1;
> + }
> +
> + if ((IS_DGFX(i915) || DISPLAY_VER(i915) >= 14) &&
> + !(intel_de_read(i915, GU_CNTL_PROTECTED) & DEPRESENT)) {
> + drm_info(&i915->drm, "Display not present, disabling\n");
> + goto display_fused_off;
> + }
> +
> + if (IS_GRAPHICS_VER(i915, 7, 8) && HAS_PCH_SPLIT(i915)) {
> + u32 fuse_strap = intel_de_read(i915, FUSE_STRAP);
> + u32 sfuse_strap = intel_de_read(i915, SFUSE_STRAP);
> +
> + /*
> + * SFUSE_STRAP is supposed to have a bit signalling the display
> + * is fused off. Unfortunately it seems that, at least in
> + * certain cases, fused off display means that PCH display
> + * reads don't land anywhere. In that case, we read 0s.
> + *
> + * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> + * should be set when taking over after the firmware.
> + */
> + if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> + sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> + (HAS_PCH_CPT(i915) &&
> + !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> + drm_info(&i915->drm,
> + "Display fused off, disabling\n");
> + goto display_fused_off;
> + } else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> + drm_info(&i915->drm, "PipeC fused off\n");
> + display_runtime->pipe_mask &= ~BIT(PIPE_C);
> + display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> + }
> + } else if (DISPLAY_VER(i915) >= 9) {
> + u32 dfsm = intel_de_read(i915, SKL_DFSM);
> +
> + if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> + display_runtime->pipe_mask &= ~BIT(PIPE_A);
> + display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> + display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> + }
> + if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> + display_runtime->pipe_mask &= ~BIT(PIPE_B);
> + display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> + }
> + if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> + display_runtime->pipe_mask &= ~BIT(PIPE_C);
> + display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> + }
> +
> + if (DISPLAY_VER(i915) >= 12 &&
> + (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> + display_runtime->pipe_mask &= ~BIT(PIPE_D);
> + display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> + }
I'm not sure if it's likely at all, but what if ->pipe_mask == 0 after
the disables above? Then this no longer clears display runtime info like
it used to.
Although later on that still leads to setting info->display =
&no_display. But I guess I'd like to not make functional changes like
that here if possible.
> +
> + if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> + display_runtime->has_hdcp = 0;
> +
> + if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> + display_runtime->fbc_mask = 0;
> +
> + if (DISPLAY_VER(i915) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> + display_runtime->has_dmc = 0;
> +
> + if (IS_DISPLAY_VER(i915, 10, 12) &&
> + (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> + display_runtime->has_dsc = 0;
> + }
> +
> + return;
> +
> +display_fused_off:
> + memset(display_runtime, 0, sizeof(*display_runtime));
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 2aa82cbdf1c5..4f931258d81d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -124,5 +124,6 @@ struct intel_display_device_info {
> const struct intel_display_device_info *
> intel_display_device_probe(struct drm_i915_private *i915, bool has_gmdid,
> u16 *ver, u16 *rel, u16 *step);
> +void intel_display_device_info_runtime_init(struct drm_i915_private *i915);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 2f79d232b04a..ed6183aaaa5c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -27,9 +27,7 @@
> #include <drm/drm_print.h>
> #include <drm/i915_pciids.h>
>
> -#include "display/intel_cdclk.h"
> -#include "display/intel_de.h"
> -#include "display/intel_display.h"
> +#include "display/intel_display_device.h"
> #include "gt/intel_gt_regs.h"
> #include "i915_drv.h"
> #include "i915_reg.h"
> @@ -411,126 +409,12 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> {
> struct intel_device_info *info = mkwrite_device_info(dev_priv);
> struct intel_runtime_info *runtime = RUNTIME_INFO(dev_priv);
> - struct intel_display_runtime_info *display_runtime =
> - DISPLAY_RUNTIME_INFO(dev_priv);
> - enum pipe pipe;
>
> - /* Wa_14011765242: adl-s A0,A1 */
> - if (IS_ADLS_DISPLAY_STEP(dev_priv, STEP_A0, STEP_A2))
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_scalers[pipe] = 0;
> - else if (DISPLAY_VER(dev_priv) >= 11) {
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_scalers[pipe] = 2;
> - } else if (DISPLAY_VER(dev_priv) >= 9) {
> - display_runtime->num_scalers[PIPE_A] = 2;
> - display_runtime->num_scalers[PIPE_B] = 2;
> - display_runtime->num_scalers[PIPE_C] = 1;
> - }
> + if (HAS_DISPLAY(dev_priv))
> + intel_display_device_info_runtime_init(dev_priv);
I'm wondering if it would make sense to have that function return a
value that would get used instead of the later !HAS_DISPLAY() branch
later. Now it feels like there's a bit of a disconnect between the
two. Or at least move that block right here:
if (HAS_DISPLAY(dev_priv)) {
intel_display_device_info_runtime_init(dev_priv);
if (!HAS_DISPLAY(dev_priv)) {
...
}
}
This way there's more clarity I think. Can be a follow-up patch too.
I think the contents of that branch i.e.
dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
DRIVER_ATOMIC);
info->display = &no_display;
should stay here (like you've left them) to have all of that at about
the same layer.
BR,
Jani.
>
> BUILD_BUG_ON(BITS_PER_TYPE(intel_engine_mask_t) < I915_NUM_ENGINES);
>
> - if (DISPLAY_VER(dev_priv) >= 13 || HAS_D12_PLANE_MINIMIZATION(dev_priv))
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_sprites[pipe] = 4;
> - else if (DISPLAY_VER(dev_priv) >= 11)
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_sprites[pipe] = 6;
> - else if (DISPLAY_VER(dev_priv) == 10)
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_sprites[pipe] = 3;
> - else if (IS_BROXTON(dev_priv)) {
> - /*
> - * Skylake and Broxton currently don't expose the topmost plane as its
> - * use is exclusive with the legacy cursor and we only want to expose
> - * one of those, not both. Until we can safely expose the topmost plane
> - * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
> - * we don't expose the topmost plane at all to prevent ABI breakage
> - * down the line.
> - */
> -
> - display_runtime->num_sprites[PIPE_A] = 2;
> - display_runtime->num_sprites[PIPE_B] = 2;
> - display_runtime->num_sprites[PIPE_C] = 1;
> - } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_sprites[pipe] = 2;
> - } else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv)) {
> - for_each_pipe(dev_priv, pipe)
> - display_runtime->num_sprites[pipe] = 1;
> - }
> -
> - if (HAS_DISPLAY(dev_priv) &&
> - (IS_DGFX(dev_priv) || DISPLAY_VER(dev_priv) >= 14) &&
> - !(intel_de_read(dev_priv, GU_CNTL_PROTECTED) & DEPRESENT)) {
> - drm_info(&dev_priv->drm, "Display not present, disabling\n");
> -
> - display_runtime->pipe_mask = 0;
> - }
> -
> - if (HAS_DISPLAY(dev_priv) && IS_GRAPHICS_VER(dev_priv, 7, 8) &&
> - HAS_PCH_SPLIT(dev_priv)) {
> - u32 fuse_strap = intel_de_read(dev_priv, FUSE_STRAP);
> - u32 sfuse_strap = intel_de_read(dev_priv, SFUSE_STRAP);
> -
> - /*
> - * SFUSE_STRAP is supposed to have a bit signalling the display
> - * is fused off. Unfortunately it seems that, at least in
> - * certain cases, fused off display means that PCH display
> - * reads don't land anywhere. In that case, we read 0s.
> - *
> - * On CPT/PPT, we can detect this case as SFUSE_STRAP_FUSE_LOCK
> - * should be set when taking over after the firmware.
> - */
> - if (fuse_strap & ILK_INTERNAL_DISPLAY_DISABLE ||
> - sfuse_strap & SFUSE_STRAP_DISPLAY_DISABLED ||
> - (HAS_PCH_CPT(dev_priv) &&
> - !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) {
> - drm_info(&dev_priv->drm,
> - "Display fused off, disabling\n");
> - display_runtime->pipe_mask = 0;
> - } else if (fuse_strap & IVB_PIPE_C_DISABLE) {
> - drm_info(&dev_priv->drm, "PipeC fused off\n");
> - display_runtime->pipe_mask &= ~BIT(PIPE_C);
> - display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> - }
> - } else if (HAS_DISPLAY(dev_priv) && DISPLAY_VER(dev_priv) >= 9) {
> - u32 dfsm = intel_de_read(dev_priv, SKL_DFSM);
> -
> - if (dfsm & SKL_DFSM_PIPE_A_DISABLE) {
> - display_runtime->pipe_mask &= ~BIT(PIPE_A);
> - display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_A);
> - display_runtime->fbc_mask &= ~BIT(INTEL_FBC_A);
> - }
> - if (dfsm & SKL_DFSM_PIPE_B_DISABLE) {
> - display_runtime->pipe_mask &= ~BIT(PIPE_B);
> - display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_B);
> - }
> - if (dfsm & SKL_DFSM_PIPE_C_DISABLE) {
> - display_runtime->pipe_mask &= ~BIT(PIPE_C);
> - display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_C);
> - }
> -
> - if (DISPLAY_VER(dev_priv) >= 12 &&
> - (dfsm & TGL_DFSM_PIPE_D_DISABLE)) {
> - display_runtime->pipe_mask &= ~BIT(PIPE_D);
> - display_runtime->cpu_transcoder_mask &= ~BIT(TRANSCODER_D);
> - }
> -
> - if (dfsm & SKL_DFSM_DISPLAY_HDCP_DISABLE)
> - display_runtime->has_hdcp = 0;
> -
> - if (dfsm & SKL_DFSM_DISPLAY_PM_DISABLE)
> - display_runtime->fbc_mask = 0;
> -
> - if (DISPLAY_VER(dev_priv) >= 11 && (dfsm & ICL_DFSM_DMC_DISABLE))
> - display_runtime->has_dmc = 0;
> -
> - if (IS_DISPLAY_VER(dev_priv, 10, 12) &&
> - (dfsm & GLK_DFSM_DISPLAY_DSC_DISABLE))
> - display_runtime->has_dsc = 0;
> - }
> -
> if (GRAPHICS_VER(dev_priv) == 6 && i915_vtd_active(dev_priv)) {
> drm_info(&dev_priv->drm,
> "Disabling ppGTT for VT-d support\n");
> @@ -544,14 +428,6 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
> dev_priv->drm.driver_features &= ~(DRIVER_MODESET |
> DRIVER_ATOMIC);
> info->display = &no_display;
> -
> - display_runtime->cpu_transcoder_mask = 0;
> - memset(display_runtime->num_sprites, 0, sizeof(display_runtime->num_sprites));
> - memset(display_runtime->num_scalers, 0, sizeof(display_runtime->num_scalers));
> - display_runtime->fbc_mask = 0;
> - display_runtime->has_hdcp = false;
> - display_runtime->has_dmc = false;
> - display_runtime->has_dsc = false;
> }
>
> /* Disable nuclear pageflip by default on pre-g4x */
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2023-06-02 10:04 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-01 21:25 [Intel-gfx] [PATCH] drm/i915/display: Extract display init from intel_device_info_runtime_init Matt Roper
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-06-01 23:31 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-06-01 23:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-06-02 10:04 ` Jani Nikula [this message]
2023-06-02 18:04 ` [Intel-gfx] [PATCH] " Matt Roper
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=87bkhyfaeh.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.