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

[Rob Clark]

On Fri, Jul 11, 2014 at 11:47 AM, Boris BREZILLON
<boris.brezillon at free-electrons.com> wrote:
> On Fri, 11 Jul 2014 11:41:12 -0400
> Rob Clark <robdclark at gmail.com> wrote:
>
>> On Fri, Jul 11, 2014 at 11:17 AM, Boris BREZILLON
>> <boris.brezillon at 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 at 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 ;-)

yeah, I think passing gfp flags is the better idea

>> 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).

we do have existing users that call drm_flip_work_queue() from irq..
but I suppose adding gfp flags arg to drm_flip_work_queue() seems like
a reasonable solution.

BR,
-R

> 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