Re: [PATCH v4 4/4] drm/mgag200: Add drm_panic support

[Jocelyn Falempe <jfalempe@redhat.com>]

On 07/10/2023 16:30, Noralf Trønnes wrote:
> 
> 
> On 10/3/23 16:22, Jocelyn Falempe wrote:
>> Add support for the drm_panic module, which displays a message to
>> the screen when a kernel panic occurs.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_drv.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 976f0ab2006b..229d9c116b42 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -12,10 +12,12 @@
>>   #include <drm/drm_aperture.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fbdev_generic.h>
>> +#include <drm/drm_framebuffer.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_ioctl.h>
>>   #include <drm/drm_managed.h>
>>   #include <drm/drm_module.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_pciids.h>
>>   
>>   #include "mgag200_drv.h"
>> @@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem, resource_size_t size)
>>   	return offset - 65536;
>>   }
>>   
>> +static int mgag200_get_scanout_buffer(struct drm_device *dev,
>> +				      struct drm_scanout_buffer *sb)
>> +{
>> +	struct drm_plane *plane;
>> +	struct mga_device *mdev = to_mga_device(dev);
>> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
>> +
>> +	/* mgag200 has only one plane */
>> +	drm_for_each_plane(plane, dev) {
> 
> In my first 2016 attempt on a panic handler I was told to trylock crtc
> and plane and skip if it wasn't possible:
> 
> - We need locking. One of the biggest problems with the old oops handling
>    was that it was very good at trampling over driver state, causing more
>    (unrelated) oopses in kms code and making sure the original oops was no
>    longer visible. I think the shared code must take care of all the
>    locking needs to avoid fragile code in drivers. ww_mutex_trylock on the
>    drm_crtc and drm_plane should be enough (we need both for drivers where
>    planes can be reassigned between drivers).

drm_panic register a panic handler, so it won't get called on oopses.
So when the panic handler is called, no other task can run in parallel, 
and no drm code will run after it.

Also for the Matrox driver, it's always safe to write to the VRAM. The 
only risk is that if you're in the middle of a modeset, you may get 
garbage output.
There might still be a race condition for fb->format, width, height and 
pitches, and looping through the plane list.

I also need to check if crtc and plane locks are not taken when the 
driver is copying the damage region to the VRAM, otherwise the 
probability to actually see the panic will be very low.

I also need to wake up the monitor, if it's currently in DPMS off.

-- 

Jocelyn


> 
> See [1] for a list of other things to consider.
> 
> Noralf.
> 
> [1]
> https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/
> 
>> +		if (!plane->state || !plane->state->fb)
>> +			return -ENODEV;
>> +		sb->format = plane->state->fb->format;
>> +		sb->width = plane->state->fb->width;
>> +		sb->height = plane->state->fb->height;
>> +		sb->pitch = plane->state->fb->pitches[0];
>> +		sb->map = map;
>> +		return 0;
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>>   /*
>>    * DRM driver
>>    */
>> @@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
>>   	.major = DRIVER_MAJOR,
>>   	.minor = DRIVER_MINOR,
>>   	.patchlevel = DRIVER_PATCHLEVEL,
>> +	.get_scanout_buffer = mgag200_get_scanout_buffer,
>>   	DRM_GEM_SHMEM_DRIVER_OPS,
>>   };
>>   
>