From: Jani Nikula <jani.nikula@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>, intel-gfx@lists.freedesktop.org
Cc: ravi.kumar.vodapalli@intel.com, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 12/14] drm/i915: Define multicast registers as a new type
Date: Tue, 04 Oct 2022 16:00:57 +0300 [thread overview]
Message-ID: <87a66bu4ja.fsf@intel.com> (raw)
In-Reply-To: <87czb7u4qc.fsf@intel.com>
On Tue, 04 Oct 2022, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 30 Sep 2022, Matt Roper <matthew.d.roper@intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
>> index 8f486f77609f..e823869b9afd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>> @@ -104,22 +104,16 @@ typedef struct {
>>
>> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>
>> -#define INVALID_MMIO_REG _MMIO(0)
>> -
>> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
>> -{
>> - return reg.reg;
>> -}
>> +typedef struct {
>> + u32 reg;
>> +} i915_mcr_reg_t;
>>
>> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
>> -{
>> - return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
>> -}
>> +#define INVALID_MMIO_REG _MMIO(0)
>>
>> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>> -{
>> - return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>> -}
>> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
>> +#define i915_mmio_reg_offset(r) (r.reg)
>> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b))
>> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
>>
>
> I don't really like losing the type safety here. The whole and only
> purpose of typedeffing i915_reg_t as a struct instead of just u32 was
> the strict type safety.
PS. Changes like this should really be highlighted better, in the commit
subject and title. Now it feels like it's hidden within a big commit
within a big series, without even mentioning it in the commit message.
BR,
Jani.
>
> BR,
> Jani.
--
Jani Nikula, Intel Open Source Graphics Center
WARNING: multiple messages have this Message-ID (diff)
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>, intel-gfx@lists.freedesktop.org
Cc: ravi.kumar.vodapalli@intel.com,
balasubramani.vivekanandan@intel.com,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 12/14] drm/i915: Define multicast registers as a new type
Date: Tue, 04 Oct 2022 16:00:57 +0300 [thread overview]
Message-ID: <87a66bu4ja.fsf@intel.com> (raw)
In-Reply-To: <87czb7u4qc.fsf@intel.com>
On Tue, 04 Oct 2022, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 30 Sep 2022, Matt Roper <matthew.d.roper@intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
>> index 8f486f77609f..e823869b9afd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>> @@ -104,22 +104,16 @@ typedef struct {
>>
>> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>
>> -#define INVALID_MMIO_REG _MMIO(0)
>> -
>> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
>> -{
>> - return reg.reg;
>> -}
>> +typedef struct {
>> + u32 reg;
>> +} i915_mcr_reg_t;
>>
>> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
>> -{
>> - return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
>> -}
>> +#define INVALID_MMIO_REG _MMIO(0)
>>
>> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>> -{
>> - return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>> -}
>> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
>> +#define i915_mmio_reg_offset(r) (r.reg)
>> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b))
>> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
>>
>
> I don't really like losing the type safety here. The whole and only
> purpose of typedeffing i915_reg_t as a struct instead of just u32 was
> the strict type safety.
PS. Changes like this should really be highlighted better, in the commit
subject and title. Now it feels like it's hidden within a big commit
within a big series, without even mentioning it in the commit message.
BR,
Jani.
>
> BR,
> Jani.
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2022-10-04 13:01 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-01 0:45 [Intel-gfx] [PATCH v2 00/14] Explicit MCR handling and MTL steering Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 01/14] drm/i915/gen8: Create separate reg definitions for new MCR registers Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 02/14] drm/i915/xehp: " Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 03/14] drm/i915/gt: Drop a few unused register definitions Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 04/14] drm/i915/gt: Correct prefix on a few registers Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 05/14] drm/i915/gt: Add intel_gt_mcr_multicast_rmw() operation Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 06/14] drm/i915/xehp: Check for faults on primary GAM Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 07/14] drm/i915/gt: Add intel_gt_mcr_wait_for_reg_fw() Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 08/14] drm/i915: Define MCR registers explicitly Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 09/14] drm/i915/gt: Always use MCR functions on multicast registers Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-12 16:18 ` [Intel-gfx] " Balasubramani Vivekanandan
2022-10-12 16:18 ` Balasubramani Vivekanandan
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 10/14] drm/i915/guc: Handle save/restore of MCR registers explicitly Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 11/14] drm/i915/gt: Add MCR-specific workaround initializers Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 12/14] drm/i915: Define multicast registers as a new type Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-04 12:56 ` [Intel-gfx] " Jani Nikula
2022-10-04 12:56 ` Jani Nikula
2022-10-04 13:00 ` Jani Nikula [this message]
2022-10-04 13:00 ` Jani Nikula
2022-10-05 1:01 ` [Intel-gfx] " Matt Roper
2022-10-05 1:01 ` Matt Roper
2022-10-13 7:21 ` [Intel-gfx] " Balasubramani Vivekanandan
2022-10-13 7:21 ` Balasubramani Vivekanandan
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 13/14] drm/i915/mtl: Add multicast steering for render GT Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-14 16:02 ` [Intel-gfx] " Balasubramani Vivekanandan
2022-10-14 16:02 ` Balasubramani Vivekanandan
2022-10-14 22:46 ` [Intel-gfx] " Matt Roper
2022-10-14 22:46 ` Matt Roper
2022-10-01 0:45 ` [Intel-gfx] [PATCH v2 14/14] drm/i915/mtl: Add multicast steering for media GT Matt Roper
2022-10-01 0:45 ` Matt Roper
2022-10-03 8:56 ` [Intel-gfx] " Tvrtko Ursulin
2022-10-03 19:32 ` Matt Roper
2022-10-04 10:13 ` Tvrtko Ursulin
2022-10-01 1:49 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Explicit MCR handling and MTL steering (rev2) Patchwork
2022-10-01 1:49 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-10-01 2:13 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2022-10-01 5:35 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Explicit MCR handling and MTL steering (rev3) Patchwork
2022-10-01 5:35 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-10-01 5:58 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-10-01 21:41 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a66bu4ja.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@intel.com \
--cc=ravi.kumar.vodapalli@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.