[Intel-xe] [PATCH v2 4/8] drm/xe: Fix xe_mmio_rmw32 operation

[Lucas De Marchi]

On Thu, Apr 13, 2023 at 12:49:11PM +0200, Maarten Lankhorst wrote:
>Hey,
>
>I thought this was on purpose, and xe_mmio_rmw32 specified which parts 
>to keep,
>
>as opposed to i915 which set a clear mask.

it's only the params that are named differently. Xe follows the more
traditional names for a rmw (pretty much any other place mentionining
read-modify-write in the kernel and on the web I could find - also see
include/linux/regmap.h:regmap_hw_reg_update_bits).
i915 chose some clearer names IMO since there is no confusion that the
bits are cleared.

I think renaming to clr + set would be ok.

Loking at the original history of the branch: 9a903ac967fc ("drm/xe: GuC
submission squashed into 1 patch"): it doesn't seem to be intended to
have a different semantics as the call there was also buggy (and still
is).  We were supposed to clear that bit when enabling GuC communication
but we are doing exactly nothing with the way it is write now.


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

but I'd prefer to rename the args to clr and set like mentioned above.
And we will need a fixup commit for the display part as mentioned by
Maarten.

Lucas De Marchi

>
>If this was not, can you also fix display/xe_de.h to no longer invert?
>
>Otherwise, you break the entirety of display.
>
>~Maarten
>
>On 2023-04-13 00:52, Matt Roper wrote:
>>xe_mmio_rmw32 was failing to invert the passed in mask, resulting in a
>>register updated that wasn't the expected RMW operation.  Fortunately
>>the impact of this mistake was limited, since xe_mmio_rmw32() is only
>>used in two places to unmask certain GuC-related interrupts.
>>
>>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>---
>>  drivers/gpu/drm/xe/xe_mmio.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>>index 388a633b438b..a3b7f9f5db67 100644
>>--- a/drivers/gpu/drm/xe/xe_mmio.h
>>+++ b/drivers/gpu/drm/xe/xe_mmio.h
>>@@ -40,7 +40,7 @@ static inline u32 xe_mmio_rmw32(struct xe_gt *gt, u32 reg, u32 mask,
>>  	u32 old, reg_val;
>>  	old = xe_mmio_read32(gt, reg);
>>-	reg_val = (old & mask) | val;
>>+	reg_val = (old & ~mask) | val;
>>  	xe_mmio_write32(gt, reg, reg_val);
>>  	return old;