| Message ID | 20211020002353.193893-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Add struct to hold IP version | expand |
On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: > The constant platform display version is not using this new struct but > the runtime variant will definitely use it. Cc: Some more folks to hijack this thread. Sorry! ;) We added runtime info to i915, because we had this idea and goal of turning the device info to a truly const pointer to the info structures in i915_pci.c that are stored in rodata. The idea was that we'll have a complete split of mutable and immutable device data, with all the mutable data in runtime info. Alas, we never got there. More and more data that was mostly const but sometimes needed tweaking kept piling up. mkwrite_device_info() was supposed to be a clue not to modify device info runtime, but instead it proliferated. Now we have places like intel_fbc_init() disabling FBC through that. But most importantly, we have fusing that considerably changes the device info, and the copying all of that data over to runtime info probably isn't worth it. Should we just acknowledge that the runtime info is useless, and move some of that data to intel_device_info and some of it elsewhere in i915? BR, Jani.
On Tue, 2021-10-19 at 17:23 -0700, José Roberto de Souza wrote: > Adding a structure to standardize access to IP versioning as future > platforms will have this information populated at runtime. > > The constant platform display version is not using this new struct > but > the runtime variant will definitely use it. > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Cc: Matt Atwood <matthew.s.atwood@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++------ > drivers/gpu/drm/i915/i915_pci.c | 18 +++++++++------ > --- > drivers/gpu/drm/i915/intel_device_info.c | 19 ++++++++++++----- > -- > drivers/gpu/drm/i915/intel_device_info.h | 12 ++++++++---- > .../gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > 6 files changed, 37 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 1e5b75ae99329..bdf85d202c55c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -808,7 +808,7 @@ int i915_driver_probe(struct pci_dev *pdev, const > struct pci_device_id *ent) > return PTR_ERR(i915); > > /* Disable nuclear pageflip by default on pre-ILK */ > - if (!i915->params.nuclear_pageflip && match_info->graphics_ver > < 5) > + if (!i915->params.nuclear_pageflip && match_info->graphics.ver > < 5) I don't find any difference on this and the similar modifications below. Am I missing something? -caz > i915->drm.driver_features &= ~DRIVER_ATOMIC; > > /* > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 12256218634f4..26b6e2b8bb5e8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1327,15 +1327,15 @@ static inline struct drm_i915_private > *pdev_to_i915(struct pci_dev *pdev) > > #define IP_VER(ver, rel) ((ver) << 8 | (rel)) > > -#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > >graphics_ver) > -#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > ->graphics_ver, \ > - INTEL_INFO(i915)- > >graphics_rel) > +#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > >graphics.ver) > +#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > ->graphics.ver, \ > + INTEL_INFO(i915)- > >graphics.rel) > #define IS_GRAPHICS_VER(i915, from, until) \ > (GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until)) > > -#define MEDIA_VER(i915) (INTEL_INFO(i915)- > >media_ver) > -#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > >media_ver, \ > - INTEL_INFO(i915)- > >media_rel) > +#define MEDIA_VER(i915) (INTEL_INFO(i915)- > >media.ver) > +#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > >media.arch, \ > + INTEL_INFO(i915)- > >media.rel) > #define IS_MEDIA_VER(i915, from, until) \ > (MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until)) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > b/drivers/gpu/drm/i915/i915_pci.c > index 169837de395d3..5e6795853dc31 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -32,8 +32,8 @@ > > #define PLATFORM(x) .platform = (x) > #define GEN(x) \ > - .graphics_ver = (x), \ > - .media_ver = (x), \ > + .graphics.ver = (x), \ > + .media.ver = (x), \ > .display.ver = (x) > > #define I845_PIPE_OFFSETS \ > @@ -899,7 +899,7 @@ static const struct intel_device_info rkl_info = > { > static const struct intel_device_info dg1_info = { > GEN12_FEATURES, > DGFX_FEATURES, > - .graphics_rel = 10, > + .graphics.rel = 10, > PLATFORM(INTEL_DG1), > .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | > BIT(PIPE_D), > .require_force_probe = 1, > @@ -986,8 +986,8 @@ static const struct intel_device_info adl_p_info > = { > I915_GTT_PAGE_SIZE_2M > > #define XE_HP_FEATURES \ > - .graphics_ver = 12, \ > - .graphics_rel = 50, \ > + .graphics.ver = 12, \ > + .graphics.rel = 50, \ > XE_HP_PAGE_SIZES, \ > .dma_mask_size = 46, \ > .has_64bit_reloc = 1, \ > @@ -1005,8 +1005,8 @@ static const struct intel_device_info > adl_p_info = { > .ppgtt_type = INTEL_PPGTT_FULL > > #define XE_HPM_FEATURES \ > - .media_ver = 12, \ > - .media_rel = 50 > + .media.ver = 12, \ > + .media.rel = 50 > > __maybe_unused > static const struct intel_device_info xehpsdv_info = { > @@ -1030,8 +1030,8 @@ static const struct intel_device_info dg2_info > = { > XE_HPM_FEATURES, > XE_LPD_FEATURES, > DGFX_FEATURES, > - .graphics_rel = 55, > - .media_rel = 55, > + .graphics.rel = 55, > + .media.rel = 55, > PLATFORM(INTEL_DG2), > .platform_engine_mask = > BIT(RCS0) | BIT(BCS0) | > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index 305facedd2841..6e6b317bc33ce 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -97,17 +97,22 @@ static const char *iommu_name(void) > void intel_device_info_print_static(const struct intel_device_info > *info, > struct drm_printer *p) > { > - if (info->graphics_rel) > - drm_printf(p, "graphics version: %u.%02u\n", info- > >graphics_ver, info->graphics_rel); > + if (info->graphics.rel) > + drm_printf(p, "graphics version: %u.%02u\n", info- > >graphics.ver, > + info->graphics.rel); > else > - drm_printf(p, "graphics version: %u\n", info- > >graphics_ver); > + drm_printf(p, "graphics version: %u\n", info- > >graphics.ver); > > - if (info->media_rel) > - drm_printf(p, "media version: %u.%02u\n", info- > >media_ver, info->media_rel); > + if (info->media.rel) > + drm_printf(p, "media version: %u.%02u\n", info- > >media.ver, info->media.rel); > else > - drm_printf(p, "media version: %u\n", info->media_ver); > + drm_printf(p, "media version: %u\n", info->media.ver); > + > + if (info->display.rel) > + drm_printf(p, "display version: %u.%02u\n", info- > >display.ver, info->display.rel); > + else > + drm_printf(p, "display version: %u\n", info- > >display.ver); > > - drm_printf(p, "display version: %u\n", info->display.ver); > drm_printf(p, "gt: %d\n", info->gt); > drm_printf(p, "iommu: %s\n", iommu_name()); > drm_printf(p, "memory-regions: %x\n", info->memory_regions); > diff --git a/drivers/gpu/drm/i915/intel_device_info.h > b/drivers/gpu/drm/i915/intel_device_info.h > index 8e6f48d1eb7bc..669f0d26c3c38 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -166,11 +166,14 @@ enum intel_ppgtt_type { > func(overlay_needs_physical); \ > func(supports_tv); > > +struct ip_version { > + u8 ver; > + u8 rel; > +}; > + > struct intel_device_info { > - u8 graphics_ver; > - u8 graphics_rel; > - u8 media_ver; > - u8 media_rel; > + struct ip_version graphics; > + struct ip_version media; > > intel_engine_mask_t platform_engine_mask; /* Engines supported > by the HW */ > > @@ -200,6 +203,7 @@ struct intel_device_info { > > struct { > u8 ver; > + u8 rel; > > #define DEFINE_FLAG(name) u8 name:1 > DEV_INFO_DISPLAY_FOR_EACH_FLAG(DEFINE_FLAG); > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index 4f81801468881..9ab3f284d1dd9 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -165,7 +165,7 @@ struct drm_i915_private *mock_gem_device(void) > /* Using the global GTT may ask questions about KMS users, so > prepare */ > drm_mode_config_init(&i915->drm); > > - mkwrite_device_info(i915)->graphics_ver = -1; > + mkwrite_device_info(i915)->graphics.ver = -1; > > mkwrite_device_info(i915)->page_sizes = > I915_GTT_PAGE_SIZE_4K |
On Wed, 2021-10-20 at 15:00 +0000, Yokoyama, Caz wrote: > On Tue, 2021-10-19 at 17:23 -0700, José Roberto de Souza wrote: > > Adding a structure to standardize access to IP versioning as future > > platforms will have this information populated at runtime. > > > > The constant platform display version is not using this new struct > > but > > the runtime variant will definitely use it. > > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Cc: Matt Atwood <matthew.s.atwood@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++------ > > drivers/gpu/drm/i915/i915_pci.c | 18 +++++++++------ > > --- > > drivers/gpu/drm/i915/intel_device_info.c | 19 ++++++++++++----- > > -- > > drivers/gpu/drm/i915/intel_device_info.h | 12 ++++++++---- > > .../gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > > 6 files changed, 37 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 1e5b75ae99329..bdf85d202c55c 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -808,7 +808,7 @@ int i915_driver_probe(struct pci_dev *pdev, const > > struct pci_device_id *ent) > > return PTR_ERR(i915); > > > > /* Disable nuclear pageflip by default on pre-ILK */ > > - if (!i915->params.nuclear_pageflip && match_info->graphics_ver > > < 5) > > + if (!i915->params.nuclear_pageflip && match_info->graphics.ver > > < 5) > I don't find any difference on this and the similar modifications > below. Am I missing something? Changing u8 graphics_ver to struct ip_version that contains a member called ver. So only changing '_' to '.' in most places. > -caz > > > i915->drm.driver_features &= ~DRIVER_ATOMIC; > > > > /* > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 12256218634f4..26b6e2b8bb5e8 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1327,15 +1327,15 @@ static inline struct drm_i915_private > > *pdev_to_i915(struct pci_dev *pdev) > > > > #define IP_VER(ver, rel) ((ver) << 8 | (rel)) > > > > -#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > graphics_ver) > > -#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > ->graphics_ver, \ > > - INTEL_INFO(i915)- > > > graphics_rel) > > +#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > graphics.ver) > > +#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > ->graphics.ver, \ > > + INTEL_INFO(i915)- > > > graphics.rel) > > #define IS_GRAPHICS_VER(i915, from, until) \ > > (GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until)) > > > > -#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > media_ver) > > -#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > media_ver, \ > > - INTEL_INFO(i915)- > > > media_rel) > > +#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > media.ver) > > +#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > media.arch, \ > > + INTEL_INFO(i915)- > > > media.rel) > > #define IS_MEDIA_VER(i915, from, until) \ > > (MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until)) > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > b/drivers/gpu/drm/i915/i915_pci.c > > index 169837de395d3..5e6795853dc31 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -32,8 +32,8 @@ > > > > #define PLATFORM(x) .platform = (x) > > #define GEN(x) \ > > - .graphics_ver = (x), \ > > - .media_ver = (x), \ > > + .graphics.ver = (x), \ > > + .media.ver = (x), \ > > .display.ver = (x) > > > > #define I845_PIPE_OFFSETS \ > > @@ -899,7 +899,7 @@ static const struct intel_device_info rkl_info = > > { > > static const struct intel_device_info dg1_info = { > > GEN12_FEATURES, > > DGFX_FEATURES, > > - .graphics_rel = 10, > > + .graphics.rel = 10, > > PLATFORM(INTEL_DG1), > > .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | > > BIT(PIPE_D), > > .require_force_probe = 1, > > @@ -986,8 +986,8 @@ static const struct intel_device_info adl_p_info > > = { > > I915_GTT_PAGE_SIZE_2M > > > > #define XE_HP_FEATURES \ > > - .graphics_ver = 12, \ > > - .graphics_rel = 50, \ > > + .graphics.ver = 12, \ > > + .graphics.rel = 50, \ > > XE_HP_PAGE_SIZES, \ > > .dma_mask_size = 46, \ > > .has_64bit_reloc = 1, \ > > @@ -1005,8 +1005,8 @@ static const struct intel_device_info > > adl_p_info = { > > .ppgtt_type = INTEL_PPGTT_FULL > > > > #define XE_HPM_FEATURES \ > > - .media_ver = 12, \ > > - .media_rel = 50 > > + .media.ver = 12, \ > > + .media.rel = 50 > > > > __maybe_unused > > static const struct intel_device_info xehpsdv_info = { > > @@ -1030,8 +1030,8 @@ static const struct intel_device_info dg2_info > > = { > > XE_HPM_FEATURES, > > XE_LPD_FEATURES, > > DGFX_FEATURES, > > - .graphics_rel = 55, > > - .media_rel = 55, > > + .graphics.rel = 55, > > + .media.rel = 55, > > PLATFORM(INTEL_DG2), > > .platform_engine_mask = > > BIT(RCS0) | BIT(BCS0) | > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > b/drivers/gpu/drm/i915/intel_device_info.c > > index 305facedd2841..6e6b317bc33ce 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > @@ -97,17 +97,22 @@ static const char *iommu_name(void) > > void intel_device_info_print_static(const struct intel_device_info > > *info, > > struct drm_printer *p) > > { > > - if (info->graphics_rel) > > - drm_printf(p, "graphics version: %u.%02u\n", info- > > > graphics_ver, info->graphics_rel); > > + if (info->graphics.rel) > > + drm_printf(p, "graphics version: %u.%02u\n", info- > > > graphics.ver, > > + info->graphics.rel); > > else > > - drm_printf(p, "graphics version: %u\n", info- > > > graphics_ver); > > + drm_printf(p, "graphics version: %u\n", info- > > > graphics.ver); > > > > - if (info->media_rel) > > - drm_printf(p, "media version: %u.%02u\n", info- > > > media_ver, info->media_rel); > > + if (info->media.rel) > > + drm_printf(p, "media version: %u.%02u\n", info- > > > media.ver, info->media.rel); > > else > > - drm_printf(p, "media version: %u\n", info->media_ver); > > + drm_printf(p, "media version: %u\n", info->media.ver); > > + > > + if (info->display.rel) > > + drm_printf(p, "display version: %u.%02u\n", info- > > > display.ver, info->display.rel); > > + else > > + drm_printf(p, "display version: %u\n", info- > > > display.ver); > > > > - drm_printf(p, "display version: %u\n", info->display.ver); > > drm_printf(p, "gt: %d\n", info->gt); > > drm_printf(p, "iommu: %s\n", iommu_name()); > > drm_printf(p, "memory-regions: %x\n", info->memory_regions); > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h > > b/drivers/gpu/drm/i915/intel_device_info.h > > index 8e6f48d1eb7bc..669f0d26c3c38 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -166,11 +166,14 @@ enum intel_ppgtt_type { > > func(overlay_needs_physical); \ > > func(supports_tv); > > > > +struct ip_version { > > + u8 ver; > > + u8 rel; > > +}; > > + > > struct intel_device_info { > > - u8 graphics_ver; > > - u8 graphics_rel; > > - u8 media_ver; > > - u8 media_rel; > > + struct ip_version graphics; > > + struct ip_version media; > > > > intel_engine_mask_t platform_engine_mask; /* Engines supported > > by the HW */ > > > > @@ -200,6 +203,7 @@ struct intel_device_info { > > > > struct { > > u8 ver; > > + u8 rel; > > > > #define DEFINE_FLAG(name) u8 name:1 > > DEV_INFO_DISPLAY_FOR_EACH_FLAG(DEFINE_FLAG); > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > index 4f81801468881..9ab3f284d1dd9 100644 > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > @@ -165,7 +165,7 @@ struct drm_i915_private *mock_gem_device(void) > > /* Using the global GTT may ask questions about KMS users, so > > prepare */ > > drm_mode_config_init(&i915->drm); > > > > - mkwrite_device_info(i915)->graphics_ver = -1; > > + mkwrite_device_info(i915)->graphics.ver = -1; > > > > mkwrite_device_info(i915)->page_sizes = > > I915_GTT_PAGE_SIZE_4K |
On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote: > On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: > > The constant platform display version is not using this new struct but > > the runtime variant will definitely use it. > > Cc: Some more folks to hijack this thread. Sorry! ;) > > We added runtime info to i915, because we had this idea and goal of > turning the device info to a truly const pointer to the info structures > in i915_pci.c that are stored in rodata. The idea was that we'll have a > complete split of mutable and immutable device data, with all the > mutable data in runtime info. > > Alas, we never got there. More and more data that was mostly const but > sometimes needed tweaking kept piling up. mkwrite_device_info() was > supposed to be a clue not to modify device info runtime, but instead it > proliferated. Now we have places like intel_fbc_init() disabling FBC > through that. But most importantly, we have fusing that considerably > changes the device info, and the copying all of that data over to > runtime info probably isn't worth it. > > Should we just acknowledge that the runtime info is useless, and move > some of that data to intel_device_info and some of it elsewhere in i915? With newer platforms getting more and more modular, I believe we will need to store even more mutable platform information. In my opinion a separation of immutable and mutable platform information is cleaner and easier to maintain. > > > BR, > Jani. >
On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@intel.com> wrote: > On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote: >> On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: >> > The constant platform display version is not using this new struct but >> > the runtime variant will definitely use it. >> >> Cc: Some more folks to hijack this thread. Sorry! ;) >> >> We added runtime info to i915, because we had this idea and goal of >> turning the device info to a truly const pointer to the info structures >> in i915_pci.c that are stored in rodata. The idea was that we'll have a >> complete split of mutable and immutable device data, with all the >> mutable data in runtime info. >> >> Alas, we never got there. More and more data that was mostly const but >> sometimes needed tweaking kept piling up. mkwrite_device_info() was >> supposed to be a clue not to modify device info runtime, but instead it >> proliferated. Now we have places like intel_fbc_init() disabling FBC >> through that. But most importantly, we have fusing that considerably >> changes the device info, and the copying all of that data over to >> runtime info probably isn't worth it. >> >> Should we just acknowledge that the runtime info is useless, and move >> some of that data to intel_device_info and some of it elsewhere in i915? > > With newer platforms getting more and more modular, I believe we will > need to store even more mutable platform information. > > In my opinion a separation of immutable and mutable platform > information is cleaner and easier to maintain. Yeah, that's kind of what the original point was with device and runtime info split. It's just that a lot of the supposedly immutable platform info has turned into mutable information. I think either we need to properly follow through with that idea, and only store a const struct intel_device_info * to the rodata in i915_pci.c, or just scrap it. None of this "almost immutable" business that we currently have. "Almost immutable" means "mutable". The main problem is that we'll still want to have the initial values in static data. One idea is something like this: struct intel_device_info { const struct intel_runtime_info *runtime_info; /* ... */ }; static const struct intel_device_info i965g_info = { .runtime_info = &i965g_initial_runtime_info; /* ... */ }; And things like .pipe_mask would be part of struct intel_runtime_info. You'd copy the stuff over from intel_device_info runtime_info member to i915->__runtime, but i915->__info would be a const pointer to the device info. You'd never access the runtime_info member after of intel_device_info after probe. It's just really painful, for instance because we already have two sets of flags, display and non-display, and those would be multiplied to mutable/immutable. And we should probably increase, not decrease, the split between display and non-display. The macro horror show of i915_pci.c would just grow worse. BR, Jani. > >> >> >> BR, >> Jani. >> >
On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote: >On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@intel.com> wrote: >> On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote: >>> On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: >>> > The constant platform display version is not using this new struct but >>> > the runtime variant will definitely use it. >>> >>> Cc: Some more folks to hijack this thread. Sorry! ;) >>> >>> We added runtime info to i915, because we had this idea and goal of >>> turning the device info to a truly const pointer to the info structures >>> in i915_pci.c that are stored in rodata. The idea was that we'll have a >>> complete split of mutable and immutable device data, with all the >>> mutable data in runtime info. >>> >>> Alas, we never got there. More and more data that was mostly const but >>> sometimes needed tweaking kept piling up. mkwrite_device_info() was >>> supposed to be a clue not to modify device info runtime, but instead it >>> proliferated. Now we have places like intel_fbc_init() disabling FBC >>> through that. But most importantly, we have fusing that considerably >>> changes the device info, and the copying all of that data over to >>> runtime info probably isn't worth it. >>> >>> Should we just acknowledge that the runtime info is useless, and move >>> some of that data to intel_device_info and some of it elsewhere in i915? >> >> With newer platforms getting more and more modular, I believe we will >> need to store even more mutable platform information. >> >> In my opinion a separation of immutable and mutable platform >> information is cleaner and easier to maintain. > >Yeah, that's kind of what the original point was with device and runtime >info split. It's just that a lot of the supposedly immutable platform >info has turned into mutable information. > >I think either we need to properly follow through with that idea, and >only store a const struct intel_device_info * to the rodata in >i915_pci.c, or just scrap it. None of this "almost immutable" business >that we currently have. "Almost immutable" means "mutable". > >The main problem is that we'll still want to have the initial values in >static data. One idea is something like this: > >struct intel_device_info { > const struct intel_runtime_info *runtime_info; > /* ... */ >}; > >static const struct intel_device_info i965g_info = { > .runtime_info = &i965g_initial_runtime_info; > /* ... */ >}; > >And things like .pipe_mask would be part of struct >intel_runtime_info. You'd copy the stuff over from intel_device_info >runtime_info member to i915->__runtime, but i915->__info would be a >const pointer to the device info. You'd never access the runtime_info >member after of intel_device_info after probe. I like this approach. I think the only problem would be that if someone inadvertently do a i915->__info->runtime_info they will be accessing the wrong data. So maybe to be clear do struct intel_device_info { const void *initial_runtime_info; /* ... */ }; static const struct intel_device_info i965g_info = { .initial_runtime_info = &i965g_initial_runtime_info; /* ... */ }; this would make it opaque and even hint by the name so the developer is not tempted to add a cast. Lucas De Marchi > >It's just really painful, for instance because we already have two sets >of flags, display and non-display, and those would be multiplied to >mutable/immutable. And we should probably increase, not decrease, the >split between display and non-display. The macro horror show of >i915_pci.c would just grow worse. > > >BR, >Jani. > > > >> >>> >>> >>> BR, >>> Jani. >>> >> > >-- >Jani Nikula, Intel Open Source Graphics Center
On Wed, 2021-10-20 at 19:19 +0000, Souza, Jose wrote: > On Wed, 2021-10-20 at 15:00 +0000, Yokoyama, Caz wrote: > > On Tue, 2021-10-19 at 17:23 -0700, José Roberto de Souza wrote: > > > Adding a structure to standardize access to IP versioning as > > > future > > > platforms will have this information populated at runtime. > > > > > > The constant platform display version is not using this new > > > struct > > > but > > > the runtime variant will definitely use it. > > > > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > Cc: Matt Atwood <matthew.s.atwood@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++------ > > > drivers/gpu/drm/i915/i915_pci.c | 18 +++++++++-- > > > ---- > > > --- > > > drivers/gpu/drm/i915/intel_device_info.c | 19 ++++++++++++- > > > ---- > > > -- > > > drivers/gpu/drm/i915/intel_device_info.h | 12 ++++++++---- > > > .../gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > > > 6 files changed, 37 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > b/drivers/gpu/drm/i915/i915_drv.c > > > index 1e5b75ae99329..bdf85d202c55c 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -808,7 +808,7 @@ int i915_driver_probe(struct pci_dev *pdev, > > > const > > > struct pci_device_id *ent) > > > return PTR_ERR(i915); > > > > > > /* Disable nuclear pageflip by default on pre-ILK */ > > > - if (!i915->params.nuclear_pageflip && match_info- > > > >graphics_ver > > > < 5) > > > + if (!i915->params.nuclear_pageflip && match_info- > > > >graphics.ver > > > < 5) > > I don't find any difference on this and the similar modifications > > below. Am I missing something? > > Changing u8 graphics_ver to struct ip_version that contains a member > called ver. > So only changing '_' to '.' in most places. > > > -caz > > > > > i915->drm.driver_features &= ~DRIVER_ATOMIC; > > > > > > /* > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 12256218634f4..26b6e2b8bb5e8 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1327,15 +1327,15 @@ static inline struct drm_i915_private > > > *pdev_to_i915(struct pci_dev *pdev) > > > > > > #define IP_VER(ver, rel) ((ver) << 8 | (rel)) > > > > > > -#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > > graphics_ver) > > > -#define > > > GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > > ->graphics_ver, \ > > > - INTEL_INFO(i915)- > > > > graphics_rel) > > > +#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > > graphics.ver) > > > +#define > > > GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > > ->graphics.ver, \ > > > + INTEL_INFO(i915)- > > > > graphics.rel) > > > #define IS_GRAPHICS_VER(i915, from, until) \ > > > (GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= > > > (until)) > > > > > > -#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > > media_ver) > > > -#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > > media_ver, \ > > > - INTEL_INFO(i915)- > > > > media_rel) > > > +#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > > media.ver) > > > +#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > > media.arch, \ > > > + INTEL_INFO(i915)- > > > > media.rel) > > > #define IS_MEDIA_VER(i915, from, until) \ > > > (MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until)) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > > b/drivers/gpu/drm/i915/i915_pci.c > > > index 169837de395d3..5e6795853dc31 100644 > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > @@ -32,8 +32,8 @@ > > > > > > #define PLATFORM(x) .platform = (x) > > > #define GEN(x) \ > > > - .graphics_ver = (x), \ > > > - .media_ver = (x), \ > > > + .graphics.ver = (x), \ > > > + .media.ver = (x), \ > > > .display.ver = (x) > > > > > > #define I845_PIPE_OFFSETS \ > > > @@ -899,7 +899,7 @@ static const struct intel_device_info > > > rkl_info = > > > { > > > static const struct intel_device_info dg1_info = { > > > GEN12_FEATURES, > > > DGFX_FEATURES, > > > - .graphics_rel = 10, > > > + .graphics.rel = 10, > > > PLATFORM(INTEL_DG1), > > > .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | > > > BIT(PIPE_D), > > > .require_force_probe = 1, > > > @@ -986,8 +986,8 @@ static const struct intel_device_info > > > adl_p_info > > > = { > > > I915_GTT_PAGE_SIZE_2M > > > > > > #define XE_HP_FEATURES \ > > > - .graphics_ver = 12, \ > > > - .graphics_rel = 50, \ > > > + .graphics.ver = 12, \ > > > + .graphics.rel = 50, \ > > > XE_HP_PAGE_SIZES, \ > > > .dma_mask_size = 46, \ > > > .has_64bit_reloc = 1, \ > > > @@ -1005,8 +1005,8 @@ static const struct intel_device_info > > > adl_p_info = { > > > .ppgtt_type = INTEL_PPGTT_FULL > > > > > > #define XE_HPM_FEATURES \ > > > - .media_ver = 12, \ > > > - .media_rel = 50 > > > + .media.ver = 12, \ > > > + .media.rel = 50 > > > > > > __maybe_unused > > > static const struct intel_device_info xehpsdv_info = { > > > @@ -1030,8 +1030,8 @@ static const struct intel_device_info > > > dg2_info > > > = { > > > XE_HPM_FEATURES, > > > XE_LPD_FEATURES, > > > DGFX_FEATURES, > > > - .graphics_rel = 55, > > > - .media_rel = 55, > > > + .graphics.rel = 55, > > > + .media.rel = 55, > > > PLATFORM(INTEL_DG2), > > > .platform_engine_mask = > > > BIT(RCS0) | BIT(BCS0) | > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > > b/drivers/gpu/drm/i915/intel_device_info.c > > > index 305facedd2841..6e6b317bc33ce 100644 > > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > > @@ -97,17 +97,22 @@ static const char *iommu_name(void) > > > void intel_device_info_print_static(const struct > > > intel_device_info > > > *info, > > > struct drm_printer *p) > > > { > > > - if (info->graphics_rel) > > > - drm_printf(p, "graphics version: %u.%02u\n", info- > > > > graphics_ver, info->graphics_rel); > > > + if (info->graphics.rel) > > > + drm_printf(p, "graphics version: %u.%02u\n", info- > > > > graphics.ver, > > > + info->graphics.rel); > > > else > > > - drm_printf(p, "graphics version: %u\n", info- > > > > graphics_ver); > > > + drm_printf(p, "graphics version: %u\n", info- > > > > graphics.ver); > > > > > > - if (info->media_rel) > > > - drm_printf(p, "media version: %u.%02u\n", info- > > > > media_ver, info->media_rel); > > > + if (info->media.rel) > > > + drm_printf(p, "media version: %u.%02u\n", info- > > > > media.ver, info->media.rel); > > > else > > > - drm_printf(p, "media version: %u\n", info- > > > >media_ver); > > > + drm_printf(p, "media version: %u\n", info- > > > >media.ver); > > > + > > > + if (info->display.rel) > > > + drm_printf(p, "display version: %u.%02u\n", info- > > > > display.ver, info->display.rel); > > > + else > > > + drm_printf(p, "display version: %u\n", info- > > > > display.ver); > > > > > > - drm_printf(p, "display version: %u\n", info->display.ver); > > > drm_printf(p, "gt: %d\n", info->gt); > > > drm_printf(p, "iommu: %s\n", iommu_name()); > > > drm_printf(p, "memory-regions: %x\n", info- > > > >memory_regions); > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h > > > b/drivers/gpu/drm/i915/intel_device_info.h > > > index 8e6f48d1eb7bc..669f0d26c3c38 100644 > > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > > @@ -166,11 +166,14 @@ enum intel_ppgtt_type { > > > func(overlay_needs_physical); \ > > > func(supports_tv); > > > > > > +struct ip_version { > > > + u8 ver; > > > + u8 rel; > > > +}; > > > + > > > struct intel_device_info { > > > - u8 graphics_ver; > > > - u8 graphics_rel; > > > - u8 media_ver; > > > - u8 media_rel; > > > + struct ip_version graphics; > > > + struct ip_version media; > > > > > > intel_engine_mask_t platform_engine_mask; /* Engines > > > supported > > > by the HW */ > > > > > > @@ -200,6 +203,7 @@ struct intel_device_info { > > > > > > struct { > > > u8 ver; > > > + u8 rel; Even this is mentioned in the comment as "need for runtime variant", I have no idea why it is needed because it is copied on i915_drv.c and shown by drm_printf() in intel_device_info.c. If it is really needed, Reviewed-by: Caz Yokoyama <caz.yokoyama@intel.com> -caz > > > > > > #define DEFINE_FLAG(name) u8 name:1 > > > DEV_INFO_DISPLAY_FOR_EACH_FLAG(DEFINE_FLAG); > > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > index 4f81801468881..9ab3f284d1dd9 100644 > > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > @@ -165,7 +165,7 @@ struct drm_i915_private > > > *mock_gem_device(void) > > > /* Using the global GTT may ask questions about KMS users, > > > so > > > prepare */ > > > drm_mode_config_init(&i915->drm); > > > > > > - mkwrite_device_info(i915)->graphics_ver = -1; > > > + mkwrite_device_info(i915)->graphics.ver = -1; > > > > > > mkwrite_device_info(i915)->page_sizes = > > > I915_GTT_PAGE_SIZE_4K |
On Fri, 22 Oct 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote: >>On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@intel.com> wrote: >>> On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote: >>>> On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: >>>> > The constant platform display version is not using this new struct but >>>> > the runtime variant will definitely use it. >>>> >>>> Cc: Some more folks to hijack this thread. Sorry! ;) >>>> >>>> We added runtime info to i915, because we had this idea and goal of >>>> turning the device info to a truly const pointer to the info structures >>>> in i915_pci.c that are stored in rodata. The idea was that we'll have a >>>> complete split of mutable and immutable device data, with all the >>>> mutable data in runtime info. >>>> >>>> Alas, we never got there. More and more data that was mostly const but >>>> sometimes needed tweaking kept piling up. mkwrite_device_info() was >>>> supposed to be a clue not to modify device info runtime, but instead it >>>> proliferated. Now we have places like intel_fbc_init() disabling FBC >>>> through that. But most importantly, we have fusing that considerably >>>> changes the device info, and the copying all of that data over to >>>> runtime info probably isn't worth it. >>>> >>>> Should we just acknowledge that the runtime info is useless, and move >>>> some of that data to intel_device_info and some of it elsewhere in i915? >>> >>> With newer platforms getting more and more modular, I believe we will >>> need to store even more mutable platform information. >>> >>> In my opinion a separation of immutable and mutable platform >>> information is cleaner and easier to maintain. >> >>Yeah, that's kind of what the original point was with device and runtime >>info split. It's just that a lot of the supposedly immutable platform >>info has turned into mutable information. >> >>I think either we need to properly follow through with that idea, and >>only store a const struct intel_device_info * to the rodata in >>i915_pci.c, or just scrap it. None of this "almost immutable" business >>that we currently have. "Almost immutable" means "mutable". >> >>The main problem is that we'll still want to have the initial values in >>static data. One idea is something like this: >> >>struct intel_device_info { >> const struct intel_runtime_info *runtime_info; >> /* ... */ >>}; >> >>static const struct intel_device_info i965g_info = { >> .runtime_info = &i965g_initial_runtime_info; >> /* ... */ >>}; >> >>And things like .pipe_mask would be part of struct >>intel_runtime_info. You'd copy the stuff over from intel_device_info >>runtime_info member to i915->__runtime, but i915->__info would be a >>const pointer to the device info. You'd never access the runtime_info >>member after of intel_device_info after probe. > > > I like this approach. I think the only problem would be that if someone > inadvertently do a i915->__info->runtime_info they will be accessing the > wrong data. So maybe to be clear do > > struct intel_device_info { > const void *initial_runtime_info; > /* ... */ > }; > > static const struct intel_device_info i965g_info = { > .initial_runtime_info = &i965g_initial_runtime_info; > /* ... */ > }; > > this would make it opaque and even hint by the name so the developer is > not tempted to add a cast. I think that's all fairly straightforward. Any ideas on how to do the flags split cleanly, though? I already dislike the DEV_INFO_FOR_EACH_FLAG() and DEV_INFO_DISPLAY_FOR_EACH_FLAG() split. BR, Jani. > > Lucas De Marchi > >> >>It's just really painful, for instance because we already have two sets >>of flags, display and non-display, and those would be multiplied to >>mutable/immutable. And we should probably increase, not decrease, the >>split between display and non-display. The macro horror show of >>i915_pci.c would just grow worse. >> >> >>BR, >>Jani. >> >> >> >>> >>>> >>>> >>>> BR, >>>> Jani. >>>> >>> >> >>-- >>Jani Nikula, Intel Open Source Graphics Center
On Fri, 2021-10-22 at 21:26 +0000, Yokoyama, Caz wrote: > On Wed, 2021-10-20 at 19:19 +0000, Souza, Jose wrote: > > On Wed, 2021-10-20 at 15:00 +0000, Yokoyama, Caz wrote: > > > On Tue, 2021-10-19 at 17:23 -0700, José Roberto de Souza wrote: > > > > Adding a structure to standardize access to IP versioning as > > > > future > > > > platforms will have this information populated at runtime. > > > > > > > > The constant platform display version is not using this new > > > > struct > > > > but > > > > the runtime variant will definitely use it. > > > > > > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > > Cc: Matt Atwood <matthew.s.atwood@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > > > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++------ > > > > drivers/gpu/drm/i915/i915_pci.c | 18 +++++++++-- > > > > ---- > > > > --- > > > > drivers/gpu/drm/i915/intel_device_info.c | 19 ++++++++++++- > > > > ---- > > > > -- > > > > drivers/gpu/drm/i915/intel_device_info.h | 12 ++++++++---- > > > > .../gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > > > > 6 files changed, 37 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > index 1e5b75ae99329..bdf85d202c55c 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -808,7 +808,7 @@ int i915_driver_probe(struct pci_dev *pdev, > > > > const > > > > struct pci_device_id *ent) > > > > return PTR_ERR(i915); > > > > > > > > /* Disable nuclear pageflip by default on pre-ILK */ > > > > - if (!i915->params.nuclear_pageflip && match_info- > > > > > graphics_ver > > > > < 5) > > > > + if (!i915->params.nuclear_pageflip && match_info- > > > > > graphics.ver > > > > < 5) > > > I don't find any difference on this and the similar modifications > > > below. Am I missing something? > > > > Changing u8 graphics_ver to struct ip_version that contains a member > > called ver. > > So only changing '_' to '.' in most places. > > > > > -caz > > > > > > > i915->drm.driver_features &= ~DRIVER_ATOMIC; > > > > > > > > /* > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 12256218634f4..26b6e2b8bb5e8 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -1327,15 +1327,15 @@ static inline struct drm_i915_private > > > > *pdev_to_i915(struct pci_dev *pdev) > > > > > > > > #define IP_VER(ver, rel) ((ver) << 8 | (rel)) > > > > > > > > -#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > > > graphics_ver) > > > > -#define > > > > GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > > > ->graphics_ver, \ > > > > - INTEL_INFO(i915)- > > > > > graphics_rel) > > > > +#define GRAPHICS_VER(i915) (INTEL_INFO(i915)- > > > > > graphics.ver) > > > > +#define > > > > GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915) > > > > ->graphics.ver, \ > > > > + INTEL_INFO(i915)- > > > > > graphics.rel) > > > > #define IS_GRAPHICS_VER(i915, from, until) \ > > > > (GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= > > > > (until)) > > > > > > > > -#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > > > media_ver) > > > > -#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > > > media_ver, \ > > > > - INTEL_INFO(i915)- > > > > > media_rel) > > > > +#define MEDIA_VER(i915) (INTEL_INFO(i915)- > > > > > media.ver) > > > > +#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)- > > > > > media.arch, \ > > > > + INTEL_INFO(i915)- > > > > > media.rel) > > > > #define IS_MEDIA_VER(i915, from, until) \ > > > > (MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until)) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > > > b/drivers/gpu/drm/i915/i915_pci.c > > > > index 169837de395d3..5e6795853dc31 100644 > > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > > @@ -32,8 +32,8 @@ > > > > > > > > #define PLATFORM(x) .platform = (x) > > > > #define GEN(x) \ > > > > - .graphics_ver = (x), \ > > > > - .media_ver = (x), \ > > > > + .graphics.ver = (x), \ > > > > + .media.ver = (x), \ > > > > .display.ver = (x) > > > > > > > > #define I845_PIPE_OFFSETS \ > > > > @@ -899,7 +899,7 @@ static const struct intel_device_info > > > > rkl_info = > > > > { > > > > static const struct intel_device_info dg1_info = { > > > > GEN12_FEATURES, > > > > DGFX_FEATURES, > > > > - .graphics_rel = 10, > > > > + .graphics.rel = 10, > > > > PLATFORM(INTEL_DG1), > > > > .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | > > > > BIT(PIPE_D), > > > > .require_force_probe = 1, > > > > @@ -986,8 +986,8 @@ static const struct intel_device_info > > > > adl_p_info > > > > = { > > > > I915_GTT_PAGE_SIZE_2M > > > > > > > > #define XE_HP_FEATURES \ > > > > - .graphics_ver = 12, \ > > > > - .graphics_rel = 50, \ > > > > + .graphics.ver = 12, \ > > > > + .graphics.rel = 50, \ > > > > XE_HP_PAGE_SIZES, \ > > > > .dma_mask_size = 46, \ > > > > .has_64bit_reloc = 1, \ > > > > @@ -1005,8 +1005,8 @@ static const struct intel_device_info > > > > adl_p_info = { > > > > .ppgtt_type = INTEL_PPGTT_FULL > > > > > > > > #define XE_HPM_FEATURES \ > > > > - .media_ver = 12, \ > > > > - .media_rel = 50 > > > > + .media.ver = 12, \ > > > > + .media.rel = 50 > > > > > > > > __maybe_unused > > > > static const struct intel_device_info xehpsdv_info = { > > > > @@ -1030,8 +1030,8 @@ static const struct intel_device_info > > > > dg2_info > > > > = { > > > > XE_HPM_FEATURES, > > > > XE_LPD_FEATURES, > > > > DGFX_FEATURES, > > > > - .graphics_rel = 55, > > > > - .media_rel = 55, > > > > + .graphics.rel = 55, > > > > + .media.rel = 55, > > > > PLATFORM(INTEL_DG2), > > > > .platform_engine_mask = > > > > BIT(RCS0) | BIT(BCS0) | > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > > > > b/drivers/gpu/drm/i915/intel_device_info.c > > > > index 305facedd2841..6e6b317bc33ce 100644 > > > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > > > @@ -97,17 +97,22 @@ static const char *iommu_name(void) > > > > void intel_device_info_print_static(const struct > > > > intel_device_info > > > > *info, > > > > struct drm_printer *p) > > > > { > > > > - if (info->graphics_rel) > > > > - drm_printf(p, "graphics version: %u.%02u\n", info- > > > > > graphics_ver, info->graphics_rel); > > > > + if (info->graphics.rel) > > > > + drm_printf(p, "graphics version: %u.%02u\n", info- > > > > > graphics.ver, > > > > + info->graphics.rel); > > > > else > > > > - drm_printf(p, "graphics version: %u\n", info- > > > > > graphics_ver); > > > > + drm_printf(p, "graphics version: %u\n", info- > > > > > graphics.ver); > > > > > > > > - if (info->media_rel) > > > > - drm_printf(p, "media version: %u.%02u\n", info- > > > > > media_ver, info->media_rel); > > > > + if (info->media.rel) > > > > + drm_printf(p, "media version: %u.%02u\n", info- > > > > > media.ver, info->media.rel); > > > > else > > > > - drm_printf(p, "media version: %u\n", info- > > > > > media_ver); > > > > + drm_printf(p, "media version: %u\n", info- > > > > > media.ver); > > > > + > > > > + if (info->display.rel) > > > > + drm_printf(p, "display version: %u.%02u\n", info- > > > > > display.ver, info->display.rel); > > > > + else > > > > + drm_printf(p, "display version: %u\n", info- > > > > > display.ver); > > > > > > > > - drm_printf(p, "display version: %u\n", info->display.ver); > > > > drm_printf(p, "gt: %d\n", info->gt); > > > > drm_printf(p, "iommu: %s\n", iommu_name()); > > > > drm_printf(p, "memory-regions: %x\n", info- > > > > > memory_regions); > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h > > > > b/drivers/gpu/drm/i915/intel_device_info.h > > > > index 8e6f48d1eb7bc..669f0d26c3c38 100644 > > > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > > > @@ -166,11 +166,14 @@ enum intel_ppgtt_type { > > > > func(overlay_needs_physical); \ > > > > func(supports_tv); > > > > > > > > +struct ip_version { > > > > + u8 ver; > > > > + u8 rel; > > > > +}; > > > > + > > > > struct intel_device_info { > > > > - u8 graphics_ver; > > > > - u8 graphics_rel; > > > > - u8 media_ver; > > > > - u8 media_rel; > > > > + struct ip_version graphics; > > > > + struct ip_version media; > > > > > > > > intel_engine_mask_t platform_engine_mask; /* Engines > > > > supported > > > > by the HW */ > > > > > > > > @@ -200,6 +203,7 @@ struct intel_device_info { > > > > > > > > struct { > > > > u8 ver; > > > > + u8 rel; > Even this is mentioned in the comment as "need for runtime variant", I > have no idea why it is needed because it is copied on i915_drv.c and > shown by drm_printf() in intel_device_info.c. It is to make standard access between every IP, right now we already graphics IP with ver and rel(DG2 graphics_rel = 55 while xehpsdv is graphics_rel=50) but not a display one. But that will change in future platforms, so making the access the same for every IP. > If it is really needed, > Reviewed-by: Caz Yokoyama <caz.yokoyama@intel.com> > -caz > > > > > > > > > #define DEFINE_FLAG(name) u8 name:1 > > > > DEV_INFO_DISPLAY_FOR_EACH_FLAG(DEFINE_FLAG); > > > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > > b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > > index 4f81801468881..9ab3f284d1dd9 100644 > > > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > > > > @@ -165,7 +165,7 @@ struct drm_i915_private > > > > *mock_gem_device(void) > > > > /* Using the global GTT may ask questions about KMS users, > > > > so > > > > prepare */ > > > > drm_mode_config_init(&i915->drm); > > > > > > > > - mkwrite_device_info(i915)->graphics_ver = -1; > > > > + mkwrite_device_info(i915)->graphics.ver = -1; > > > > > > > > mkwrite_device_info(i915)->page_sizes = > > > > I915_GTT_PAGE_SIZE_4K |
Reviewed-by: Caz Yokoyama <caz.yokoyama@intel.com> -caz On Thu, 2021-10-28 at 21:08 +0000, Souza, Jose wrote: > Reviewed-by: Caz Yokoyama <caz.yokoyama@intel.com>
On Mon, 2021-10-25 at 12:04 +0300, Jani Nikula wrote: > On Fri, 22 Oct 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote: > > > On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@intel.com> wrote: > > > > On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote: > > > > > On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote: > > > > > > The constant platform display version is not using this new struct but > > > > > > the runtime variant will definitely use it. > > > > > > > > > > Cc: Some more folks to hijack this thread. Sorry! ;) > > > > > > > > > > We added runtime info to i915, because we had this idea and goal of > > > > > turning the device info to a truly const pointer to the info structures > > > > > in i915_pci.c that are stored in rodata. The idea was that we'll have a > > > > > complete split of mutable and immutable device data, with all the > > > > > mutable data in runtime info. > > > > > > > > > > Alas, we never got there. More and more data that was mostly const but > > > > > sometimes needed tweaking kept piling up. mkwrite_device_info() was > > > > > supposed to be a clue not to modify device info runtime, but instead it > > > > > proliferated. Now we have places like intel_fbc_init() disabling FBC > > > > > through that. But most importantly, we have fusing that considerably > > > > > changes the device info, and the copying all of that data over to > > > > > runtime info probably isn't worth it. > > > > > > > > > > Should we just acknowledge that the runtime info is useless, and move > > > > > some of that data to intel_device_info and some of it elsewhere in i915? > > > > > > > > With newer platforms getting more and more modular, I believe we will > > > > need to store even more mutable platform information. > > > > > > > > In my opinion a separation of immutable and mutable platform > > > > information is cleaner and easier to maintain. > > > > > > Yeah, that's kind of what the original point was with device and runtime > > > info split. It's just that a lot of the supposedly immutable platform > > > info has turned into mutable information. > > > > > > I think either we need to properly follow through with that idea, and > > > only store a const struct intel_device_info * to the rodata in > > > i915_pci.c, or just scrap it. None of this "almost immutable" business > > > that we currently have. "Almost immutable" means "mutable". > > > > > > The main problem is that we'll still want to have the initial values in > > > static data. One idea is something like this: > > > > > > struct intel_device_info { > > > const struct intel_runtime_info *runtime_info; > > > /* ... */ > > > }; > > > > > > static const struct intel_device_info i965g_info = { > > > .runtime_info = &i965g_initial_runtime_info; > > > /* ... */ > > > }; > > > > > > And things like .pipe_mask would be part of struct > > > intel_runtime_info. You'd copy the stuff over from intel_device_info > > > runtime_info member to i915->__runtime, but i915->__info would be a > > > const pointer to the device info. You'd never access the runtime_info > > > member after of intel_device_info after probe. > > > > > > I like this approach. I think the only problem would be that if someone > > inadvertently do a i915->__info->runtime_info they will be accessing the > > wrong data. So maybe to be clear do > > > > struct intel_device_info { > > const void *initial_runtime_info; > > /* ... */ > > }; > > > > static const struct intel_device_info i965g_info = { > > .initial_runtime_info = &i965g_initial_runtime_info; > > /* ... */ > > }; > > > > this would make it opaque and even hint by the name so the developer is > > not tempted to add a cast. > > I think that's all fairly straightforward. Any ideas on how to do the > flags split cleanly, though? I already dislike the > DEV_INFO_FOR_EACH_FLAG() and DEV_INFO_DISPLAY_FOR_EACH_FLAG() split. Just to be sure, this discussion that Jani started can be build on top of this patch right? Will try to get someone to review the remaining patch of this series to merge it. > > BR, > Jani. > > > > > > > Lucas De Marchi > > > > > > > > It's just really painful, for instance because we already have two sets > > > of flags, display and non-display, and those would be multiplied to > > > mutable/immutable. And we should probably increase, not decrease, the > > > split between display and non-display. The macro horror show of > > > i915_pci.c would just grow worse. > > > > > > > > > BR, > > > Jani. > > > > > > > > > > > > > > > > > > > > > > > > > > > > BR, > > > > > Jani. > > > > > > > > > > > > > > > -- > > > Jani Nikula, Intel Open Source Graphics Center >
On Tue, Oct 19, 2021 at 05:23:51PM -0700, Jose Souza wrote: >Adding a structure to standardize access to IP versioning as future >platforms will have this information populated at runtime. > >The constant platform display version is not using this new struct but >the runtime variant will definitely use it. > >Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> >Cc: Matt Atwood <matthew.s.atwood@intel.com> >Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi
On Wed, 2021-10-20 at 05:10 +0000, Patchwork wrote: > Patch Details > Series: series starting with [1/3] drm/i915: Add struct to hold IP version > URL: https://patchwork.freedesktop.org/series/96038/ > State: failure > Details: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21385/index.html > CI Bug Log - changes from CI_DRM_10762_full -> Patchwork_21385_full > > Summary > > FAILURE > > Serious unknown changes coming with Patchwork_21385_full absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_21385_full, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > Possible new issues > > Here are the unknown changes that may have been introduced in Patchwork_21385_full: > > IGT changes > > Possible regressions > > igt@sysfs_heartbeat_interval@precise@vcs1: > shard-tglb: PASS -> INCOMPLETE Not related. Patches pushed to drm-intel-gt-next, thanks for the reviews Caz and Lucas. > Known issues > > Here are the changes found in Patchwork_21385_full that come from known issues: > > IGT changes > > Issues hit > > igt@gem_create@create-massive: > > shard-snb: NOTRUN -> DMESG-WARN ([i915#3002]) > igt@gem_ctx_isolation@preservation-s3@bcs0: > > shard-apl: PASS -> DMESG-WARN ([i915#180]) > igt@gem_ctx_persistence@legacy-engines-hostile-preempt: > > shard-snb: NOTRUN -> SKIP ([fdo#109271] / [i915#1099]) +3 similar issues > igt@gem_ctx_sseu@mmap-args: > > shard-tglb: NOTRUN -> SKIP ([i915#280]) +1 similar issue > igt@gem_exec_fair@basic-none-vip@rcs0: > > shard-tglb: NOTRUN -> FAIL ([i915#2842]) +5 similar issues > igt@gem_exec_fair@basic-pace@vcs1: > > shard-iclb: NOTRUN -> FAIL ([i915#2842]) > igt@gem_exec_whisper@basic-queues-forked: > > shard-glk: PASS -> DMESG-WARN ([i915#118]) +1 similar issue > igt@gem_huc_copy@huc-copy: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#2190]) > igt@gem_pxp@create-regular-context-1: > > shard-tglb: NOTRUN -> SKIP ([i915#4270]) > igt@gem_userptr_blits@input-checking: > > shard-apl: NOTRUN -> DMESG-WARN ([i915#3002]) +1 similar issue > igt@gem_userptr_blits@vma-merge: > > shard-snb: NOTRUN -> FAIL ([i915#2724]) > > shard-apl: NOTRUN -> FAIL ([i915#3318]) > > igt@gen7_exec_parse@chained-batch: > > shard-tglb: NOTRUN -> SKIP ([fdo#109289]) > igt@gen9_exec_parse@bb-start-out: > > shard-tglb: NOTRUN -> SKIP ([i915#2856]) > igt@i915_pm_rc6_residency@rc6-idle: > > shard-tglb: NOTRUN -> WARN ([i915#2681] / [i915#2684]) > igt@kms_async_flips@crc: > > shard-skl: NOTRUN -> FAIL ([i915#4272]) > igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [i915#3777]) +1 similar issue > igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-async-flip: > > shard-skl: NOTRUN -> FAIL ([i915#3743]) > igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-0-hflip: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#3777]) > igt@kms_big_fb@y-tiled-8bpp-rotate-270: > > shard-tglb: NOTRUN -> SKIP ([fdo#111614]) > igt@kms_big_fb@yf-tiled-addfb-size-overflow: > > shard-tglb: NOTRUN -> SKIP ([fdo#111615]) +2 similar issues > igt@kms_big_joiner@basic: > > shard-tglb: NOTRUN -> SKIP ([i915#2705]) > igt@kms_bw@linear-tiling-1-displays-3840x2160p: > > shard-tglb: NOTRUN -> FAIL ([i915#1385] / [i915#4299]) > igt@kms_bw@linear-tiling-2-displays-1920x1080p: > > shard-apl: NOTRUN -> DMESG-FAIL ([i915#4298]) +1 similar issue > igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_rc_ccs_cc: > > shard-iclb: NOTRUN -> SKIP ([fdo#109278] / [i915#3886]) > igt@kms_ccs@pipe-a-random-ccs-data-y_tiled_gen12_mc_ccs: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [i915#3886]) +3 similar issues > igt@kms_ccs@pipe-b-crc-sprite-planes-basic-y_tiled_gen12_mc_ccs: > > shard-tglb: NOTRUN -> SKIP ([i915#3689] / [i915#3886]) +2 similar issues > igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_mc_ccs: > > shard-skl: NOTRUN -> SKIP ([fdo#109271] / [i915#3886]) > igt@kms_ccs@pipe-c-bad-rotation-90-y_tiled_gen12_mc_ccs: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#3886]) +6 similar issues > igt@kms_ccs@pipe-c-crc-sprite-planes-basic-yf_tiled_ccs: > > shard-tglb: NOTRUN -> SKIP ([i915#3689]) +6 similar issues > igt@kms_ccs@pipe-d-bad-pixel-format-y_tiled_ccs: > > shard-snb: NOTRUN -> SKIP ([fdo#109271]) +232 similar issues > igt@kms_ccs@pipe-d-crc-primary-rotation-180-yf_tiled_ccs: > > shard-apl: NOTRUN -> SKIP ([fdo#109271]) +167 similar issues > igt@kms_chamelium@hdmi-hpd-enable-disable-mode: > > shard-snb: NOTRUN -> SKIP ([fdo#109271] / [fdo#111827]) +10 similar issues > igt@kms_chamelium@vga-hpd-fast: > > shard-skl: NOTRUN -> SKIP ([fdo#109271] / [fdo#111827]) +2 similar issues > igt@kms_chamelium@vga-hpd-for-each-pipe: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [fdo#111827]) +11 similar issues > igt@kms_color_chamelium@pipe-a-ctm-0-5: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [fdo#111827]) +16 similar issues > igt@kms_color_chamelium@pipe-a-ctm-0-75: > > shard-iclb: NOTRUN -> SKIP ([fdo#109284] / [fdo#111827]) > igt@kms_color_chamelium@pipe-d-ctm-red-to-blue: > > shard-tglb: NOTRUN -> SKIP ([fdo#109284] / [fdo#111827]) +8 similar issues > igt@kms_content_protection@dp-mst-type-1: > > shard-tglb: NOTRUN -> SKIP ([i915#3116]) > igt@kms_content_protection@mei_interface: > > shard-tglb: NOTRUN -> SKIP ([fdo#111828]) > igt@kms_cursor_crc@pipe-a-cursor-32x32-rapid-movement: > > shard-tglb: NOTRUN -> SKIP ([i915#3319]) +2 similar issues > igt@kms_cursor_crc@pipe-b-cursor-32x10-random: > > shard-tglb: NOTRUN -> SKIP ([i915#3359]) +2 similar issues > igt@kms_cursor_crc@pipe-b-cursor-32x32-onscreen: > > shard-skl: NOTRUN -> SKIP ([fdo#109271]) +16 similar issues > igt@kms_cursor_crc@pipe-c-cursor-32x10-rapid-movement: > > shard-iclb: NOTRUN -> SKIP ([fdo#109278]) > igt@kms_cursor_crc@pipe-d-cursor-512x512-offscreen: > > shard-tglb: NOTRUN -> SKIP ([fdo#109279] / [i915#3359]) +2 similar issues > igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions: > > shard-skl: NOTRUN -> FAIL ([i915#2346]) > igt@kms_cursor_legacy@pipe-d-torture-bo: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#533]) > igt@kms_cursor_legacy@short-busy-flip-before-cursor-toggle: > > shard-tglb: NOTRUN -> SKIP ([i915#4103]) > igt@kms_flip@flip-vs-expired-vblank@c-edp1: > > shard-skl: NOTRUN -> FAIL ([i915#79]) > igt@kms_flip@plain-flip-fb-recreate-interruptible@c-edp1: > > shard-skl: PASS -> FAIL ([i915#2122]) +1 similar issue > igt@kms_frontbuffer_tracking@fbc-suspend: > > shard-kbl: PASS -> DMESG-WARN ([i915#180]) +3 similar issues > > shard-tglb: PASS -> INCOMPLETE ([i915#2411] / [i915#456]) > > igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-shrfb-plflip-blt: > > shard-tglb: NOTRUN -> SKIP ([fdo#111825]) +21 similar issues > igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-mmap-cpu: > > shard-iclb: NOTRUN -> SKIP ([fdo#109280]) > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > > shard-apl: NOTRUN -> DMESG-WARN ([i915#180]) > igt@kms_plane_alpha_blend@pipe-a-alpha-7efc: > > shard-apl: NOTRUN -> FAIL ([fdo#108145] / [i915#265]) > igt@kms_plane_alpha_blend@pipe-b-alpha-basic: > > shard-kbl: NOTRUN -> FAIL ([fdo#108145] / [i915#265]) +3 similar issues > igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb: > > shard-apl: NOTRUN -> FAIL ([i915#265]) > igt@kms_plane_lowres@pipe-a-tiling-x: > > shard-tglb: NOTRUN -> SKIP ([i915#3536]) > igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-5: > > shard-skl: NOTRUN -> SKIP ([fdo#109271] / [i915#658]) > igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-5: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#658]) +2 similar issues > igt@kms_psr2_sf@plane-move-sf-dmg-area-2: > > shard-tglb: NOTRUN -> SKIP ([i915#2920]) > igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-5: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [i915#658]) +2 similar issues > igt@kms_psr2_su@page_flip: > > shard-tglb: NOTRUN -> SKIP ([i915#1911]) > igt@kms_psr@psr2_sprite_mmap_gtt: > > shard-iclb: PASS -> SKIP ([fdo#109441]) > igt@kms_psr@psr2_sprite_render: > > shard-tglb: NOTRUN -> FAIL ([i915#132] / [i915#3467]) +1 similar issue > igt@kms_universal_plane@disable-primary-vs-flip-pipe-d: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271]) +144 similar issues > igt@kms_writeback@writeback-fb-id: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [i915#2437]) +1 similar issue > igt@nouveau_crc@pipe-a-source-outp-complete: > > shard-tglb: NOTRUN -> SKIP ([i915#2530]) > igt@perf@polling-parameterized: > > shard-skl: PASS -> FAIL ([i915#1542]) > igt@prime_nv_api@nv_self_import: > > shard-tglb: NOTRUN -> SKIP ([fdo#109291]) +1 similar issue > igt@prime_nv_api@nv_self_import_to_different_fd: > > shard-iclb: NOTRUN -> SKIP ([fdo#109291]) +1 similar issue > igt@prime_vgem@fence-write-hang: > > shard-tglb: NOTRUN -> SKIP ([fdo#109295]) > igt@sysfs_clients@fair-7: > > shard-apl: NOTRUN -> SKIP ([fdo#109271] / [i915#2994]) +1 similar issue > igt@sysfs_clients@recycle: > > shard-kbl: NOTRUN -> SKIP ([fdo#109271] / [i915#2994]) +1 similar issue > igt@sysfs_clients@sema-50: > > shard-tglb: NOTRUN -> SKIP ([i915#2994]) > igt@sysfs_clients@split-10: > > shard-iclb: NOTRUN -> SKIP ([i915#2994]) > Possible fixes > > igt@gem_eio@unwedge-stress: > > shard-tglb: TIMEOUT ([i915#2369] / [i915#3063] / [i915#3648]) -> PASS > igt@gem_exec_fair@basic-none-share@rcs0: > > shard-tglb: FAIL ([i915#2842]) -> PASS +2 similar issues > igt@gem_exec_fair@basic-throttle@rcs0: > > shard-iclb: FAIL ([i915#2842]) -> PASS > igt@gem_exec_whisper@basic-queues-priority: > > shard-iclb: INCOMPLETE ([i915#1895]) -> PASS > igt@gem_spin_batch@resubmit-new-all@vecs0: > > shard-skl: DMESG-WARN ([i915#1982]) -> PASS +1 similar issue > igt@gem_workarounds@suspend-resume: > > shard-skl: INCOMPLETE ([i915#198]) -> PASS > igt@i915_selftest@live@hangcheck: > > shard-snb: INCOMPLETE ([i915#3921]) -> PASS > igt@i915_selftest@perf@region: > > shard-iclb: INCOMPLETE -> PASS > igt@kms_big_fb@x-tiled-32bpp-rotate-180: > > shard-glk: DMESG-WARN ([i915#118]) -> PASS > igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180: > > shard-snb: SKIP ([fdo#109271]) -> PASS +1 similar issue > igt@kms_cursor_crc@pipe-c-cursor-suspend: > > shard-tglb: INCOMPLETE ([i915#2411] / [i915#456]) -> PASS > igt@kms_cursor_crc@pipe-d-cursor-suspend: > > shard-tglb: INCOMPLETE ([i915#2411] / [i915#4211]) -> PASS > igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2: > > shard-glk: FAIL ([i915#2122]) -> PASS > igt@kms_flip@flip-vs-expired-vblank-interruptible@b-edp1: > > shard-skl: FAIL ([i915#79]) -> PASS > igt@kms_flip@flip-vs-suspend-interruptible@a-dp1: > > shard-kbl: DMESG-WARN ([i915#180]) -> PASS +6 similar issues > > shard-apl: DMESG-WARN ([i915#180]) -> PASS +2 similar issues > > igt@kms_flip@plain-flip-fb-recreate@c-edp1: > > shard-skl: FAIL ([i915#2122]) -> PASS > igt@kms_hdr@bpc-switch-dpms: > > shard-skl: FAIL ([i915#1188]) -> PASS +1 similar issue > igt@kms_plane_alpha_blend@pipe-a-coverage-7efc: > > shard-skl: FAIL ([fdo#108145] / [i915#265]) -> PASS +1 similar issue > igt@kms_psr@psr2_sprite_plane_move: > > shard-iclb: SKIP ([fdo#109441]) -> PASS +3 similar issues > igt@perf@polling-parameterized: > > shard-glk: FAIL ([i915#1542]) -> PASS > Warnings > > igt@gem_exec_fair@basic-pace@vcs1: > > shard-kbl: FAIL ([i915#2842]) -> SKIP ([fdo#109271]) > igt@kms_bw@linear-tiling-5-displays-2560x1440p: > > shard-tglb: FAIL ([i915#1385]) -> FAIL ([i915#1385] / [i915#4299]) +10 similar issues > igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-1: > > shard-iclb: [SKIP][133] ([i915#2920]) -> [SKIP][134] ([i
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 1e5b75ae99329..bdf85d202c55c 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -808,7 +808,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return PTR_ERR(i915); /* Disable nuclear pageflip by default on pre-ILK */ - if (!i915->params.nuclear_pageflip && match_info->graphics_ver < 5) + if (!i915->params.nuclear_pageflip && match_info->graphics.ver < 5) i915->drm.driver_features &= ~DRIVER_ATOMIC; /* diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 12256218634f4..26b6e2b8bb5e8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1327,15 +1327,15 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev) #define IP_VER(ver, rel) ((ver) << 8 | (rel)) -#define GRAPHICS_VER(i915) (INTEL_INFO(i915)->graphics_ver) -#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915)->graphics_ver, \ - INTEL_INFO(i915)->graphics_rel) +#define GRAPHICS_VER(i915) (INTEL_INFO(i915)->graphics.ver) +#define GRAPHICS_VER_FULL(i915) IP_VER(INTEL_INFO(i915)->graphics.ver, \ + INTEL_INFO(i915)->graphics.rel) #define IS_GRAPHICS_VER(i915, from, until) \ (GRAPHICS_VER(i915) >= (from) && GRAPHICS_VER(i915) <= (until)) -#define MEDIA_VER(i915) (INTEL_INFO(i915)->media_ver) -#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)->media_ver, \ - INTEL_INFO(i915)->media_rel) +#define MEDIA_VER(i915) (INTEL_INFO(i915)->media.ver) +#define MEDIA_VER_FULL(i915) IP_VER(INTEL_INFO(i915)->media.arch, \ + INTEL_INFO(i915)->media.rel) #define IS_MEDIA_VER(i915, from, until) \ (MEDIA_VER(i915) >= (from) && MEDIA_VER(i915) <= (until)) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 169837de395d3..5e6795853dc31 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -32,8 +32,8 @@ #define PLATFORM(x) .platform = (x) #define GEN(x) \ - .graphics_ver = (x), \ - .media_ver = (x), \ + .graphics.ver = (x), \ + .media.ver = (x), \ .display.ver = (x) #define I845_PIPE_OFFSETS \ @@ -899,7 +899,7 @@ static const struct intel_device_info rkl_info = { static const struct intel_device_info dg1_info = { GEN12_FEATURES, DGFX_FEATURES, - .graphics_rel = 10, + .graphics.rel = 10, PLATFORM(INTEL_DG1), .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D), .require_force_probe = 1, @@ -986,8 +986,8 @@ static const struct intel_device_info adl_p_info = { I915_GTT_PAGE_SIZE_2M #define XE_HP_FEATURES \ - .graphics_ver = 12, \ - .graphics_rel = 50, \ + .graphics.ver = 12, \ + .graphics.rel = 50, \ XE_HP_PAGE_SIZES, \ .dma_mask_size = 46, \ .has_64bit_reloc = 1, \ @@ -1005,8 +1005,8 @@ static const struct intel_device_info adl_p_info = { .ppgtt_type = INTEL_PPGTT_FULL #define XE_HPM_FEATURES \ - .media_ver = 12, \ - .media_rel = 50 + .media.ver = 12, \ + .media.rel = 50 __maybe_unused static const struct intel_device_info xehpsdv_info = { @@ -1030,8 +1030,8 @@ static const struct intel_device_info dg2_info = { XE_HPM_FEATURES, XE_LPD_FEATURES, DGFX_FEATURES, - .graphics_rel = 55, - .media_rel = 55, + .graphics.rel = 55, + .media.rel = 55, PLATFORM(INTEL_DG2), .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 305facedd2841..6e6b317bc33ce 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -97,17 +97,22 @@ static const char *iommu_name(void) void intel_device_info_print_static(const struct intel_device_info *info, struct drm_printer *p) { - if (info->graphics_rel) - drm_printf(p, "graphics version: %u.%02u\n", info->graphics_ver, info->graphics_rel); + if (info->graphics.rel) + drm_printf(p, "graphics version: %u.%02u\n", info->graphics.ver, + info->graphics.rel); else - drm_printf(p, "graphics version: %u\n", info->graphics_ver); + drm_printf(p, "graphics version: %u\n", info->graphics.ver); - if (info->media_rel) - drm_printf(p, "media version: %u.%02u\n", info->media_ver, info->media_rel); + if (info->media.rel) + drm_printf(p, "media version: %u.%02u\n", info->media.ver, info->media.rel); else - drm_printf(p, "media version: %u\n", info->media_ver); + drm_printf(p, "media version: %u\n", info->media.ver); + + if (info->display.rel) + drm_printf(p, "display version: %u.%02u\n", info->display.ver, info->display.rel); + else + drm_printf(p, "display version: %u\n", info->display.ver); - drm_printf(p, "display version: %u\n", info->display.ver); drm_printf(p, "gt: %d\n", info->gt); drm_printf(p, "iommu: %s\n", iommu_name()); drm_printf(p, "memory-regions: %x\n", info->memory_regions); diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 8e6f48d1eb7bc..669f0d26c3c38 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -166,11 +166,14 @@ enum intel_ppgtt_type { func(overlay_needs_physical); \ func(supports_tv); +struct ip_version { + u8 ver; + u8 rel; +}; + struct intel_device_info { - u8 graphics_ver; - u8 graphics_rel; - u8 media_ver; - u8 media_rel; + struct ip_version graphics; + struct ip_version media; intel_engine_mask_t platform_engine_mask; /* Engines supported by the HW */ @@ -200,6 +203,7 @@ struct intel_device_info { struct { u8 ver; + u8 rel; #define DEFINE_FLAG(name) u8 name:1 DEV_INFO_DISPLAY_FOR_EACH_FLAG(DEFINE_FLAG); diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index 4f81801468881..9ab3f284d1dd9 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -165,7 +165,7 @@ struct drm_i915_private *mock_gem_device(void) /* Using the global GTT may ask questions about KMS users, so prepare */ drm_mode_config_init(&i915->drm); - mkwrite_device_info(i915)->graphics_ver = -1; + mkwrite_device_info(i915)->graphics.ver = -1; mkwrite_device_info(i915)->page_sizes = I915_GTT_PAGE_SIZE_4K |
Adding a structure to standardize access to IP versioning as future platforms will have this information populated at runtime. The constant platform display version is not using this new struct but the runtime variant will definitely use it. Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> Cc: Matt Atwood <matthew.s.atwood@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 12 ++++++------ drivers/gpu/drm/i915/i915_pci.c | 18 +++++++++--------- drivers/gpu/drm/i915/intel_device_info.c | 19 ++++++++++++------- drivers/gpu/drm/i915/intel_device_info.h | 12 ++++++++---- .../gpu/drm/i915/selftests/mock_gem_device.c | 2 +- 6 files changed, 37 insertions(+), 28 deletions(-)