On Fri, Jan 12, 2024 at 02:56:17PM +0100, Maxime Ripard wrote: > On Fri, Jan 12, 2024 at 02:41:53PM +0100, Daniel Vetter wrote: > > > + fb = plane->state->fb; > > > + /* Only support linear modifier */ > > > + if (fb->modifier != DRM_FORMAT_MOD_LINEAR) > > > + continue; > > > + > > > + /* Check if color format is supported */ > > > + if (!drm_panic_is_format_supported(fb->format->format)) > > > + continue; > > > + > > > + dma_obj = drm_fb_dma_get_gem_obj(fb, 0); > > > + > > > + /* Buffer should be accessible from the CPU */ > > > + if (dma_obj->base.import_attach) > > > > This might be a bit too restrictive, since some drivers import dma-buf > > including a vmap. So just checking for ->vaddr might be better. But can be > > changed later on. > > > > > + continue; > > > + > > > + /* Buffer should be already mapped to CPU */ > > > > I'd clarify this comment to state that vaddr is invariant over the > > lifetime of the buffer and therefore needs no locking. Correct locking > > that a) takes all the locks b) never ever stalls for one is absolutely > > crucial for a panic handler that won't make the situation worse. > > I think this comment was made to address buffers that are accessible to > the CPU but don't have a CPU mapping (ie, created with > DMA_ATTR_NO_KERNEL_MAPPING for example). But then the NULL value of vaddr would also be invariant ... My emphasis is more that we need to be really careful with all the locking rules here in the panic handler and not just assume it's going to be safe. I've stitched some tricky design together, but need to still move it from the whiteboard to a patch. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
