From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris BREZILLON Subject: Re: [RFC PATCH] drm: rework flip-work helpers to avoid calling func when the FIFO is full Date: Fri, 11 Jul 2014 17:57:50 +0200 Message-ID: <20140711175750.20c57776@bbrezillon> References: <1405091821-31839-1-git-send-email-boris.brezillon@free-electrons.com> <20140711174705.099e3a3a@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.free-electrons.com (top.free-electrons.com [176.31.233.9]) by gabe.freedesktop.org (Postfix) with ESMTP id 303DB6E042 for ; Fri, 11 Jul 2014 08:57:53 -0700 (PDT) In-Reply-To: <20140711174705.099e3a3a@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris BREZILLON Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On Fri, 11 Jul 2014 17:47:05 +0200 Boris BREZILLON wrote: > On Fri, 11 Jul 2014 11:41:12 -0400 > Rob Clark wrote: > > > On Fri, Jul 11, 2014 at 11:17 AM, Boris BREZILLON > > 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 > > > --- > > > 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