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

[Daniel Vetter]

On Wed, Oct 05, 2016 at 09:36:17PM +0200, Noralf Trønnes wrote:
> 
> 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.

dma_buf's kmap_atomic is a bit ill-defined, since atm it's not clear how
to get the buffer pinned in the first place. Originally the plan was to
use begin/end_cpu_access for this (which again can block, so not suitable
for panic context). But that was kinda reused as the coherence control
interface, and doesn't really pin anything. I guess we should probably
just nuke the kmap interface part of dma_buf, the idea was to use that for
ttm, but that never really happened.

> > 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 :-)

Yeah I think this is a bit too much paranoia. I think you can remove these
NULL checks here, it's truly impossible. Where we really need to be
careful is with locking, both to make sure we exclusively use trylocks for
everything, and that we do grab all the locks (to avoid disaster when the
part that panicked is kms itself).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch