Re: [PATCH v2 04/13] drm/panthor: Expose the selected coherency protocol to the UMD

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 10 Oct 2025 15:22:50 +0100
Steven Price <steven.price@arm.com> 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 <boris.brezillon@collabora.com>
> > ---
> >  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?

> 
> >  }
> >  
> >  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.