From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C9469C43381 for ; Wed, 6 Mar 2019 17:08:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 895A720661 for ; Wed, 6 Mar 2019 17:08:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727449AbfCFRIB (ORCPT ); Wed, 6 Mar 2019 12:08:01 -0500 Received: from mailgw01.mediatek.com ([210.61.82.183]:9435 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1727297AbfCFRIA (ORCPT ); Wed, 6 Mar 2019 12:08:00 -0500 X-UUID: e69d12f15fd74838b43a7ba0c13c2d92-20190307 X-UUID: e69d12f15fd74838b43a7ba0c13c2d92-20190307 Received: from mtkcas08.mediatek.inc [(172.21.101.126)] by mailgw01.mediatek.com (envelope-from ) (mhqrelay.mediatek.com ESMTP with TLS) with ESMTP id 2027153789; Thu, 07 Mar 2019 01:07:52 +0800 Received: from mtkcas07.mediatek.inc (172.21.101.84) by mtkmbs01n2.mediatek.inc (172.21.101.79) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Thu, 7 Mar 2019 01:07:44 +0800 Received: from [172.21.84.99] (172.21.84.99) by mtkcas07.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Thu, 7 Mar 2019 01:07:44 +0800 Message-ID: <1551892064.13389.9.camel@mtksdccf07> Subject: Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver From: Frederic Chen To: Brian Norris CC: "tfiga@chromium.org" , "matthias.bgg@gmail.com" , "mchehab@kernel.org" , "linux-mediatek@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" , Sean Cheng =?UTF-8?Q?=28=E9=84=AD=E6=98=87=E5=BC=98=29?= , "Sj Huang =?UTF-8?Q?=28=E9=BB=83=E4=BF=A1=E7=92=8B=29?=" , Christie Yu =?UTF-8?Q?=28=E6=B8=B8=E9=9B=85=E6=83=A0=29?= , Holmes Chiou =?UTF-8?Q?=28=E9=82=B1=E6=8C=BA=29?= , Jerry-ch Chen =?UTF-8?Q?=28=E9=99=B3=E6=95=AC=E6=86=B2=29?= , Jungo Lin =?UTF-8?Q?=28=E6=9E=97=E6=98=8E=E4=BF=8A=29?= , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , "linux-media@vger.kernel.org" , srv_heupstream , "laurent.pinchart+renesas@ideasonboard.com" , "hans.verkuil@cisco.com" Date: Thu, 7 Mar 2019 01:07:44 +0800 In-Reply-To: <20190228032411.GA84890@google.com> References: <1549020091-42064-1-git-send-email-frederic.chen@mediatek.com> <1549020091-42064-8-git-send-email-frederic.chen@mediatek.com> <20190207190824.GA98164@google.com> <1550756198.11724.86.camel@mtksdccf07> <1550902734.18654.36.camel@mtksdccf07> <20190228032411.GA84890@google.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-TM-SNTS-SMTP: C02EF19127A0190340605BFC88FA49D694483501FEEE67FBF39EB4FFAA40E16C2000:8 X-MTK: N Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Dear Brian, I appreciate your comments. On Wed, 2019-02-27 at 19:24 -0800, Brian Norris wrote: > Hi Frederic, > > On Sat, Feb 23, 2019 at 02:18:54PM +0800, Frederic Chen wrote: > > Dear Brian, > > > > I appreciate your comments. I'm really sorry for the delay in responding > > to the comments due to some mail subscribing failed issue inside my company. > > No problem. > > > On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote: > > > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote: > > > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote: > > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > > +{ > > > > > + struct mtk_dip_submit_work *dip_submit_work = > > > > > + container_of(work, struct mtk_dip_submit_work, frame_work); > > > > > + > > > > > + struct mtk_dip_hw_ctx *dip_ctx = dip_submit_work->dip_ctx; > > > > > + struct mtk_dip_work *dip_work; > > > > > + struct dip_device *dip_dev; > > > > > + struct dip_subframe *buf; > > > > > + u32 len, num; > > > > > + int ret; > > > > > + > > > > > + dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx); > > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > > + dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, > > > > > + struct mtk_dip_work, list_entry); > > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > > > > > I see you grab the head of the list here, but then you release the lock. > > > > Then you later assume that reference is still valid, throughout this > > > > function. > > > > > > > > That's usually true, because you only remove/delete entries from this > > > > list within this same workqueue (at the end of this function). But it's > > > > not true in dip_release_context() (which doesn't even grab the lock, > > > > BTW). > > > > > > > > I think there could be several ways to solve this, but judging by how > > > > this list entry is used...couldn't you just remove it from the list > > > > here, while holding the lock? Then you only have to kfree() it when > > > > you're done under the free_work_list label. > > > > > > > > I see. I would like to modify the codes as following: > > > > mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > You missed the part where you get the head of the list: > > dip_work = list_first_entry(...); > > But otherwise mostly looks OK. > > > dip_work->user_id->num--; > > Why do you need to do that with the queuelock held? Once you remove this > work item from the list (safely under the lock), shouldn't you be the > only one accessing it? > > (Note, I don't actually know what that 'num' really means. I'm just > looking at basic driver mechanics.) > Yes, there is only one user of the dip work at that time. I made a mistake on the usage of dip_useridlist.queuelock and dip_worklist.queuelock here. What I would like to do is to decrease the total number of the frames of the user, which is protected by dip_useridlist.queuelock. (user_id->num saves the total number of the dip frames belongs to a user; the user may be the preview or capture context.) On the other hand, the list of dip work is protected by another lock, dip_worklist.queuelock. In regarding to that point, I would like change the codes as following: mutex_lock(&dip_ctx->dip_worklist.queuelock); dip_work = list_first_entry(&dip_ctx->dip_worklist.queue, struct mtk_dip_work, list_entry); list_del(&dip_work->list_entry); dip_ctx->dip_worklist.queue_cnt--; len = dip_ctx->dip_worklist.queue_cnt; mutex_unlock(&dip_ctx->dip_worklist.queuelock); /* If the frame's user (preview or capture device) */ /* is in stream off state, */ /* return and release the buffers of the frame */ mutex_lock(&dip_ctx->dip_useridlist.queuelock); if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { dip_work->user_id->num--; mutex_unlock(&dip_ctx->dip_useridlist.queuelock); dip_work->frameparams.state = FRAME_STATE_STREAMOFF; call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); goto free_work_list; mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > list_del(&dip_work->list_entry); > > dip_ctx->dip_worklist.queue_cnt--; > > len = dip_ctx->dip_worklist.queue_cnt; > > mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > goto free_work_list; > > > > /* ...... */ > > > > free_work_list: > > kfree(dip_work); > > > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + if (dip_work->user_id->state == DIP_STATE_STREAMOFF) { > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + dip_work->frameparams.state = FRAME_STATE_STREAMOFF; > > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + dip_work->user_id->num--; > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n", > > > > > + dip_work->user_id->id, dip_work->user_id->num, > > > > > + dip_work->frameparams.frame_no, > > > > > + dip_work->frameparams.index); > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + goto free_work_list; > > > > > + } > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + while (num >= DIP_COMPOSING_MAX_NUM) { > > > > > + ret = wait_event_interruptible_timeout > > > > > + (dip_ctx->composing_wq, > > > > > + (num < DIP_COMPOSING_MAX_NUM), > > > > > + msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT)); > > > > > + > > > > > + if (ret == -ERESTARTSYS) > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "interrupted by a signal!\n"); > > > > > + else if (ret == 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "timeout frame_no(%d), num: %d\n", > > > > > + dip_work->frameparams.frame_no, num); > > > > > + else > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "wakeup frame_no(%d), num: %d\n", > > > > > + dip_work->frameparams.frame_no, num); > > > > > + > > > > > + num = atomic_read(&dip_ctx->num_composing); > > > > > + }; > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + if (list_empty(&dip_ctx->dip_freebufferlist.queue)) { > > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) index: %x no free buffer: %d\n", > > > > > + dip_work->frameparams.frame_no, > > > > > + dip_work->frameparams.index, > > > > > + dip_ctx->dip_freebufferlist.queue_cnt); > > > > > + > > > > > + /* Call callback to notify V4L2 common framework > > > > > + * for failure of enqueue > > > > > + */ > > > > > + dip_work->frameparams.state = FRAME_STATE_ERROR; > > > > > + call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_useridlist.queuelock); > > > > > + dip_work->user_id->num--; > > > > > + mutex_unlock(&dip_ctx->dip_useridlist.queuelock); > > > > > + > > > > > + goto free_work_list; > > > > > + } > > > > > + > > > > > + buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue, > > > > > + struct dip_subframe, > > > > > + list_entry); > > > > > + list_del(&buf->list_entry); > > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > > + mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue); > > > > > + dip_ctx->dip_usedbufferlist.queue_cnt++; > > > > > + mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + > > > > > + memcpy(&dip_work->frameparams.subfrm_data, > > > > > + &buf->buffer, sizeof(buf->buffer)); > > > > > + > > > > > + memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ); > > > > > + > > > > > + memcpy(&dip_work->frameparams.config_data, > > > > > + &buf->config_data, sizeof(buf->config_data)); > > > > > + > > > > > + memset((char *)buf->config_data.va, 0, DIP_COMP_SZ); > > > > > + > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) has no tuning_data\n", > > > > > + dip_work->frameparams.frame_no); > > > > > + > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > + > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > + /* When user enqueued without tuning buffer, > > > > > + * it would use driver internal buffer. > > > > > + * So, tuning_data.va should be 0 > > > > > + */ > > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > + } > > > > > + > > > > > + dip_work->frameparams.drv_data = (u64)dip_ctx; > > > > > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > > > > > + > > > > > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > > > > > + sizeof(dip_work->frameparams)); > > > > > + > > > > > + dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME, > > > > > + (void *)&dip_work->frameparams, > > > > > + sizeof(dip_work->frameparams), 0); > > > > > + num = atomic_inc_return(&dip_ctx->num_composing); > > > > > + > > > > > +free_work_list: > > > > > + > > > > > + mutex_lock(&dip_ctx->dip_worklist.queuelock); > > > > > + list_del(&dip_work->list_entry); > > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > > + len = dip_ctx->dip_worklist.queue_cnt; > > > > > + mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > > + > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "frame_no(%d) index: %x, worklist count: %d, composing num: %d\n", > > > > > + dip_work->frameparams.frame_no, dip_work->frameparams.index, > > > > > + len, num); > > > > > + > > > > > + kfree(dip_work); > > > > > +} > > > > > > > +int dip_release_context(struct dip_device *dip_dev) > > > > > > > > Should be static. > > > > > > > > I will change it to static. > > > > > > > +{ > > > > > + u32 i = 0; > > > > > + struct dip_subframe *buf, *tmpbuf; > > > > > + struct mtk_dip_work *dip_work, *tmp_work; > > > > > + struct dip_user_id *dip_userid, *tmp_id; > > > > > + struct mtk_dip_hw_ctx *dip_ctx; > > > > > + > > > > > + dip_ctx = &dip_dev->dip_ctx; > > > > > + dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n", > > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > > + > > > > > + list_for_each_entry_safe(dip_work, tmp_work, > > > > > + &dip_ctx->dip_worklist.queue, > > > > > + list_entry) { > > > > > > > > Shouldn't you be holding the mutex for this? Or alternatively, cancel > > > > any outstanding work and move the flush_workqueue()/destroy_workqueue() > > > > up. > > > > > > > > Similar questions for the other lists we're going through here. > > > > > > > > We missed the mutex holding here. I would like to change the codes as following: > > > > mutex_lock(&dip_ctx->dip_worklist.queuelock); > > list_for_each_entry_safe(dip_work, tmp_work, > > &dip_ctx->dip_worklist.queue, > > list_entry) { > > list_del(&dip_work->list_entry); > > dip_ctx->dip_worklist.queue_cnt--; > > kfree(dip_work); > > } > > mutex_unlock(&dip_ctx->dip_worklist.queuelock); > > > > I will also modify dip_useridlist and dip_ctx->dip_freebufferlist > > parts like dip_ctx->dip_worklist. > > Seems about right. > > Brian > > > > > > + list_del(&dip_work->list_entry); > > > > > + dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n", > > > > > + dip_work->frameparams.frame_no); > > > > > + kfree(dip_work); > > > > > + dip_ctx->dip_worklist.queue_cnt--; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_worklist.queue_cnt != 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "dip_worklist is not empty (%d)\n", > > > > > + dip_ctx->dip_worklist.queue_cnt); > > > > > + > > > > > + list_for_each_entry_safe(dip_userid, tmp_id, > > > > > + &dip_ctx->dip_useridlist.queue, > > > > > + list_entry) { > > > > > + list_del(&dip_userid->list_entry); > > > > > + dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n", > > > > > + dip_userid->id); > > > > > + kfree(dip_userid); > > > > > + dip_ctx->dip_useridlist.queue_cnt--; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_useridlist.queue_cnt != 0) > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "dip_useridlist is not empty (%d)\n", > > > > > + dip_ctx->dip_useridlist.queue_cnt); > > > > > + > > > > > + flush_workqueue(dip_ctx->mdpcb_workqueue); > > > > > + destroy_workqueue(dip_ctx->mdpcb_workqueue); > > > > > + dip_ctx->mdpcb_workqueue = NULL; > > > > > + > > > > > + flush_workqueue(dip_ctx->composer_wq); > > > > > + destroy_workqueue(dip_ctx->composer_wq); > > > > > + dip_ctx->composer_wq = NULL; > > > > > + > > > > > + atomic_set(&dip_ctx->num_composing, 0); > > > > > + atomic_set(&dip_ctx->num_running, 0); > > > > > + > > > > > + kthread_stop(dip_ctx->dip_runner_thread.thread); > > > > > + dip_ctx->dip_runner_thread.thread = NULL; > > > > > + > > > > > + atomic_set(&dip_ctx->dip_user_cnt, 0); > > > > > + atomic_set(&dip_ctx->dip_stream_cnt, 0); > > > > > + atomic_set(&dip_ctx->dip_enque_cnt, 0); > > > > > + > > > > > + /* All the buffer should be in the freebufferlist when release */ > > > > > + list_for_each_entry_safe(buf, tmpbuf, > > > > > + &dip_ctx->dip_freebufferlist.queue, > > > > > + list_entry) { > > > > > + struct sg_table *sgt = &buf->table; > > > > > + > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "buf pa (%d): %x\n", i, buf->buffer.pa); > > > > > + dip_ctx->dip_freebufferlist.queue_cnt--; > > > > > + dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl, > > > > > + sgt->orig_nents, > > > > > + DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC); > > > > > + sg_free_table(sgt); > > > > > + list_del(&buf->list_entry); > > > > > + kfree(buf); > > > > > + buf = NULL; > > > > > + i++; > > > > > + } > > > > > + > > > > > + if (dip_ctx->dip_freebufferlist.queue_cnt != 0 && > > > > > + i != DIP_SUB_FRM_DATA_NUM) > > > > > + dev_err(&dip_dev->pdev->dev, > > > > > + "dip_freebufferlist is not empty (%d/%d)\n", > > > > > + dip_ctx->dip_freebufferlist.queue_cnt, i); > > > > > + > > > > > + mutex_destroy(&dip_ctx->dip_useridlist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_worklist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock); > > > > > + mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + Sincerely, Frederic Chen