Re: [PATCH 1/5] drm/i915/display: Abstract pipe/trans/cursor offset calculation

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/15/2025 5:14 PM, Jani Nikula wrote:
> On Mon, 15 Dec 2025, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Introduce INTEL_DISPLAY_DEVICE_*_OFFSET() macros to compute absolute
>> MMIO offsets for pipe, transcoder, and cursor registers.
>>
>> Update _MMIO_PIPE2/_MMIO_TRANS2/_MMIO_CURSOR2 to use these macros
>> for cleaner abstraction and to prepare for external API usage (e.g. GVT).
>>
>> Also move DISPLAY_MMIO_BASE() to intel_display_device.h so it can be
>> abstracted in GVT, allowing register macros to resolve via
>> exported helpers rather than peeking into struct intel_display.
>>
>> Suggested-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_display_device.h | 17 +++++++++++++++++
>>   .../drm/i915/display/intel_display_reg_defs.h   | 15 ++++-----------
>>   2 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index 50b2e9ae2c18..05bba7a9899a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -260,6 +260,23 @@ struct intel_display_platforms {
>>   	 ((id) == ARLS_HOST_BRIDGE_PCI_ID3) || \
>>   	 ((id) == ARLS_HOST_BRIDGE_PCI_ID4))
>>   
>> +#define INTEL_DISPLAY_DEVICE_PIPE_OFFSET(display, pipe) \
>> +	(DISPLAY_INFO(display)->pipe_offsets[(pipe)] - \
>> +	 DISPLAY_INFO(display)->pipe_offsets[PIPE_A] + \
>> +	 DISPLAY_MMIO_BASE(display))
>> +
>> +#define INTEL_DISPLAY_DEVICE_TRANS_OFFSET(display, trans) \
>> +	(DISPLAY_INFO(display)->trans_offsets[(trans)] - \
>> +	 DISPLAY_INFO(display)->trans_offsets[TRANSCODER_A] + \
>> +	 DISPLAY_MMIO_BASE(display))
>> +
>> +#define INTEL_DISPLAY_DEVICE_CURSOR_OFFSET(display, pipe) \
>> +	(DISPLAY_INFO(display)->cursor_offsets[(pipe)] - \
>> +	 DISPLAY_INFO(display)->cursor_offsets[PIPE_A] + \
>> +	 DISPLAY_MMIO_BASE(display))
>> +
>> +#define DISPLAY_MMIO_BASE(dev_priv)	(DISPLAY_INFO(dev_priv)->mmio_offset)
> Please s/dev_priv/display/ while at it.

Ohh yeah. sorry missed it while blindly copying/pasting. Will change this.

>
>> +
>>   struct intel_display_runtime_info {
>>   	struct intel_display_ip_ver {
>>   		u16 ver;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_reg_defs.h b/drivers/gpu/drm/i915/display/intel_display_reg_defs.h
>> index b83ad06f2ea7..74f572d3a202 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_reg_defs.h
>> @@ -8,8 +8,6 @@
>>   
>>   #include "i915_reg_defs.h"
>>   
>> -#define DISPLAY_MMIO_BASE(dev_priv)	(DISPLAY_INFO(dev_priv)->mmio_offset)
>> -
>>   #define VLV_DISPLAY_BASE		0x180000
>>   
>>   /*
>> @@ -36,14 +34,9 @@
>>    * Device info offset array based helpers for groups of registers with unevenly
>>    * spaced base offsets.
>>    */
>> -#define _MMIO_PIPE2(display, pipe, reg)		_MMIO(DISPLAY_INFO(display)->pipe_offsets[(pipe)] - \
>> -						      DISPLAY_INFO(display)->pipe_offsets[PIPE_A] + \
>> -						      DISPLAY_MMIO_BASE(display) + (reg))
>> -#define _MMIO_TRANS2(display, tran, reg)	_MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
>> -						      DISPLAY_INFO(display)->trans_offsets[TRANSCODER_A] + \
>> -						      DISPLAY_MMIO_BASE(display) + (reg))
>> -#define _MMIO_CURSOR2(display, pipe, reg)	_MMIO(DISPLAY_INFO(display)->cursor_offsets[(pipe)] - \
>> -						      DISPLAY_INFO(display)->cursor_offsets[PIPE_A] + \
>> -						      DISPLAY_MMIO_BASE(display) + (reg))
>> +
>> +#define _MMIO_PIPE2(display, pipe, reg)		_MMIO(INTEL_DISPLAY_DEVICE_PIPE_OFFSET(display, pipe) + (reg))
>> +#define _MMIO_TRANS2(display, trans, reg)	_MMIO(INTEL_DISPLAY_DEVICE_TRANS_OFFSET(display, trans) + (reg))
>> +#define _MMIO_CURSOR2(display, pipe, reg)	_MMIO(INTEL_DISPLAY_DEVICE_CURSOR_OFFSET(display, pipe) + (reg))
> Please wrap the macro argument usage in parenthesis, even if not
> strictly needed here. IMO it's just good code hygiene, and sets the
> example.

Sure.


Thanks & Regards,

Ankit

> With these fixed,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
> BR,
> Jani.
>
>>   
>>   #endif /* __INTEL_DISPLAY_REG_DEFS_H__ */