On Fri, 10 Oct 2025 15:50:58 +0100 Steven Price <[email protected]> wrote:
> On 10/10/2025 11:11, Boris Brezillon wrote: > > Will be needed if we want to skip CPU cache maintenance operations when > > the GPU can snoop CPU caches. > > > > v2: > > - New commit > > > > Signed-off-by: Boris Brezillon <[email protected]> > > --- > > drivers/gpu/drm/panfrost/panfrost_device.h | 1 + > > drivers/gpu/drm/panfrost/panfrost_drv.c | 1 + > > drivers/gpu/drm/panfrost/panfrost_gpu.c | 18 +++++++++++++++++- > > drivers/gpu/drm/panfrost/panfrost_regs.h | 2 ++ > > include/uapi/drm/panfrost_drm.h | 7 +++++++ > > 5 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h > > b/drivers/gpu/drm/panfrost/panfrost_device.h > > index 1e73efad02a8..bd38b7ae169e 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_device.h > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h > > @@ -75,6 +75,7 @@ struct panfrost_features { > > u32 thread_max_workgroup_sz; > > u32 thread_max_barrier_sz; > > u32 coherency_features; > > + u32 selected_coherency; > > u32 afbc_features; > > u32 texture_features[4]; > > u32 js_features[16]; > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > > b/drivers/gpu/drm/panfrost/panfrost_drv.c > > index 607a5b8448d0..3ffcd08f7745 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > > @@ -94,6 +94,7 @@ static int panfrost_ioctl_get_param(struct drm_device > > *ddev, void *data, struct > > PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15); > > PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups); > > PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc); > > + PANFROST_FEATURE(SELECTED_COHERENCY, selected_coherency); > > > > case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP: > > ret = panfrost_ioctl_query_timestamp(pfdev, ¶m->value); > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > > b/drivers/gpu/drm/panfrost/panfrost_gpu.c > > index 174e190ba40f..fed323e6a307 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c > > @@ -260,7 +260,23 @@ static void panfrost_gpu_init_features(struct > > panfrost_device *pfdev) > > pfdev->features.max_threads = gpu_read(pfdev, GPU_THREAD_MAX_THREADS); > > pfdev->features.thread_max_workgroup_sz = gpu_read(pfdev, > > GPU_THREAD_MAX_WORKGROUP_SIZE); > > pfdev->features.thread_max_barrier_sz = gpu_read(pfdev, > > GPU_THREAD_MAX_BARRIER_SIZE); > > - pfdev->features.coherency_features = gpu_read(pfdev, > > GPU_COHERENCY_FEATURES); > > + > > + if (panfrost_has_hw_feature(pfdev, HW_FEATURE_COHERENCY_REG)) > > + pfdev->features.coherency_features = gpu_read(pfdev, > > GPU_COHERENCY_FEATURES); > > + else > > + pfdev->features.coherency_features = COHERENCY_ACE_LITE; > > + > > + if (!pfdev->coherent) { > > + pfdev->features.selected_coherency = COHERENCY_NONE; > > + } else if (pfdev->features.coherency_features & COHERENCY_ACE) { > > + pfdev->features.selected_coherency = COHERENCY_ACE; > > + } else if (pfdev->features.coherency_features & COHERENCY_ACE_LITE) { > > + pfdev->features.selected_coherency = COHERENCY_ACE_LITE; > > + } else { > > + drm_WARN(pfdev->ddev, true, "No known coherency protocol > > supported"); > > + pfdev->features.selected_coherency = COHERENCY_NONE; > > + } > > Same comment as for panthor about not using bits when we can't have more > than one. But also here because selected_coherency is only a UAPI > concept, it would make sense to use the UAPI values, i.e. > DRM_PANFROST_GPU_COHERENCY_ACE_LITE etc rather than the private > COHERENCY_ACE_LITE defines. For simplicity (we simply copy the coherency_features from the GPU reg at the moment), I want the HW/uAPI values to match, so I've added BUILD_BUG_ON()s. And I think I'd prefer to stick to the defs in panfrost_regs.h, such that if we ever end up writing that back to COHERENCY_ENABLE on newer HW, it's obvious we based the initialization on those HW values. > > Although there is actually a COHERENCY_ENABLE register on some GPUs > (BASE_HW_FEATURE_COHERENCY_REG in the kbase driver). Looks like it was > probably introduced for Bifrost. But AFAIK the above checks should be ok. Yep. I didn't dare writing it back, because it's working as-is on all supported HW, and I don't want to regress things. Not that I've played with this COHERENCY_ENABLE reg on my amlogic board, which is coherent, to fake a non-coherent setup, and it works like a charm :-).
