| Message ID | b0db62045a96a3fd4cf123685da88cc777f9b485.1687878757.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: further device info fixes and cleanups | expand |
On 27/06/2023 16:13, Jani Nikula wrote: > Instead of modifying the device info on the fly, use static const mock > device info. > > It's not okay to modify device info at runtime; we've added separate > runtime info for info that needs to be modified at runtime. We've added > safeguards to device info to prevent it from being modified, but commit > 5e352e32aec2 ("drm/i915: preparation for using PAT index") just cast the > const away and modified it anyway. This prevents device info from being > moved to rodata. > > Fixes: 5e352e32aec2 ("drm/i915: preparation for using PAT index") > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > .../gpu/drm/i915/selftests/mock_gem_device.c | 45 ++++++++++--------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index 09d4bbcdcdbf..4de6a4e8280d 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -118,15 +118,31 @@ static void mock_gt_probe(struct drm_i915_private *i915) > i915->gt[0]->name = "Mock GT"; > } > > +static const struct intel_device_info mock_info = { > + .__runtime.graphics.ip.ver = -1, > + .__runtime.page_sizes = (I915_GTT_PAGE_SIZE_4K | > + I915_GTT_PAGE_SIZE_64K | > + I915_GTT_PAGE_SIZE_2M), > + .__runtime.memory_regions = REGION_SMEM, > + .__runtime.platform_engine_mask = BIT(0), > + > + /* simply use legacy cache level for mock device */ > + .max_pat_index = 3, > + .cachelevel_to_pat = { > + [I915_CACHE_NONE] = 0, > + [I915_CACHE_LLC] = 1, > + [I915_CACHE_L3_LLC] = 2, > + [I915_CACHE_WT] = 3, > + }, > +}; > + > struct drm_i915_private *mock_gem_device(void) > { > #if IS_ENABLED(CONFIG_IOMMU_API) && defined(CONFIG_INTEL_IOMMU) > static struct dev_iommu fake_iommu = { .priv = (void *)-1 }; > #endif > struct drm_i915_private *i915; > - struct intel_device_info *i915_info; > struct pci_dev *pdev; > - unsigned int i; > int ret; > > pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); > @@ -159,15 +175,18 @@ struct drm_i915_private *mock_gem_device(void) > > pci_set_drvdata(pdev, i915); > > + /* Device parameters start as a copy of module parameters. */ > + i915_params_copy(&i915->params, &i915_modparams); > + > + /* Set up device info and initial runtime info. */ > + intel_device_info_driver_create(i915, pdev->device, &mock_info); This one was new to me but figuring out what will dev->device contain, or if it could have any unintended consequences was too time consuming. If something breaks it will break loudly in testing. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > + > dev_pm_domain_set(&pdev->dev, &pm_domain); > pm_runtime_enable(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > if (pm_runtime_enabled(&pdev->dev)) > WARN_ON(pm_runtime_get_sync(&pdev->dev)); > > - > - i915_params_copy(&i915->params, &i915_modparams); > - > intel_runtime_pm_init_early(&i915->runtime_pm); > /* wakeref tracking has significant overhead */ > i915->runtime_pm.no_wakeref_tracking = true; > @@ -175,21 +194,6 @@ 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); > > - RUNTIME_INFO(i915)->graphics.ip.ver = -1; > - > - RUNTIME_INFO(i915)->page_sizes = > - I915_GTT_PAGE_SIZE_4K | > - I915_GTT_PAGE_SIZE_64K | > - I915_GTT_PAGE_SIZE_2M; > - > - RUNTIME_INFO(i915)->memory_regions = REGION_SMEM; > - > - /* simply use legacy cache level for mock device */ > - i915_info = (struct intel_device_info *)INTEL_INFO(i915); > - i915_info->max_pat_index = 3; > - for (i = 0; i < I915_MAX_CACHE_LEVEL; i++) > - i915_info->cachelevel_to_pat[i] = i; > - > intel_memory_regions_hw_probe(i915); > > spin_lock_init(&i915->gpu_error.lock); > @@ -223,7 +227,6 @@ struct drm_i915_private *mock_gem_device(void) > mock_init_ggtt(to_gt(i915)); > to_gt(i915)->vm = i915_vm_get(&to_gt(i915)->ggtt->vm); > > - RUNTIME_INFO(i915)->platform_engine_mask = BIT(0); > to_gt(i915)->info.engine_mask = BIT(0); > > to_gt(i915)->engine[RCS0] = mock_engine(i915, "mock", RCS0);
Hi Jani, On Tue, Jun 27, 2023 at 06:13:58PM +0300, Jani Nikula wrote: > Instead of modifying the device info on the fly, use static const mock > device info. > > It's not okay to modify device info at runtime; we've added separate > runtime info for info that needs to be modified at runtime. We've added > safeguards to device info to prevent it from being modified, but commit > 5e352e32aec2 ("drm/i915: preparation for using PAT index") just cast the > const away and modified it anyway. This prevents device info from being > moved to rodata. > > Fixes: 5e352e32aec2 ("drm/i915: preparation for using PAT index") > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> thanks for fixing this! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi
On 27.06.2023 17:13, Jani Nikula wrote: > Instead of modifying the device info on the fly, use static const mock > device info. > > It's not okay to modify device info at runtime; we've added separate > runtime info for info that needs to be modified at runtime. We've added > safeguards to device info to prevent it from being modified, but commit > 5e352e32aec2 ("drm/i915: preparation for using PAT index") just cast the > const away and modified it anyway. This prevents device info from being > moved to rodata. > > Fixes: 5e352e32aec2 ("drm/i915: preparation for using PAT index") > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > --- > .../gpu/drm/i915/selftests/mock_gem_device.c | 45 ++++++++++--------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index 09d4bbcdcdbf..4de6a4e8280d 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -118,15 +118,31 @@ static void mock_gt_probe(struct drm_i915_private *i915) > i915->gt[0]->name = "Mock GT"; > } > > +static const struct intel_device_info mock_info = { > + .__runtime.graphics.ip.ver = -1, > + .__runtime.page_sizes = (I915_GTT_PAGE_SIZE_4K | > + I915_GTT_PAGE_SIZE_64K | > + I915_GTT_PAGE_SIZE_2M), > + .__runtime.memory_regions = REGION_SMEM, > + .__runtime.platform_engine_mask = BIT(0), > + > + /* simply use legacy cache level for mock device */ > + .max_pat_index = 3, > + .cachelevel_to_pat = { > + [I915_CACHE_NONE] = 0, > + [I915_CACHE_LLC] = 1, > + [I915_CACHE_L3_LLC] = 2, > + [I915_CACHE_WT] = 3, > + }, > +}; > + > struct drm_i915_private *mock_gem_device(void) > { > #if IS_ENABLED(CONFIG_IOMMU_API) && defined(CONFIG_INTEL_IOMMU) > static struct dev_iommu fake_iommu = { .priv = (void *)-1 }; > #endif > struct drm_i915_private *i915; > - struct intel_device_info *i915_info; > struct pci_dev *pdev; > - unsigned int i; > int ret; > > pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); > @@ -159,15 +175,18 @@ struct drm_i915_private *mock_gem_device(void) > > pci_set_drvdata(pdev, i915); > > + /* Device parameters start as a copy of module parameters. */ > + i915_params_copy(&i915->params, &i915_modparams); > + > + /* Set up device info and initial runtime info. */ > + intel_device_info_driver_create(i915, pdev->device, &mock_info); > + > dev_pm_domain_set(&pdev->dev, &pm_domain); > pm_runtime_enable(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > if (pm_runtime_enabled(&pdev->dev)) > WARN_ON(pm_runtime_get_sync(&pdev->dev)); > > - > - i915_params_copy(&i915->params, &i915_modparams); > - > intel_runtime_pm_init_early(&i915->runtime_pm); > /* wakeref tracking has significant overhead */ > i915->runtime_pm.no_wakeref_tracking = true; > @@ -175,21 +194,6 @@ 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); > > - RUNTIME_INFO(i915)->graphics.ip.ver = -1; > - > - RUNTIME_INFO(i915)->page_sizes = > - I915_GTT_PAGE_SIZE_4K | > - I915_GTT_PAGE_SIZE_64K | > - I915_GTT_PAGE_SIZE_2M; > - > - RUNTIME_INFO(i915)->memory_regions = REGION_SMEM; > - > - /* simply use legacy cache level for mock device */ > - i915_info = (struct intel_device_info *)INTEL_INFO(i915); > - i915_info->max_pat_index = 3; > - for (i = 0; i < I915_MAX_CACHE_LEVEL; i++) > - i915_info->cachelevel_to_pat[i] = i; > - > intel_memory_regions_hw_probe(i915); > > spin_lock_init(&i915->gpu_error.lock); > @@ -223,7 +227,6 @@ struct drm_i915_private *mock_gem_device(void) > mock_init_ggtt(to_gt(i915)); > to_gt(i915)->vm = i915_vm_get(&to_gt(i915)->ggtt->vm); > > - RUNTIME_INFO(i915)->platform_engine_mask = BIT(0); > to_gt(i915)->info.engine_mask = BIT(0); > > to_gt(i915)->engine[RCS0] = mock_engine(i915, "mock", RCS0);
> Instead of modifying the device info on the fly, use static const > mock device info. > > It's not okay to modify device info at runtime; we've added separate > runtime info for info that needs to be modified at runtime. We've > added safeguards to device info to prevent it from being modified, > but commit 5e352e32aec2 ("drm/i915: preparation for using PAT index") > just cast the const away and modified it anyway. This prevents device > info from being moved to rodata. > > Fixes: 5e352e32aec2 ("drm/i915: preparation for using PAT index") > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Thanks for fixing this. Reviewed-by: Fei Yang <fei.yang@intel.com> > --- > .../gpu/drm/i915/selftests/mock_gem_device.c | 45 ++++++++++--------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index 09d4bbcdcdbf..4de6a4e8280d 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -118,15 +118,31 @@ static void mock_gt_probe(struct drm_i915_private *i915) > i915->gt[0]->name = "Mock GT"; > } > > +static const struct intel_device_info mock_info = { > + .__runtime.graphics.ip.ver = -1, > + .__runtime.page_sizes = (I915_GTT_PAGE_SIZE_4K | > + I915_GTT_PAGE_SIZE_64K | > + I915_GTT_PAGE_SIZE_2M), > + .__runtime.memory_regions = REGION_SMEM, > + .__runtime.platform_engine_mask = BIT(0), > + > + /* simply use legacy cache level for mock device */ > + .max_pat_index = 3, > + .cachelevel_to_pat = { > + [I915_CACHE_NONE] = 0, > + [I915_CACHE_LLC] = 1, > + [I915_CACHE_L3_LLC] = 2, > + [I915_CACHE_WT] = 3, > + }, > +}; > + > struct drm_i915_private *mock_gem_device(void) { #if IS_ENABLED(CONFIG_IOMMU_API) && defined(CONFIG_INTEL_IOMMU) > static struct dev_iommu fake_iommu = { .priv = (void *)-1 }; #endif > struct drm_i915_private *i915; > - struct intel_device_info *i915_info; > struct pci_dev *pdev; > - unsigned int i; > int ret; > > pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); @@ -159,15 +175,18 @@ struct drm_i915_private *mock_gem_device(void) > > pci_set_drvdata(pdev, i915); > > + /* Device parameters start as a copy of module parameters. */ > + i915_params_copy(&i915->params, &i915_modparams); > + > + /* Set up device info and initial runtime info. */ > + intel_device_info_driver_create(i915, pdev->device, &mock_info); > + > dev_pm_domain_set(&pdev->dev, &pm_domain); > pm_runtime_enable(&pdev->dev); > pm_runtime_dont_use_autosuspend(&pdev->dev); > if (pm_runtime_enabled(&pdev->dev)) > WARN_ON(pm_runtime_get_sync(&pdev->dev)); > > - > - i915_params_copy(&i915->params, &i915_modparams); > - > intel_runtime_pm_init_early(&i915->runtime_pm); > /* wakeref tracking has significant overhead */ > i915->runtime_pm.no_wakeref_tracking = true; > @@ -175,21 +194,6 @@ 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); > > - RUNTIME_INFO(i915)->graphics.ip.ver = -1; > - > - RUNTIME_INFO(i915)->page_sizes = > - I915_GTT_PAGE_SIZE_4K | > - I915_GTT_PAGE_SIZE_64K | > - I915_GTT_PAGE_SIZE_2M; > - > - RUNTIME_INFO(i915)->memory_regions = REGION_SMEM; > - > - /* simply use legacy cache level for mock device */ > - i915_info = (struct intel_device_info *)INTEL_INFO(i915); > - i915_info->max_pat_index = 3; > - for (i = 0; i < I915_MAX_CACHE_LEVEL; i++) > - i915_info->cachelevel_to_pat[i] = i; > - > intel_memory_regions_hw_probe(i915); > > spin_lock_init(&i915->gpu_error.lock); > @@ -223,7 +227,6 @@ struct drm_i915_private *mock_gem_device(void) > mock_init_ggtt(to_gt(i915)); > to_gt(i915)->vm = i915_vm_get(&to_gt(i915)->ggtt->vm); > > - RUNTIME_INFO(i915)->platform_engine_mask = BIT(0); > to_gt(i915)->info.engine_mask = BIT(0); > > to_gt(i915)->engine[RCS0] = mock_engine(i915, "mock", RCS0); > -- > 2.39.2
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index 09d4bbcdcdbf..4de6a4e8280d 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -118,15 +118,31 @@ static void mock_gt_probe(struct drm_i915_private *i915) i915->gt[0]->name = "Mock GT"; } +static const struct intel_device_info mock_info = { + .__runtime.graphics.ip.ver = -1, + .__runtime.page_sizes = (I915_GTT_PAGE_SIZE_4K | + I915_GTT_PAGE_SIZE_64K | + I915_GTT_PAGE_SIZE_2M), + .__runtime.memory_regions = REGION_SMEM, + .__runtime.platform_engine_mask = BIT(0), + + /* simply use legacy cache level for mock device */ + .max_pat_index = 3, + .cachelevel_to_pat = { + [I915_CACHE_NONE] = 0, + [I915_CACHE_LLC] = 1, + [I915_CACHE_L3_LLC] = 2, + [I915_CACHE_WT] = 3, + }, +}; + struct drm_i915_private *mock_gem_device(void) { #if IS_ENABLED(CONFIG_IOMMU_API) && defined(CONFIG_INTEL_IOMMU) static struct dev_iommu fake_iommu = { .priv = (void *)-1 }; #endif struct drm_i915_private *i915; - struct intel_device_info *i915_info; struct pci_dev *pdev; - unsigned int i; int ret; pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); @@ -159,15 +175,18 @@ struct drm_i915_private *mock_gem_device(void) pci_set_drvdata(pdev, i915); + /* Device parameters start as a copy of module parameters. */ + i915_params_copy(&i915->params, &i915_modparams); + + /* Set up device info and initial runtime info. */ + intel_device_info_driver_create(i915, pdev->device, &mock_info); + dev_pm_domain_set(&pdev->dev, &pm_domain); pm_runtime_enable(&pdev->dev); pm_runtime_dont_use_autosuspend(&pdev->dev); if (pm_runtime_enabled(&pdev->dev)) WARN_ON(pm_runtime_get_sync(&pdev->dev)); - - i915_params_copy(&i915->params, &i915_modparams); - intel_runtime_pm_init_early(&i915->runtime_pm); /* wakeref tracking has significant overhead */ i915->runtime_pm.no_wakeref_tracking = true; @@ -175,21 +194,6 @@ 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); - RUNTIME_INFO(i915)->graphics.ip.ver = -1; - - RUNTIME_INFO(i915)->page_sizes = - I915_GTT_PAGE_SIZE_4K | - I915_GTT_PAGE_SIZE_64K | - I915_GTT_PAGE_SIZE_2M; - - RUNTIME_INFO(i915)->memory_regions = REGION_SMEM; - - /* simply use legacy cache level for mock device */ - i915_info = (struct intel_device_info *)INTEL_INFO(i915); - i915_info->max_pat_index = 3; - for (i = 0; i < I915_MAX_CACHE_LEVEL; i++) - i915_info->cachelevel_to_pat[i] = i; - intel_memory_regions_hw_probe(i915); spin_lock_init(&i915->gpu_error.lock); @@ -223,7 +227,6 @@ struct drm_i915_private *mock_gem_device(void) mock_init_ggtt(to_gt(i915)); to_gt(i915)->vm = i915_vm_get(&to_gt(i915)->ggtt->vm); - RUNTIME_INFO(i915)->platform_engine_mask = BIT(0); to_gt(i915)->info.engine_mask = BIT(0); to_gt(i915)->engine[RCS0] = mock_engine(i915, "mock", RCS0);
Instead of modifying the device info on the fly, use static const mock device info. It's not okay to modify device info at runtime; we've added separate runtime info for info that needs to be modified at runtime. We've added safeguards to device info to prevent it from being modified, but commit 5e352e32aec2 ("drm/i915: preparation for using PAT index") just cast the const away and modified it anyway. This prevents device info from being moved to rodata. Fixes: 5e352e32aec2 ("drm/i915: preparation for using PAT index") Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Fei Yang <fei.yang@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- .../gpu/drm/i915/selftests/mock_gem_device.c | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-)