| Message ID | 20231208143137.2402239-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/display: Also print raw step number | expand |
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 <lucas.demarchi@intel.com> > --- > 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 >
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 <lucas.demarchi@intel.com> >> --- >> 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
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 <lucas.demarchi@intel.com> > > > --- > > > 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 <matthew.d.roper@intel.com> > > 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
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); #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)
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 <lucas.demarchi@intel.com> --- 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(+)