[PATCH] drm/etnaviv: Read some FE registers twice

[Lucas Stach]

Am Dienstag, dem 21.05.2024 um 12:10 -0500 schrieb Derek Foreman:
> Hello,
> 
> On 2024-05-16 12:36, Lucas Stach wrote:
> > Hi Derek,
> > 
> > Am Freitag, dem 03.05.2024 um 14:11 -0500 schrieb Derek Foreman:
> > > On some hardware (such at the GC7000 rev 6009), these registers need to be
> > > read twice to return the correct value. Hide that in gpu_read().
> > > 
> > > Signed-off-by: Derek Foreman <derek.foreman at collabora.com>
> > > ---
> > >   drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > > index 197e0037732e..0f67c62be3d1 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > > @@ -11,6 +11,7 @@
> > >   #include "etnaviv_mmu.h"
> > >   #include "etnaviv_drv.h"
> > >   #include "common.xml.h"
> > > +#include "state.xml.h"
> > >   
> > >   struct etnaviv_gem_submit;
> > >   struct etnaviv_vram_mapping;
> > > @@ -170,6 +171,13 @@ static inline void gpu_write(struct etnaviv_gpu *gpu, u32 reg, u32 data)
> > >   
> > >   static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
> > >   {
> > > +	/* On some variants, such as the GC7000 6009, some FE registers
> >                                           GC7000 rev 6009
> > > +	 * need two reads to be consistent. Do that extra read here and
> > > +	 * throw away the result.
> > > +	 */
> > Please use the common comment style of this driver with a blank line
> > after the /*
> Oops - done for v2. Thanks.
> > > +	if (reg >= VIVS_FE_DMA_STATUS && reg <= VIVS_FE_AUTO_FLUSH)
> > > +		readl(gpu->mmio + reg);
> > I don't think it matters much, but we can save some of the overhead
> > here by using readl_relaxed.
> 
> Can I just not do this? I'm concerned that losing the memory barrier 
> could result in weird reads again. The galcore driver doesn't use a 
> relaxed read, and these registers are only ever read when something goes 
> wrong - so I don't think performance is a concern at all?

I understand the concern. According to the architecture the memory
barriers wouldn't make a difference, as accesses to the same slave are
ordered, but as this is already a workaround for a HW weirdness we
don't exactly know if we can trust the architecture guarantees here.

Feel free to ignore my suggestion and keep this as-is.

Regards,
Lucas