Re: [Intel-xe] [PATCH 11/18] fixup! drm/xe: Introduce a new DRM driver for Intel GPUs

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 08 May 2023, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Mon, May 08, 2023 at 05:46:11PM +0300, Jani Nikula wrote:
>> Add the read8 accessor to support intel_de_read8.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_mmio.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>> index 1a32e0f52261..b72a0a75259f 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.h
>> +++ b/drivers/gpu/drm/xe/xe_mmio.h
>> @@ -17,6 +17,14 @@ struct xe_device;
>>  
>>  int xe_mmio_init(struct xe_device *xe);
>>  
>> +static inline u8 xe_mmio_read8(struct xe_gt *gt, u32 reg)
>> +{
>> +	if (reg < gt->mmio.adj_limit)
>> +		reg += gt->mmio.adj_offset;
>
> This could probably be a warn_on; as far as I know, there are no 8-bit
> registers in the GT, so if we wind up trying to read sgunit or display
> registers through a media GT's pointer it's a clue that something has
> probably gone wrong.

Looks like Rodrigo already pushed this, so this would need to be added
separately.

> Of course this MMIO stuff is a confusing mess due to the fact that xe_gt
> is being used all over the driver for things that are completely
> unrelated to any GT.

This. The abstractions don't match reality.


BR,
Jani.


> After cleaning up the GT vs tile mess, I plan to
> work on coming up with something more sane for MMIO windows too.  I'm
> thinking something like a "struct xe_mmio_view" that will replace xe_gt
> as the first parameter to the xe_mmio_* functions and can provide them
> with additional information to also do range validation on debug builds
> to ensure you're accessing the subset of registers you think you are,
> and functions to hand you an appropriate xe_mmio_view:
>  - xe_mmio_for_display(xe): warns if read/write outside display range
>  - xe_mmio_for_gt(gt): warns if read/write outside a GT range; if media
>    GT is the parameter, also warns if outside the GSI range or one of
>    the media engines
>  - xe_mmio_for_tile(tile):  unrestricted MMIO access for a tile, mostly
>    intended for use with sgunit, soc, etc. registers that are outside
>    the GT
>
>
> Matt
>
>> +
>> +	return readb(gt->mmio.regs + reg);
>> +}
>> +
>>  static inline void xe_mmio_write32(struct xe_gt *gt,
>>  				   u32 reg, u32 val)
>>  {
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center