Re: [RFC PATCH] drm: rework flip-work helpers to avoid calling func when the FIFO is full

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

On Fri, 11 Jul 2014 17:47:05 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> On Fri, 11 Jul 2014 11:41:12 -0400
> Rob Clark <robdclark@gmail.com> wrote:
> 
> > On Fri, Jul 11, 2014 at 11:17 AM, Boris BREZILLON
> > <boris.brezillon@free-electrons.com> wrote:
> > > Make use of lists instead of kfifo in order to dynamically allocate
> > > task entry when someone require some delayed work, and thus preventing
> > > drm_flip_work_queue from directly calling func instead of queuing this
> > > call.
> > > This allow drm_flip_work_queue to be safely called even within irq
> > > handlers.
> > >
> > > Add new helper functions to allocate a flip work task and queue it when
> > > needed. This prevents allocating data within irq context (which might
> > > impact the time spent in the irq handler).
> > >
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > > Hi Rob,
> > >
> > > This is a proposal for what you suggested (dynamically growing the drm
> > > flip work queue in order to avoid direct call of work->func when calling
> > > drm_flip_work_queue).
> > >
> > > I'm not sure this is exactly what you expected, because I'm now using
> > > lists instead of kfifo (and thus lose the lockless part), but at least
> > > we can now safely call drm_flip_work_queue or drm_flip_work_queue_task
> > > from irq handlers :-).
> > >
> > > You were also worried about queueing the same framebuffer multiple times
> > > and with this implementation you shouldn't have any problem (at least with
> > > drm_flip_work_queue, what people do with drm_flip_work_queue_task is their
> > > own responsability, but they should allocate one task for each operation
> > > even if they are manipulating the same framebuffer).
> > 
> > yeah, if we are dynamically allocating the list nodes, that solves the
> > queuing-up-multiple-times issue..
> > 
> > I wonder if drm_flip_work_allocate_task() should use GPF_ATOMIC when
> > allocating?
> 
> That's funny, I was actually modifying the API to pass gfp_t flags to
> this function ;-)
> 
> > I guess maybe it is possible to pre-allocate the task
> > from non-irq context, and then queue it from irq context.. it makes
> > the API a bit more complex, but there are only a couple users
> > currently, so I suppose this should be doable.
> 
> I tried to keep the existing API so that existing users won't see the
> difference (I guess none of them are calling drm_flip_work_queue).

Some words are missing :-):

(I guess none of them are calling drm_flip_work_queue from irq
handlers).

> 
> I just added the drm_flip_work_allocate_task and
> drm_flip_work_queue_task for those who want more control on the
> queuing process.
> 
> Best Regards,
> 
> Boris
> 
> 
> 
> 
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com