Re: [PATCH] modified: gpu/drm/panfrost/panfrost_gpu.c

[Steven Price]

On 10/06/2021 14:06, Chunyou Tang wrote:
> Hi Steven,

Hi Chunyou,

For some reason I'm not directly receiving your emails (only via the
list) - can you double check your email configuration?

>>> The GPU exception fault status register(0x3C),the low 8 bit is the
>>> EXCEPTION_TYPE.We can see the description at P3-78 in spec.
> 
> 	You can see the spec
> 	<arm_heimdall_technical_reference_manual_100612_0001_00_en.pdf>.

Thanks - please include that in the commit message - there are many TRMs
(one for each GPU) so without the information about exactly which
specification the page number is pretty useless. Sadly this
documentation isn't public which would be even better but I don't think
there are any public specs for this information.

>> However this change is correct - panfrost_exception_name() should be
>> taking only the lower 8 bits. Even better though would be to to report
>> the full raw fault information as well as the high bits can contain
>> useful information:
>>
>> 	dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
>> 		 fault_status,
>> 		 panfrost_exception_name(pfdev, fault_status & 0xFF),
>> 		 address);
> 
> So I change it according to what you said?

Yes, please send a v2.

Thanks,

Steve

> ÓÚ Thu, 10 Jun 2021 11:41:52 +0100
> Steven Price <steven.price@xxxxxxx> Đ´ľŔ:
> 
>> The subject should have the prefix "drm/panfrost" and should mention
>> what the patch is changing (not just the filename).
>>
>> On 09/06/2021 07:38, ChunyouTang wrote:
>>> From: tangchunyou <tangchunyou@xxxxxxxxxxxxxxxx>
>>>
>>> The GPU exception fault status register(0x3C),the low 8 bit is the
>>> EXCEPTION_TYPE.We can see the description at P3-78 in spec.
>>
>> Nit: When referring to a spec it's always good to mention the name -
>> I'm not sure which specification you found this in.
>>
>>>
>>> Signed-off-by: tangchunyou <tangchunyou@xxxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c index
>>> 2aae636f1cf5..1fffb6a0b24f 100644 ---
>>> a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++
>>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -33,7 +33,7 @@ static
>>> irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) address
>>> |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); 
>>>  		dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at
>>> 0x%016llx\n",
>>> -			 fault_status & 0xFF,
>>> panfrost_exception_name(pfdev, fault_status),
>>> +			 fault_status & 0xFF,
>>> panfrost_exception_name(pfdev, fault_status & 0xFF),
>>
>> However this change is correct - panfrost_exception_name() should be
>> taking only the lower 8 bits. Even better though would be to to report
>> the full raw fault information as well as the high bits can contain
>> useful information:
>>
>> 	dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
>> 		 fault_status,
>> 		 panfrost_exception_name(pfdev, fault_status & 0xFF),
>> 		 address);
>>
>> Thanks,
>>
>> Steve
>>
>>>  			 address);
>>>  
>>>  		if (state & GPU_IRQ_MULTIPLE_FAULT)
>>>
> 
>