Re: [Intel-gfx] [PATCH v8 1/2] drm/i915: preparation for using PAT index

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Jani and Tvrtko,

> > > This patch is a preparation for replacing enum i915_cache_level with PAT
> > > index. Caching policy for buffer objects is set through the PAT index in
> > > PTE, the old i915_cache_level is not sufficient to represent all caching
> > > modes supported by the hardware.
> > > 
> > > Preparing the transition by adding some platform dependent data structures
> > > and helper functions to translate the cache_level to pat_index.
> > > 
> > > cachelevel_to_pat: a platform dependent array mapping cache_level to
> > >                     pat_index.
> > > 
> > > max_pat_index: the maximum PAT index recommended in hardware specification
> > >                 Needed for validating the PAT index passed in from user
> > >                 space.
> > > 
> > > i915_gem_get_pat_index: function to convert cache_level to PAT index.
> > > 
> > > obj_to_i915(obj): macro moved to header file for wider usage.
> > > 
> > > I915_MAX_CACHE_LEVEL: upper bound of i915_cache_level for the
> > >                        convenience of coding.
> > > 
> > > Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Fei Yang <fei.yang@intel.com>
> > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> > > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > 
> > [snip]
> > 
> > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > index f6a7c0bd2955..0eda8b4ee17f 100644
> > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > @@ -123,7 +123,9 @@ struct drm_i915_private *mock_gem_device(void)
> > >   	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);
> > > @@ -180,6 +182,13 @@ struct drm_i915_private *mock_gem_device(void)
> > >   		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);
> > 
> > This is not okay. It's not okay to modify device info at runtime. This
> > is why we've separated runtime info from device info. This is why we've
> > made device info const, and localized the modifications to a couple of
> > places.
> > 
> > If you need to modify it, it belongs in runtime info. Even if it's only
> > ever modified for mock devices.
> > 
> > We were at the brink of being able to finally convert INTEL_INFO() into
> > a pointer to const rodata [1], where you are unable to modify it, but
> > this having been merged as commit 5e352e32aec2 ("drm/i915: preparation
> > for using PAT index") sets us back. (With [1] this oopses trying to
> > modify read-only data.)
> > 
> > This has been posted to the public list 20+ times, and nobody noticed or
> > pointed this out?!

That's not cool, indeed.

> > Throwing away const should be a huge red flag to any developer or
> > reviewer. Hell, *any* cast should be.
> > 
> > I've got a patch ready moving cachelevel_to_pat and max_pat_index to
> > runtime info. It's not great, since we'd be doing it only for the mock
> > device. Better ideas? I'm not waiting long.
> > 
> > 
> > BR,
> > Jani.
> > 
> > 
> > [1] https://patchwork.freedesktop.org/patch/msgid/0badc36ce6dd6b030507bdfd8a42ab984fb38d12.1686236840.git.jani.nikula@intel.com
> > 
> > 
> > > +	i915_info->max_pat_index = 3;
> > > +	for (i = 0; i < I915_MAX_CACHE_LEVEL; i++)
> > > +		i915_info->cachelevel_to_pat[i] = i;
> > > +
> 
> I'd simply suggest having a local static const table for the mock device. It
> should be trivial once i915->__info becomes a pointer so in that series I
> guess.

Fei... do you have bandwidth to look into this or do you want me
to try Tvrtko's suggestion out?

Thank you Jani for reporting it and thank you Tvrtko for
proposing the fix.

Andi