[PATCH v5 6/6] drm/imx: Add drm_panic support

[Jocelyn Falempe]

On 14/12/2023 14:48, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Nov 03, 2023 at 03:53:30PM +0100, Jocelyn Falempe wrote:
>> Proof of concept to add drm_panic support on an arm based GPU.
>> I've tested it with X11/llvmpipe, because I wasn't able to have
>> 3d rendering with etnaviv on my imx6 board.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> 
> Like I said in the v6, this shouldn't be dropped because it also kind of
> documents and shows what we are expecting from a "real" driver.

Ok, I can bring it back in v7.

> 
>> ---
>>   drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 30 ++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
>> index 4a866ac60fff..db24b4976c61 100644
>> --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/dma-buf.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/iosys-map.h>
>>   
>>   #include <video/imx-ipu-v3.h>
>>   
>> @@ -17,9 +18,12 @@
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fbdev_dma.h>
>> +#include <drm/drm_fb_dma_helper.h>
>> +#include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_dma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>>   #include <drm/drm_managed.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_of.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <drm/drm_vblank.h>
>> @@ -160,6 +164,31 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
>>   	return ret;
>>   }
>>   
>> +static int imx_drm_get_scanout_buffer(struct drm_device *dev,
>> +				      struct drm_scanout_buffer *sb)
>> +{
>> +	struct drm_plane *plane;
>> +	struct drm_gem_dma_object *dma_obj;
>> +
>> +	drm_for_each_plane(plane, dev) {
>> +		if (!plane->state || !plane->state->fb || !plane->state->visible ||
>> +		    plane->type != DRM_PLANE_TYPE_PRIMARY)
>> +			continue;
>> +
>> +		dma_obj = drm_fb_dma_get_gem_obj(plane->state->fb, 0);
>> +		if (!dma_obj->vaddr)
>> +			continue;
>> +
>> +		iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
>> +		sb->format = plane->state->fb->format;
> 
> Planes can be using a framebuffer in one of the numerous YUV format the
> driver advertises.
> 
>> +		sb->height = plane->state->fb->height;
>> +		sb->width = plane->state->fb->width;
>> +		sb->pitch = plane->state->fb->pitches[0];
> 
> And your code assumes that the buffer will be large enough for an RGB
> buffer, which probably isn't the case for a single-planar YUV format,
> and certainly not the case for a multi-planar one.

Yes, this code is a bit hacky, and on my test setup the framebuffer was 
in RGB, so I didn't handle other formats.
Also it should be possible to add YUV format later, but I would like to 
have a minimal drm_panic merged, before adding more features.
> 
> When the driver gives back its current framebuffer, the code should check:
> 
>    * If the buffer backed by an actual buffer (and not a dma-buf handle)

Regarding the struct drm_framebuffer, I'm not sure how do you 
differentiate an actual buffer from a dma-buf handle ?

>    * If the buffer is mappable by the CPU

If "dma_obj->vaddr" is not null, then it's already mapped by the CPU right ?
>    * If the buffer is in a format that the panic code can handle
>    * If the buffer uses a linear modifier

Yes, that must be checked too.

> 
> Failing that, your code cannot work at the moment. We need to be clear
> about that and "gracefully" handle things instead of going forward and
> writing pixels to places we might not be able to write to.
> 
> Which kind of makes me think, why do we need to involve the driver at
> all there?
> 
> If in the panic code, we're going over all enabled CRTCs, finding the
> primary plane currently setup for them and getting the drm_framebuffer
> assigned to them, it should be enough to get us all the informations we
> need, right?

Yes, I think I can do a generic implementation for the drivers using the 
drm_gem_dma helper like imx6.
But for simpledrm, ast, or mgag200, I need to retrieve the VRAM address, 
because it's not in the drm_framebuffer struct, so they won't be able to 
use this generic implementation.

> 
> Maxime

Thanks for the review,

-- 

Jocelyn