| Message ID | 1473619662-20777-3-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. 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. > + 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. > +} > + > 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,
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@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,
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@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
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@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).
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. */ + return cma_obj ? cma_obj->vaddr : NULL; +} + 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,
This enables panic message output for fb cma helper framebuffers. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+)