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

[Laurent Pinchart]

Hi Daniel,

On Thursday 06 Oct 2016 11:12:40 Daniel Vetter wrote:
> On Wed, Oct 05, 2016 at 09:36:17PM +0200, Noralf Trønnes wrote:
> > Den 05.10.2016 15:22, skrev Laurent Pinchart:
> > > 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.

I agree with you, and I'd go one step further: we need to define very 
precisely the semantics of the dmabuf operations. Drivers cache various 
information in different ways, making interoperability very fragile.

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

-- 
Regards,

Laurent Pinchart