<!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <div class="moz-cite-prefix">Hi Lucas,</div> <div class="moz-cite-prefix"> </div> <div class="moz-cite-prefix">On 2024-05-03 12:12, Lucas Stach wrote: </div> <blockquote type="cite" cite="mid:489f726758ecfcec0b3955530f405d93ea4a4e99.camel@pengutronix.de"> <pre class="moz-quote-pre" wrap="">Hi Derek, Am Freitag, dem 03.05.2024 um 09:27 -0500 schrieb Derek Foreman: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">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? </pre> </blockquote> <pre class="moz-quote-pre" wrap="">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.</pre> </blockquote> Yes, this is a GC7000 rev 6009, and it seems to occasionally return very exciting values on the first read. An example: 0000065c = 0000c1f8 00000660 = 00000001 Cmd: [dec DMA: idle Fetch: idle] Req idle Cal idle 00000664 = 00000905 Command DMA address 00000668 = 0000c1f8 FE fetched word 0 0000066c = 080105d0 FE fetched word 1 00000670 = feb71000 Some of the values are shifted by one register. 905 should have been in 660, c1f8 should have been in 664, and the state load should have been in 668 instead of 66c. Very confusing. I can only assume this weirdness is why the gpu-viv driver unconditionally reads the DMA registers twice every time. I expect that the extra read is pointless on most of the registers in the dump, but I was lazy - would you prefer I limit the extra read to only DMA registers? It looks like we use these registers in other places (verify_dma, etnaviv_gpu_debugfs). Should I add this double read there as well? <blockquote type="cite" cite="mid:489f726758ecfcec0b3955530f405d93ea4a4e99.camel@pengutronix.de"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">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? </pre> </blockquote> <pre class="moz-quote-pre" wrap="">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.</pre> </blockquote> Fair enough - I won't bother sending a patch to change that behavior. I do wonder if setting the timeout to be twice as long instead of requiring it to trigger twice would be more intuitive, though. Thanks, Derek <blockquote type="cite" cite="mid:489f726758ecfcec0b3955530f405d93ea4a4e99.camel@pengutronix.de"> <pre class="moz-quote-pre" wrap=""> Regards, Lucas </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Thanks, Derek On 2024-05-03 09:22, Derek Foreman wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:derek.foreman@collabora.com"><derek.foreman@collabora.com></a> --- 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)); } </pre> </blockquote> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>