[PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Noralf Trønnes]

Den 05.10.2016 15:22, skrev Laurent Pinchart:
> Hi Noralf,
>
> Thank you for the patch.
>
> On Sunday 11 Sep 2016 20:47:41 Noralf Trønnes wrote:
>> This enables panic message output for fb cma helper framebuffers.
>>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac..2f1b012 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct drm_framebuffer
>> *fb, }
>>   EXPORT_SYMBOL(drm_fb_cma_create_handle);
>>
>> +static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
>> +{
>> +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
>> +	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
>> +
>> +	/* A PRIME imported buffer will not have it's vaddr set. */
> Does this mean that, if the framebuffer that happens to be displayed when a
> panic occurs is imported, no message will be printed ? I understand how
> supporting such cases is difficult, but I'm wondering how we could proceed to
> ensure that a panic can be displayed in most (if not all) cases.

If we can vmap all cma buffers, then it's simple:
- Add dma_buf_vmap() call to drm_gem_cma_prime_import_sg_table()
- Add dma_buf_vunmap() call to drm_gem_cma_free_object()

If not then it looks more complicated, since this is atomic context.
There is dma_buf_kmap_atomic(), but there are no users.
And drm_gem_prime_dmabuf_ops doesn't support .kmap_atomic either
(returns NULL).

omapdrm is the only dma_buf provider I can find that actually uses
kmap_atomic() instead of just returning NULL or relying on an existing
virtual address. It has it's own .gem_prime_import/export functions to
accomplish this.

> Similarly, it looks like your code only handles single-planar formats, but
> there's no explicit check for that in patch 1/3. The easiest fix is to reject
> multi-planar framebuffers, but that would again result in silent panics in
> some cases.

That's correct if we talk about the default panic_draw_xy() function:
drm_framebuffer_panic_draw_xy(). Most likely this function can be extended
to support multi-planar formats, but Daniel said we could wait with that.
And the driver can also implement it's own .panic_draw_xy() function.

>> +	return cma_obj ? cma_obj->vaddr : NULL;
> Can cma_obj be NULL here ? I thought that framebuffer objects were always
> created with at least one GEM object.

I was trying to be very careful since a panic is about something gone
very wrong. But in that case I should have checked that fb_cma isn't NULL
also before dereferencing it.
Maybe I'm over the top paranoid :-)

Noralf.

>> +}
>> +
>>   static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
>>   	.destroy	= drm_fb_cma_destroy,
>>   	.create_handle	= drm_fb_cma_create_handle,
>> +	.panic_vmap	= drm_fb_cma_panic_vmap,
>>   };
>>
>>   static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,