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.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,FSL_HELO_FAKE,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=no 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 45803C43381 for ; Thu, 28 Feb 2019 03:24:26 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1352D2183F for ; Thu, 28 Feb 2019 03:24:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="c/TYSenL"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Ep2PK85I" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1352D2183F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=88agUzVy2DrIb9vzUrk7LQurmjwZO3o9xx+D59K1uYs=; b=c/TYSenLEprll4 tnQa/4E7OGj8kbP7c6m33afD/AfcxVldM/fyIqkhyubq7NItx8viV7Bh8zXgD7FsbPFqLw3EVzApa Zr9RIhNY9GnBpRo0To17TiM32LTxVHSBfenmUxxB68zgYtBWm8ybTd427A1BxyQyGQcPK3TPLV1Tx 2aau5rQDJ+ZcTq+RtLS80Wh1jwwU+V/jo3GnptQQb1EA2GEDSqbLLL8l9jB9kw0dh7Mz/qV+SOnml QsJlawS/5ML+erP4cgd4+tfhQoe2Bxc9Ed5tT+D4EW+75wbf8rKtJ3Jn407dlBgvN7o5ZizmJxltn wNnOOSeLIdrs5ofvQncQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1gzCJ4-0000cy-Aa; Thu, 28 Feb 2019 03:24:22 +0000 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1gzCIz-0000bd-WB for linux-arm-kernel@lists.infradead.org; Thu, 28 Feb 2019 03:24:20 +0000 Received: by mail-pf1-x442.google.com with SMTP id a3so8987332pff.11 for ; Wed, 27 Feb 2019 19:24:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=NEayOZWTOlW8hnuhgwXKXENHgurVhDXIl8x38Vkhwnc=; b=Ep2PK85IMTsK1Pr4E4asW5xap4WxYHVNWGtZTMzKGhq6TnBqc7vEymfZGpVuUrcI5c 0hLFf5YyFQ/SbG6nvE0BZbUNjgW1mDO8g5610xwwTC5+HjxVAr8EGc2I0xqJp6x8SVoE tQp+th28YI0dvyrPqdFrwT60gvS/7NkX6jKRE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=NEayOZWTOlW8hnuhgwXKXENHgurVhDXIl8x38Vkhwnc=; b=ad27yD9eAVnX6G5qHreHToizDIARSPMyhI2ij2gdLfbEgg0CvQnAWNGI5Ej9uyNFLL xMhh96Z+HbuycqXqUDSogbwPDlEF7Vw4k9co+k6HKh1hw13UZhyuc8nLbPDCFw82AB6n LpFunSgMolLmUw83xoWQwayDZWCBpEYncmGQOo5ACbUrcVv5PVSp/Oouvq2BHY3TOedw /bzBNtoG0mwV2p5KMEqydualjH/4UldhqafS5qcYZnfuwOoDb7BvBD+S1UU6oq+5lNWU mceA4UTH2alrdeypa///BBdxVHVj4px2zkv8G1Pd8NOAOm/2sk9gqtG7jRHVUgbl4AjP MDkQ== X-Gm-Message-State: AHQUAuaE0PM34pAzo1QDNI2PLqQoEfP6qHlsLz8POZYQ3tqsnsajavPM 8watD9h5bMukMWnAV3vuDtVVWg== X-Google-Smtp-Source: AHgI3IYjLUNFlqmDMdcGIOnvLgdzzpztO80aLWIYU7/ytOsFHy562kxjVQc46urxkGsi5+BhBjus1w== X-Received: by 2002:a62:b242:: with SMTP id x63mr5172210pfe.4.1551324257142; Wed, 27 Feb 2019 19:24:17 -0800 (PST) Received: from google.com ([2620:15c:202:1:534:b7c0:a63c:460c]) by smtp.gmail.com with ESMTPSA id s2sm21808114pgc.67.2019.02.27.19.24.15 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 27 Feb 2019 19:24:16 -0800 (PST) Date: Wed, 27 Feb 2019 19:24:13 -0800 From: Brian Norris To: Frederic Chen Subject: Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver Message-ID: <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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1550902734.18654.36.camel@mtksdccf07> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190227_192418_067183_C00A9803 X-CRM114-Status: GOOD ( 27.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sean Cheng =?utf-8?B?KOmEreaYh+W8mCk=?= , "laurent.pinchart+renesas@ideasonboard.com" , Rynn Wu =?utf-8?B?KOWQs+iCsuaBqSk=?= , srv_heupstream , Holmes Chiou =?utf-8?B?KOmCseaMuik=?= , Jerry-ch Chen =?utf-8?B?KOmZs+aVrOaGsik=?= , "tfiga@chromium.org" , Jungo Lin =?utf-8?B?KOael+aYjuS/iik=?= , Sj Huang =?utf-8?B?KOm7g+S/oeeSiyk=?= , "hans.verkuil@cisco.com" , "linux-mediatek@lists.infradead.org" , "matthias.bgg@gmail.com" , Christie Yu =?utf-8?B?KOa4uOmbheaDoCk=?= , "mchehab@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-media@vger.kernel.org" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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.) > 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; > > > > +} > > > > + _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel