[PATCH v9 1/9] drm/panic: Add drm panic locking

[Jocelyn Falempe]

On 07/03/2024 11:27, John Ogness wrote:
> On 2024-03-07, Jocelyn Falempe <jfalempe at redhat.com> wrote:
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 39ef0a6addeb..c0bb91312fb2 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -38,6 +38,7 @@
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/drm_self_refresh_helper.h>
>>   #include <drm/drm_vblank.h>
>> @@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>   		}
>>   	}
>>   
>> +	drm_panic_lock(state->dev);
>>   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>>   		WARN_ON(plane->state != old_plane_state);
>>   
>> @@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>   		state->planes[i].state = old_plane_state;
>>   		plane->state = new_plane_state;
>>   	}
>> +	drm_panic_unlock(state->dev);
> 
> Is there a reason irqsave/irqrestore variants are not used? Maybe this
> code path is too hot?

This lock will be taken for each page flip, so typically at 60Hz (or 
maybe 144Hz for gamers). I don't know what are the performance impacts 
of the irqsave/irqrestore variant.
> 
> By leaving interrupts enabled, there is the risk that a panic from
> within any interrupt handler may block the drm panic handler.

The current design is that the panic handler will just use try_lock(), 
and if it can't take it, the panic screen will not be seen.
The goal is to make sure drm_panic won't crash the machine and prevent 
kdump or other panic handler to run. So there is a very small race 
chance that the panic screen won't be seen, but that's ok.

So I think in this case the drm panic handler shouldn't be blocked, as 
it only use try_lock().

Best regards,

-- 

Jocelyn

> 
> John Ogness
>