Re: [PATCH 2/2] drm/i915/display: Print PICA version
On Fri, Dec 08, 2023 at 01:46:09PM -0600, Lucas De Marchi wrote:
> On Fri, Dec 08, 2023 at 10:02:53AM -0800, Matt Roper wrote:
> > On Fri, Dec 08, 2023 at 06:31:37AM -0800, Lucas De Marchi wrote:
> > > The PICA_DEVICE_ID follows the same format as other GMD_ID registers
> > > (graphics, display and media). Currently the only use for it is
> > > informational for developers while checking the differences between
> > > machines with the same platform. Print the raw number as there's no need
> > > for the driver to check any of the fields.
> > >
> > > In future this may change, but then the IP version in
> > > struct intel_display_runtime_info will need some refactor to allow
> > > keeping both the display engine and PICA versions.
> > >
> > > Lastly, keeping the PICA version around in the display runtime info
> > > matches its current use. If that changes in future, then it may need
> > > to be moved to the device info.
> > >
> > > Signed-off-by: Lucas De Marchi
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display_device.c | 5 +
> > > drivers/gpu/drm/i915/display/intel_display_device.h | 2 ++
> > > drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > > 3 files changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
> > > b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > index 5d1084a98b93..d9d1be008e4c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > > @@ -956,6 +956,9 @@ static void
> > > __intel_display_device_info_runtime_init(struct drm_i915_private *i9
> > > BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->cpu_transcoder_mask) <
> > > I915_MAX_TRANSCODERS);
> > > BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->port_mask) <
> > > I915_MAX_PORTS);
> > >
> > > + if (HAS_GMD_ID(i915))
> > > + display_runtime->pica_id = intel_de_read(i915, PICA_DEVICE_ID);
> > > +
> > > /* This covers both ULT and ULX */
> > > if (IS_HASWELL_ULT(i915) || IS_BROADWELL_ULT(i915))
> > > display_runtime->port_mask &= ~BIT(PORT_D);
> > > @@ -1124,6 +1127,8 @@ void intel_display_device_info_print(const struct
> > > intel_display_device_info *inf
> > > {
> > > drm_printf(p, "display version: %u.%02u.%02u\n",
> > > runtime->ip.ver, runtime->ip.rel, runtime->ip.step);
> > > + if (runtime->pica_id)
> > > + drm_printf(p, "PICA version: %#08x\n", runtime->pica_id);
> >
> > The actual, expected PICA version number on most (all?) MTL devices will
> > be 0.0. Should we make the condition here HAS_GMD_ID(i915) instead so
> > that we print the value whenever we read it? That will also help us
> > notice if the PICA register is incorrectly 0 on LNL or later platforms
> > where it should have been non-zero.
>
> it seems intel_display_device_info_print() was specially crafted so we
> don't have the i915/xe pointer and instead only operate on the info.
> Yeah, we can get back to i915/xe, but I'm not sure about doing that.
>
> What if we initialize it with U32_MAX and check for it here. I think it
> would take a long time before that becomes invalid.
>
> Another option is not to bother with the additional message in platforms
> without it, and just print it unconditionally, like patch 1 does for the
> rel/step.
>
> Thoughts?
Hmm. Maybe just printing it unconditionally would be fine. Or just
leaving it as you have here and only printing non-zero values is
probably fine too. Either way,
Reviewed-by: Matt Roper
>
> Lucas De Marchi
>
> >
> >
> > Matt
> >
> > >
> > > #define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name,
> > > str_yes_no(info->name))
> > > DEV_INFO_DISPLAY_FOR_EACH_FLAG(PRINT_FLAG);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> > > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > index 79e9f1c3e241..f8a2ada1a4ec 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > > @@ -123,6 +123,8 @@ struct intel_display_runtime_info {
> > >
> > > u8 fbc_mask;
> > >
> > > + u32 pica_id;
> > > +
> > > bool has_hdcp;
> > > bool has_dmc;
> > > bool has_dsc;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 27dc903f0553..9d70635508ae 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6362,6 +6362,8 @@ enum skl_power_gate {
> > > #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT REG_BIT(1)
> > > #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT REG_BIT(0)
> > >
> > > +#define PICA_DEVICE_ID _MMIO(0x16fe00)
> > > +
> > > #define PRIMARY_SPI_TRIGGER _MMIO(0x102040)
> > > #define PRIMARY_SPI_ADDRESS _MMIO(0x102080)
> > > #define PRIMARY_SPI_REGIONID _MMIO(0x102084)
> > > --
> > >
Re: [PATCH 2/2] drm/i915/display: Print PICA version
On Fri, Dec 08, 2023 at 10:02:53AM -0800, Matt Roper wrote:
On Fri, Dec 08, 2023 at 06:31:37AM -0800, Lucas De Marchi wrote:
The PICA_DEVICE_ID follows the same format as other GMD_ID registers
(graphics, display and media). Currently the only use for it is
informational for developers while checking the differences between
machines with the same platform. Print the raw number as there's no need
for the driver to check any of the fields.
In future this may change, but then the IP version in
struct intel_display_runtime_info will need some refactor to allow
keeping both the display engine and PICA versions.
Lastly, keeping the PICA version around in the display runtime info
matches its current use. If that changes in future, then it may need
to be moved to the device info.
Signed-off-by: Lucas De Marchi
---
drivers/gpu/drm/i915/display/intel_display_device.c | 5 +
drivers/gpu/drm/i915/display/intel_display_device.h | 2 ++
drivers/gpu/drm/i915/i915_reg.h | 2 ++
3 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
b/drivers/gpu/drm/i915/display/intel_display_device.c
index 5d1084a98b93..d9d1be008e4c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -956,6 +956,9 @@ static void __intel_display_device_info_runtime_init(struct
drm_i915_private *i9
BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->cpu_transcoder_mask) <
I915_MAX_TRANSCODERS);
BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->port_mask) <
I915_MAX_PORTS);
+ if (HAS_GMD_ID(i915))
+ display_runtime->pica_id = intel_de_read(i915, PICA_DEVICE_ID);
+
/* This covers both ULT and ULX */
if (IS_HASWELL_ULT(i915) || IS_BROADWELL_ULT(i915))
display_runtime->port_mask &= ~BIT(PORT_D);
@@ -1124,6 +1127,8 @@ void intel_display_device_info_print(const struct
intel_display_device_info *inf
{
drm_printf(p, "display version: %u.%02u.%02u\n",
runtime->ip.ver, runtime->ip.rel, runtime->ip.step);
+ if (runtime->pica_id)
+ drm_printf(p, "PICA version: %#08x\n", runtime->pica_id);
The actual, expected PICA version number on most (all?) MTL devices will
be 0.0. Should we make the condition here HAS_GMD_ID(i915) instead so
that we print the value whenever we read it? That will also help us
notice if the PICA register is incorrectly 0 on LNL or later platforms
where it should have been non-zero.
it seems intel_display_device_info_print() was specially crafted so we
don't have the i915/xe pointer and instead only operate on the info.
Yeah, we can get back to i915/xe, but I'm not sure about doing that.
What if we initialize it with U32_MAX and check for it here. I think it
would take a long time before that becomes invalid.
Another option is not to bother with the additional message in platforms
without it, and just print it unconditionally, like patch 1 does for the
rel/step.
Thoughts?
Lucas De Marchi
Matt
#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name,
str_yes_no(info->name))
DEV_INFO_DISPLAY_FOR_EACH_FLAG(PRINT_FLAG);
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
b/drivers/gpu/drm/i915/display/intel_display_device.h
index 79e9f1c3e241..f8a2ada1a4ec 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -123,6 +123,8 @@ struct intel_display_runtime_info {
u8 fbc_mask;
+ u32 pica_id;
+
bool has_hdcp;
bool has_dmc;
bool has_dsc;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 27dc903f0553..9d70635508ae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6362,6 +6362,8 @@ enum skl_power_gate {
#define TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT REG_BIT(1)
#define TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT REG_BIT(0)
+#define PICA_DEVICE_ID _MMIO(0x16fe00)
+
#define PRIMARY_SPI_TRIGGER_MMIO(0x102040)
#define PRIMARY_SPI_ADDRESS_MMIO(0x102080)
#define PRIMARY_SPI_REGIONID _MMIO(0x102084)
--
2.40.1
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
Re: [PATCH 2/2] drm/i915/display: Print PICA version
On Fri, Dec 08, 2023 at 06:31:37AM -0800, Lucas De Marchi wrote:
> The PICA_DEVICE_ID follows the same format as other GMD_ID registers
> (graphics, display and media). Currently the only use for it is
> informational for developers while checking the differences between
> machines with the same platform. Print the raw number as there's no need
> for the driver to check any of the fields.
>
> In future this may change, but then the IP version in
> struct intel_display_runtime_info will need some refactor to allow
> keeping both the display engine and PICA versions.
>
> Lastly, keeping the PICA version around in the display runtime info
> matches its current use. If that changes in future, then it may need
> to be moved to the device info.
>
> Signed-off-by: Lucas De Marchi
> ---
> drivers/gpu/drm/i915/display/intel_display_device.c | 5 +
> drivers/gpu/drm/i915/display/intel_display_device.h | 2 ++
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c
> b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 5d1084a98b93..d9d1be008e4c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -956,6 +956,9 @@ static void
> __intel_display_device_info_runtime_init(struct drm_i915_private *i9
> BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->cpu_transcoder_mask) <
> I915_MAX_TRANSCODERS);
> BUILD_BUG_ON(BITS_PER_TYPE(display_runtime->port_mask) <
> I915_MAX_PORTS);
>
> + if (HAS_GMD_ID(i915))
> + display_runtime->pica_id = intel_de_read(i915, PICA_DEVICE_ID);
> +
> /* This covers both ULT and ULX */
> if (IS_HASWELL_ULT(i915) || IS_BROADWELL_ULT(i915))
> display_runtime->port_mask &= ~BIT(PORT_D);
> @@ -1124,6 +1127,8 @@ void intel_display_device_info_print(const struct
> intel_display_device_info *inf
> {
> drm_printf(p, "display version: %u.%02u.%02u\n",
> runtime->ip.ver, runtime->ip.rel, runtime->ip.step);
> + if (runtime->pica_id)
> + drm_printf(p, "PICA version: %#08x\n", runtime->pica_id);
The actual, expected PICA version number on most (all?) MTL devices will
be 0.0. Should we make the condition here HAS_GMD_ID(i915) instead so
that we print the value whenever we read it? That will also help us
notice if the PICA register is incorrectly 0 on LNL or later platforms
where it should have been non-zero.
Matt
>
> #define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name,
> str_yes_no(info->name))
> DEV_INFO_DISPLAY_FOR_EACH_FLAG(PRINT_FLAG);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 79e9f1c3e241..f8a2ada1a4ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -123,6 +123,8 @@ struct intel_display_runtime_info {
>
> u8 fbc_mask;
>
> + u32 pica_id;
> +
> bool has_hdcp;
> bool has_dmc;
> bool has_dsc;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 27dc903f0553..9d70635508ae 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6362,6 +6362,8 @@ enum skl_power_gate {
> #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT REG_BIT(1)
> #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT REG_BIT(0)
>
> +#define PICA_DEVICE_ID _MMIO(0x16fe00)
> +
> #define PRIMARY_SPI_TRIGGER _MMIO(0x102040)
> #define PRIMARY_SPI_ADDRESS _MMIO(0x102080)
> #define PRIMARY_SPI_REGIONID _MMIO(0x102084)
> --
> 2.40.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
