On Thu, Jan 18, 2024 at 02:38:53PM +0100, Daniel Vetter wrote: > 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. Oh, sorry. I misinterpreted what you were going for with that review. Sorry for the noise. Maxime
Attachment:
signature.asc
Description: PGP signature
