Re: [PATCH 02/12] drm/i915: Print memory region info during probe

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 13, 2023 at 06:26:13PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 05:05:21PM +0100, Andrzej Hajda wrote:
> > On 13.12.2023 01:42, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Dump the details about every memory region into dmesg at probe time.
> > > Avoids having to dig those out from random places when debugging stuff.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_memory_region.c | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> > > index b2708f8cac2a..52d998e5c21a 100644
> > > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > > @@ -372,6 +372,24 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
> > >   		i915->mm.regions[i] = mem;
> > >   	}
> > >   
> > > +	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
> > > +		struct intel_memory_region *mem = i915->mm.regions[i];
> > > +		u64 region_size, io_size;
> > > +
> > > +		if (!mem)
> > > +			continue;
> > > +
> > > +		region_size = resource_size(&mem->region) >> 20;
> > > +		io_size = resource_size(&mem->io) >> 20;
> > > +
> > > +		if (resource_size(&mem->io))
> > > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: %llu MiB %pR\n",
> > > +				mem->id, mem->name, region_size, &mem->region, io_size, &mem->io);
> > > +		else
> > > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: n/a\n",
> > > +				mem->id, mem->name, region_size, &mem->region);
> > 
> > Doesn't printk handle properly 0-length resources?
> 
> Not without extra help. Apparently there is IORESOURCE_UNSET
> that would just print the size, but you have to explicitly
> set that yourself, which I suppose we could do.

This is what we get with nothing:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: 0 MiB [mem 0x00000000-0xffffffffffffffff]

This is with IORESOURCE_UNSET:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: 0 MiB [mem size 0x00000000]

And this is the original:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: n/a

I kinda like the explicit 'n/a' to make it obvious there is noting
there.

Also using IORESOURCE_UNSET would be a bit more annoying as we'd have
to explicitly preserve it in places where we resize the resource
(stolen reseved handling and selftests). But I suppose it would be
just a matter of using the full DEFINE_RES_NAMED() instead of
DEFINE_RES_MEM(). Hmm, maybe I should do that anyway just to be
safe in case someone starts to use some extra flags on these...

> 
> > 
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > 
> > Regards
> > Andrzej
> > 
> > 
> > > +	}
> > > +
> > >   	return 0;
> > >   
> > >   out_cleanup:
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel