On (20/04/28 16:47), Hans Verkuil wrote: > Hi Sergey, > > On 24/04/2020 11:29, Sergey Senozhatsky wrote: [..] > I missed that. What should happen is that q->allow_cache_hints is set by the > driver before vb2_queue_init is called. And the documentation should be updated > to say that the V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS flag is only valid when using the > MMAP streaming I/O model. > > Perhaps the flag should be renamed to V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS to > make this explicit? Other opinions are welcome. > > > > > Second. Even if the queue is setup, we still can report wrong cache > > hint values. Let's look at the following code > > > > fill_buf_caps(q, &p->capabilities); > > if (!vb2_queue_allows_cache_hints(q)) > > p->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT; > > The problem here is that vb2_queue_allows_cache_hints(q) uses stale information: > the current streaming mode instead of the requested streaming mode. > > This should read: > > if (!q->allow_cache_hints || p->memory != V4L2_MEMORY_MMAP) > p->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT; > > And V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is always set regardless of the > memory model. It just needs to be documented that this capability applies > to MMAP mode only. > > > ret = vb2_core_reqbufs(...); > > return ret; > > > > The thing here is that vb2_core_reqbufs() and vb2_core_create_bufs() > > can re-initialize the queue and invoke ->queue_setup(), possibly > > changing its memory model, etc. so cache hints cap which we set or > > clear before vb2_core_reqbufs() and vb2_core_create_bufs() can become > > invalid after we call those functions. It's the same with > > ``req->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT``, we cannot clear > > it before reqbufs()/create_bufs(). Therefore I added two simple > > functions which fixup cache hint cap and non_consistent flag after > > reqbufs()/create_bufs(). So the code looks like this now: > > > > fill_buf_caps(q, &p->capabilities); > > ret = vb2_core_reqbufs(...); > > fixup_consistency_attr(q, &p->flags); > > fixup_cache_hints_cap(q, &p->capabilities); > > These fixup functions are ugly, unless I missed something I think the > approach described above works just fine. > > With these changes I think it is ready to go in. ACK to all of these. Will send the updated patch set shortly. -ss
