On 10/10/2025 16:08, Boris Brezillon wrote:
> On Fri, 10 Oct 2025 15:22:50 +0100
> Steven Price <[email protected]> wrote:
>
>> On 10/10/2025 11:11, Boris Brezillon wrote:
>>> If we want to be able to skip CPU cache maintenance operations on
>>> CPU-cached mappings, the UMD needs to know the kind of coherency
>>> in place. Add a field to drm_panthor_gpu_info to do that. We can re-use
>>> a padding field for that since this object is write-only from the
>>> KMD perspective, and the UMD should just ignore it.
>>>
>>> v2:
>>> - New commit
>>>
>>> Signed-off-by: Boris Brezillon <[email protected]>
>>> ---
>>> drivers/gpu/drm/panthor/panthor_device.c | 6 +++-
>>> drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
>>> include/uapi/drm/panthor_drm.h | 39 ++++++++++++++++++++++--
>>> 3 files changed, 42 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c
>>> b/drivers/gpu/drm/panthor/panthor_device.c
>>> index c7033d82cef5..afe24a03a71c 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_device.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_device.c
>>> @@ -25,6 +25,8 @@
>>>
>>> static int panthor_gpu_coherency_init(struct panthor_device *ptdev)
>>> {
>>> + /* Start with no coherency, and update it if the device is flagged
>>> coherent. */
>>> + ptdev->gpu_info.selected_coherency = GPU_COHERENCY_NONE;
>>> ptdev->coherent = device_get_dma_attr(ptdev->base.dev) ==
>>> DEV_DMA_COHERENT;
>>>
>>> if (!ptdev->coherent)
>>> @@ -34,8 +36,10 @@ static int panthor_gpu_coherency_init(struct
>>> panthor_device *ptdev)
>>> * ACE protocol has never been supported for command stream frontend
>>> GPUs.
>>> */
>>> if ((gpu_read(ptdev, GPU_COHERENCY_FEATURES) &
>>> - GPU_COHERENCY_PROT_BIT(ACE_LITE)))
>>> + GPU_COHERENCY_PROT_BIT(ACE_LITE))) {
>>> + ptdev->gpu_info.selected_coherency =
>>> GPU_COHERENCY_PROT_BIT(ACE_LITE);
>>> return 0;
>>> + }
>>>
>>> drm_err(&ptdev->base, "Coherency not supported by the device");
>>> return -ENOTSUPP;
>>> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c
>>> b/drivers/gpu/drm/panthor/panthor_gpu.c
>>> index 9d98720ce03f..a95c0b94ef58 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
>>> @@ -49,7 +49,7 @@ struct panthor_gpu {
>>> static void panthor_gpu_coherency_set(struct panthor_device *ptdev)
>>> {
>>> gpu_write(ptdev, GPU_COHERENCY_PROTOCOL,
>>> - ptdev->coherent ? GPU_COHERENCY_PROT_BIT(ACE_LITE) :
>>> GPU_COHERENCY_NONE);
>>> + ptdev->gpu_info.selected_coherency);
>>
>> It looks like an existing bug, but GPU_COHERENCY_PROTOCOL doesn't take a
>> bit mask. So we should be just writing GPU_COHERENCY_ACE_LITE not
>> GPU_COHERENCY_PROT_BIT(ACE_LITE).
>
> Oops. Should I prepare a fix, or does someone at Arm intend to send a
> fix for this one?
If you're happy to write up a fix then please do!
Thanks,
Steve
>>
>>> }
>>>
>>> static void panthor_gpu_l2_config_set(struct panthor_device *ptdev)
>>> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
>>> index 467d365ed7ba..b9e483ff5e21 100644
>>> --- a/include/uapi/drm/panthor_drm.h
>>> +++ b/include/uapi/drm/panthor_drm.h
>>> @@ -245,6 +245,26 @@ enum drm_panthor_dev_query_type {
>>> DRM_PANTHOR_DEV_QUERY_GROUP_PRIORITIES_INFO,
>>> };
>>>
>>> +/**
>>> + * enum drm_panthor_gpu_coherency: Type of GPU coherency
>>> + */
>>> +enum drm_panthor_gpu_coherency {
>>> + /**
>>> + * @DRM_PANTHOR_GPU_COHERENCY_ACE_LITE: ACE Lite coherency.
>>> + */
>>> + DRM_PANTHOR_GPU_COHERENCY_ACE_LITE = 1 << 0,
>>> +
>>> + /**
>>> + * @DRM_PANTHOR_GPU_COHERENCY_ACE_LITE: ACE coherency.
>>
>> Copy/paste mistake ^^^^^
>
> Will fix.
>
>>
>>> + */
>>> + DRM_PANTHOR_GPU_COHERENCY_ACE = 1 << 1,
>>> +
>>> + /**
>>> + * @DRM_PANTHOR_GPU_COHERENCY_NONE: No coherency.
>>> + */
>>> + DRM_PANTHOR_GPU_COHERENCY_NONE = 31,
>>> +};
>>
>> This is a mix of bit mask and non-bit mask. I'm assuming this was
>> intended for the newly added selected_coherency field, in which case it
>> shouldn't be shifting - the values are 0 and 1 for ace_lite and ace.
>
> Yeah, I think I went back and forth on this, and just picked the worst
> option in the end. I'll make it a real enum with the mapping you
> suggested.
