[PATCH] drm/etnaviv: during dump, read registers twice

[Lucas Stach]

Hi Derek,

Am Freitag, dem 03.05.2024 um 09:27 -0500 schrieb Derek Foreman:
> I have a follow up question related to this:
> 
> In etnaviv_sched_timedoutjob we only read this register once - we should 
> probably read twice there as well?
> 
The register read being unreliable seems to be a specialty of the GPU
you are dealing with. I'm not saying that making this more robust is
the wrong thing to do, but I've seen quite a few GPU hang issues over
time and the reported DMA address always made sense on the GPUs I've
seen so far.

> Further, that function appears to always treat the first timeout as 
> spurious because gpu->hangcheck_dma_addr starts as 0? The Vivante 
> driver's hang check spins briefly to see if forward progress is being 
> made, would that be a better thing to do there?
> 
Briefly spinning will yield many false positives, as the DMA frontend
address doesn't move during large draws. The timeout we are using is
short enough that in most cases it's okay that we need to hit the
timeout twice until we mark the GPU as hung, but also long enough so it
doesn't get confused by large draws.

Regards,
Lucas

> Thanks,
> Derek
> 
> On 2024-05-03 09:22, Derek Foreman wrote:
> > The vivante driver always reads dma registers twice and discards the first
> > value - we need to do this too or at least the DMA address and low/high
> > fetches can return wrong results.
> > 
> > Signed-off-by: Derek Foreman <derek.foreman at collabora.com>
> > ---
> >   drivers/gpu/drm/etnaviv/etnaviv_dump.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> > index 898f84a0fc30..8a8ca8dcc49a 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> > @@ -91,6 +91,8 @@ static void etnaviv_core_dump_registers(struct core_dump_iterator *iter,
> >   		    read_addr <= VIVS_PM_PULSE_EATER)
> >   			read_addr = gpu_fix_power_address(gpu, read_addr);
> >   		reg->reg = cpu_to_le32(etnaviv_dump_registers[i]);
> > +		/* Discard first read, as it is frequently inaccurate */
> > +		gpu_read(gpu, read_addr);
> >   		reg->value = cpu_to_le32(gpu_read(gpu, read_addr));
> >   	}
> >