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=-0.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 DA6AAC3A5A5 for ; Tue, 3 Sep 2019 07:04:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 923FA22DCC for ; Tue, 3 Sep 2019 07:04:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="K0+YX7Vt" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726452AbfICHEq (ORCPT ); Tue, 3 Sep 2019 03:04:46 -0400 Received: from mail-ed1-f68.google.com ([209.85.208.68]:41212 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726062AbfICHEp (ORCPT ); Tue, 3 Sep 2019 03:04:45 -0400 Received: by mail-ed1-f68.google.com with SMTP id z9so12228518edq.8 for ; Tue, 03 Sep 2019 00:04:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=db032M6/zE4omf+cGRilv5nsZk5oRYhR48JuFpDhX0E=; b=K0+YX7VtC8i3mcTjCo3bRNli9qCdtHJx01NQ5bMbRKQAf6oJKzsNLIiknOSlueBKPy ETBBVeyeVTpEEsMEyuRaacy0a65EUnUp7Kc0XA/w6kpcCGhoFe3zG91siutUiHv5iEu8 lPt9ap0YWic/FlpxdQLJiHU/hTqmtFQiviOow= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=db032M6/zE4omf+cGRilv5nsZk5oRYhR48JuFpDhX0E=; b=Oevqlzlo5ctapbg6Rev7xVWh8PrUDQEP/QbNzrzVsCU1Jw14gorZuooH5BQXR1yBAi YbBAYw/hs3O/WHJbkSXCyHovz8beqwRYLW0UMBckY2Blu+qj1eVv1eU6dhfiSn9yNFXG SkcUkex+MMyAL4dU8gmn6fh9q1XWtgNxiO11Qy65LNsY6XR4LEJvgsTvZYztC8kBXfod a8OUPoopFaTk4ACGdayF5QeHPtnnEtKbfVsz0nBU19fYAgky/ZemgNEZVplxpEwxYzWT mLKOzlSkZoV4xivKOQhv/JjudSZtwGaAGDn76BTIdB/zEoY74ax8OVHmUZ0blKnjJytD bpUQ== X-Gm-Message-State: APjAAAV2U3b5/rF1hnYaI3hOJpLP8qX30SFK0bVD7a9ByJqs59I5T5Dc Po/7yAtlAHiZj0onSulsPkfC0tClxxyrNQ== X-Google-Smtp-Source: APXvYqwIg7F+/Woc8T9rBZlyuYcgHediFX0WClP0UKfN//ViSX8alOKYp8zv42z5eVG5dy8MuTmRow== X-Received: by 2002:a05:6402:1214:: with SMTP id c20mr24657598edw.111.1567494283203; Tue, 03 Sep 2019 00:04:43 -0700 (PDT) Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com. [209.85.221.49]) by smtp.gmail.com with ESMTPSA id a36sm2160404edc.58.2019.09.03.00.04.41 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 Sep 2019 00:04:42 -0700 (PDT) Received: by mail-wr1-f49.google.com with SMTP id 30so5208727wrk.11 for ; Tue, 03 Sep 2019 00:04:41 -0700 (PDT) X-Received: by 2002:a5d:6585:: with SMTP id q5mr9827920wru.162.1567494281422; Tue, 03 Sep 2019 00:04:41 -0700 (PDT) MIME-Version: 1.0 References: <1562661672-22439-1-git-send-email-Jerry-Ch.chen@mediatek.com> <1562661672-22439-5-git-send-email-Jerry-Ch.chen@mediatek.com> <20190802082815.GA203993@chromium.org> <1566724680.20680.8.camel@mtksdccf07> <1566957625.20680.33.camel@mtksdccf07> <1567424859.18318.32.camel@mtksdccf07> <1567493081.18318.49.camel@mtksdccf07> In-Reply-To: <1567493081.18318.49.camel@mtksdccf07> From: Tomasz Figa Date: Tue, 3 Sep 2019 16:04:29 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver To: Jerry-ch Chen Cc: "yuzhao@chromium.org" , "zwisler@chromium.org" , "linux-mediatek@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" , =?UTF-8?B?U2VhbiBDaGVuZyAo6YSt5piH5byYKQ==?= , =?UTF-8?B?U2ogSHVhbmcgKOm7g+S/oeeSiyk=?= , =?UTF-8?B?Q2hyaXN0aWUgWXUgKOa4uOmbheaDoCk=?= , =?UTF-8?B?RnJlZGVyaWMgQ2hlbiAo6Zmz5L+K5YWDKQ==?= , =?UTF-8?B?SnVuZ28gTGluICjmnpfmmI7kv4op?= , =?UTF-8?B?UnlubiBXdSAo5ZCz6IKy5oGpKQ==?= , =?UTF-8?B?UG8tWWFuZyBIdWFuZyAo6buD5p+P6Zm9KQ==?= , "shik@chromium.org" , "suleiman@chromium.org" , "linux-media@vger.kernel.org" , srv_heupstream , "devicetree@vger.kernel.org" , "laurent.pinchart+renesas@ideasonboard.com" , "hans.verkuil@cisco.com" , "mchehab@kernel.org" , "matthias.bgg@gmail.com" Content-Type: text/plain; charset="UTF-8" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen wrote: > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote: > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen wrote: > > > > > > Hi Tomasz, > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote: > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen > > > > wrote: > > > > > > > > > > Hi Tomasz, > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote: > > > > > > Hi Jerry, > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen > > > > > > wrote: > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote: > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote: > > [snip] > > > > > static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq, > > > > > unsigned int *num_buffers, > > > > > unsigned int *num_planes, > > > > > unsigned int sizes[], > > > > > struct device *alloc_devs[]) > > > > > { > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > struct device *dev = ctx->dev; > > > > > unsigned int size[2]; > > > > > > > > > > switch (vq->type) { > > > > > case V4L2_BUF_TYPE_META_CAPTURE: > > > > > size[0] = ctx->dst_fmt.buffersize; > > > > > break; > > > > > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > > > > > size[0] = ctx->src_fmt.plane_fmt[0].sizeimage; > > > > > if (*num_planes == 2) > > > > > size[1] = ctx->src_fmt.plane_fmt[1].sizeimage; > > > > > break; > > > > > } > > > > > > > > > > if (*num_planes == 1) { > > > > > if (sizes[0] < size[0]) > > > > > return -EINVAL; > > > > > } else if (*num_planes == 2) { > > > > > if ((sizes[0] < size[0]) && (sizes[1] < size[1])) > > > > > return -EINVAL; > > > > > > > > Can we just use a loop here and combine the 2 cases above? > > > > > > > > Also, we need to fail with -EINVAL if *num_planes is > 2. > > > > > > > > > } else { > > > > > *num_planes = 1; > > > > > sizes[0] = size[0]; > > > > > > > > This should be the case if *num_planes == 0 and the number of planes > > > > and sizes should match the currently active format. > > > > > > > I appreciate your comments, > > > > > > Ok, I will update as following: > > > static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq, > > > unsigned int *num_buffers, > > > unsigned int *num_planes, > > > unsigned int sizes[], > > > struct device *alloc_devs[]) > > > { > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > unsigned int size[2]; > > > unsigned int plane; > > > > > > switch (vq->type) { > > > case V4L2_BUF_TYPE_META_CAPTURE: > > > size[0] = ctx->dst_fmt.buffersize; > > > break; > > > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > > > size[0] = ctx->src_fmt.plane_fmt[0].sizeimage; > > > if (*num_planes == 2) > > > size[1] = ctx->src_fmt.plane_fmt[1].sizeimage; > > > break; > > > } > > > > > > if (*num_planes > 2) > > > return -EINVAL; > > > if (*num_planes == 0) { > > > if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) { > > > sizes[0] = ctx->dst_fmt.buffersize; > > > *num_planes = 1; > > > return 0; > > > } > > > > > > *num_planes = ctx->src_fmt.num_planes; > > > for (plane = 0; plane < *num_planes; plane++) > > > sizes[plane] = ctx->src_fmt.plane_fmt[plane].sizeimage; > > > return 0; > > > } > > > > > > for (plane = 0; plane < *num_planes; plane++) { > > > if(sizes[plane] < size[plane]) > > > return -EINVAL; > > > } > > > return 0; > > > } > > > > > > > Looks good, thanks! > > > > > > > } > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > > +{ > > > > > > > > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > > + struct vb2_buffer *vb; > > > > > > > > > > > > > > > > How do we guarantee here that the hardware isn't still accessing the buffers > > > > > > > > removed below? > > > > > > > > > > > > > > > Maybe we can check the driver state flag and aborting the unfinished > > > > > > > jobs? > > > > > > > (fd_hw->state == FD_ENQ) > > > > > > > > > > > > > > > > > > > Yes, we need to either cancel or wait for the currently processing > > > > > > job. It depends on hardware capabilities, but cancelling is generally > > > > > > preferred for the lower latency. > > > > > > > > > > > Ok, it the state is ENQ, then we can disable the FD hw by controlling > > > > > the registers. > > > > > > > > > > for example: > > > > > writel(0x0, fd->fd_base + FD_HW_ENABLE); > > > > > writel(0x0, fd->fd_base + FD_INT_EN); > > > > > > > > > > > > > What's exactly the effect of writing 0 to FD_HW_ENABLE? > > > > > > > Sorry, my last reply didn't solve the question, > > > we should implement a mtk_fd_job_abort() for v4l2_m2m_ops(). > > > > > > which is able to readl_poll_timeout_atomic() > > > and check the HW busy bits in the register FD_INT_EN; > > > > > > if they are not cleared until timeout, we could handle the last > > > processing job. > > > Otherwise, the FD irq handler should have handled the last processing > > > job and we could continue the stop_streaming(). > > > > > > For job_abort(): > > > static void mtk_fd_job_abort(void *priv) > > > { > > > struct mtk_fd_ctx *ctx = priv; > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > u32 val; > > > u32 ret; > > > > > > ret = readl_poll_timeout_atomic(fd->fd_base + MTK_FD_REG_OFFSET_INT_EN, > > > val, > > > (val & MTK_FD_HW_BUSY_MASK) == > > > MTK_FD_HW_STATE_IS_BUSY, > > > USEC_PER_MSEC, MTK_FD_STOP_HW_TIMEOUT); > > > > Hmm, would it be possible to avoid the busy wait by having a > > completion that could be signalled from the interrupt handler? > > > > Best regards, > > Tomasz > > I suppose that would be wakeup a wait queue in the interrupt handler, > the the wait_event_interrupt_timeout() will be used in here and system > suspend e.g. mtk_fd_suspend(). Yes, that should work. > Or do you suggest to wait_event_interrupt_timeout() every frame in the > mtk_fd_ipi_handler()? Nope, we shouldn't need that. > I think maybe the readl_poll_timeout_atomic would be good enough. > Not really. Busy waiting should be avoided as much as possible. What's the point of entering suspend if you end up burning the power by spinning the CPU for some milliseconds? > > One more thing, for the mtk_fd_video_device_register() > Sorry that I would need to use intermediate variable here since the 80 > columns check. > > function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS; > ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function); Why not just make it like this: ret = v4l2_m2m_register_media_controller(m2m_dev, MEDIA_ENT_F_PROC_VIDEO_STATISTICS); The above line is aligned using tabs so that its end is as close to the 80 character boundary as possible. Best regards, Tomasz