From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexandre Courbot Subject: Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver Date: Thu, 20 Jun 2019 13:48:15 +0900 Message-ID: References: <20190516032332.56844-1-daoyuan.huang@mediatek.com> <20190516032332.56844-5-daoyuan.huang@mediatek.com> <20190604112039.GA12168@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190604112039.GA12168@chromium.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Tomasz Figa Cc: devicetree@vger.kernel.org, Sean.Cheng@mediatek.com, Laurent Pinchart , Rynn.Wu@mediatek.com, christie.yu@mediatek.com, srv_heupstream@mediatek.com, Daoyuan Huang , holmes.chiou@mediatek.com, Jerry-ch.Chen@mediatek.com, jungo.lin@mediatek.com, Sj Huang , yuzhao@chromium.org, Hans Verkuil , Ping-Hsun Wu , zwisler@chromium.org, frederic.chen@mediatek.com, matthias.bgg@gmail.com, linux-mediatek@lists.infradead.org, Mauro Carvalho Chehab , linux-arm-kernel@lists.infradead.org, Linux Media Mailing List List-Id: devicetree@vger.kernel.org On Tue, Jun 4, 2019 at 8:20 PM Tomasz Figa wrote: > > + > > + ret = mdp_vpu_get_locked(mdp); > > + if (ret < 0) > > + goto err_load_vpu; > > This shouldn't happen in open(), but rather the latest possible point in > time. If one needs to keep the VPU running for the time of streaming, then > it should be start_streaming. If one can safely turn the VPU off if there is > no frame queued for long time, it should be just in m2m job_run. > > Generally the userspace should be able to > just open an m2m device for querying it, without any side effects like > actually powering on the hardware or grabbing a hardware instance (which > could block some other processes, trying to grab one too). OTOH looking at the code of mdp_vpu_get_locked(), we do the whole rproc_boot and VPU init procedure if we were the only user. So I can understand we want to avoid doing this too often. Maybe mdp_vpu_get_locked() can be reorganized in a better way. I feel like the call to mdp_vpu_register() should be done in probe, and maybe we can use runtime PM (with a reasonable timeout) to control the rproc and VPU init? > > > + > > + mutex_unlock(&mdp->m2m_lock); > > + > > + mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id); > > + > > + return 0; > > + > > +err_load_vpu: > > + mdp_frameparam_release(ctx->curr_param); > > +err_param_init: > > + v4l2_m2m_ctx_release(ctx->m2m_ctx); > > +err_m2m_ctx: > > + v4l2_ctrl_handler_free(&ctx->ctrl_handler); > > + v4l2_fh_del(&ctx->fh); > > +err_ctrls_create: > > + v4l2_fh_exit(&ctx->fh); > > + mutex_unlock(&mdp->m2m_lock); > > +err_lock: > > Incorrect naming of all the error labels here. > > > + kfree(ctx); > > + > > + return ret; > > +} > [snip] > > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f, > > + u32 mdp_color) > > +{ > > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > > + > > + if (MDP_COLOR_IS_RGB(mdp_color)) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + > > + switch (pix_mp->colorspace) { > > + case V4L2_COLORSPACE_JPEG: > > + return MDP_YCBCR_PROFILE_JPEG; > > + case V4L2_COLORSPACE_REC709: > > + case V4L2_COLORSPACE_DCI_P3: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT709; > > + return MDP_YCBCR_PROFILE_BT709; > > + case V4L2_COLORSPACE_BT2020: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT2020; > > + return MDP_YCBCR_PROFILE_BT2020; > > + } > > + /* V4L2_COLORSPACE_SRGB or else */ > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + return MDP_YCBCR_PROFILE_BT601; > > Putting this under the default clause of the switch statement would be > cleaner and the comment wouldn't be needed. > > [snip] > > +struct mdp_frameparam *mdp_frameparam_init(void) > > +{ > > + struct mdp_frameparam *param; > > + struct mdp_frame *frame; > > + > > + param = kzalloc(sizeof(*param), GFP_KERNEL); > > + if (!param) > > + return ERR_PTR(-ENOMEM); > > We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then > wouldn't need any dynamic allocation here anymore. And as a side effect, the > function could be just made void, because it couldn't fail. > > > + > > + INIT_LIST_HEAD(¶m->list); > > + mutex_init(¶m->lock); > > + param->state = 0; > > + param->limit = &mdp_def_limit; > > + param->type = MDP_STREAM_TYPE_UNKNOWN; > > We always seem to use MDP_STREAM_TYPE_BITBLT in this driver. > > > + param->frame_no = 0; > > No need for explicit initialization to 0. > > Best regards, > Tomasz > 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.8 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=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 E7A4EC43613 for ; Thu, 20 Jun 2019 04:48:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B1A4F214AF for ; Thu, 20 Jun 2019 04:48:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Yl5NlZzQ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725939AbfFTEsa (ORCPT ); Thu, 20 Jun 2019 00:48:30 -0400 Received: from mail-oi1-f193.google.com ([209.85.167.193]:35203 "EHLO mail-oi1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725872AbfFTEsa (ORCPT ); Thu, 20 Jun 2019 00:48:30 -0400 Received: by mail-oi1-f193.google.com with SMTP id a127so1185668oii.2 for ; Wed, 19 Jun 2019 21:48:29 -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=AJXIcsGFO5lMWZGuEosbW/Dq3JALel5VCKCAbEtYs6Y=; b=Yl5NlZzQPUN5Tci0Gf50JsXP4Oo8dpYdihclnZeT66LEb3dxzRiJhHirLoaUqRkGN3 nd7drf2dBhfS2HA3AMSoOGDADxhZ4nLpQMvB1T2hAahMGJBa0/7+xUJ7aJ0B4ypcW6e5 BSNJ2PwbPrdk+BCyPhpWGOWiUVhuLmDA6DGw0= 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=AJXIcsGFO5lMWZGuEosbW/Dq3JALel5VCKCAbEtYs6Y=; b=pJgu3OUY0LtqBLzw0xKyV4MQFETsCidzlqoQCiM1hqrcYMEb5MZojX8UqI8B7XhIex psJ3KJUUHdqitmC99PRXFVZ+vSw1hr0mDH3zU06M103cI4itCz0Xz9AfM5NJkGSBvNwq TNCQVmgIf3nIquq0x7s9qyxM3P3p84aUl3YNNYT5yTS6M8Zdy+QMBg/isscuciuwDNMa xmRtjsaOjsDRfsElrNgQsCj99mjEZRL++CHwhrjb+wMFC8B+lZCpORyosrsRDK68OR2z EltMpm9Y+yGL2XK4IVhyi85jgAHR6YkS1L8b4zjo/tikxU1NS4Jy2EcIL2BxGSXb1u0n NU5g== X-Gm-Message-State: APjAAAVXLpiglqfIv56uvsrmYnmPV7TFKWYwLDwBqKF9sPR6I57PvE6q +GlXeB9UFLskFHPwOqjIF+lrDBkwRl4/iw== X-Google-Smtp-Source: APXvYqwetTNMEiq1hZAXxNpJ9BKuoNmywZ/+39KqeA/nrr3YmlkopZzS7WVHfqHct22/KZE5CTaZGA== X-Received: by 2002:aca:4e16:: with SMTP id c22mr4448745oib.161.1561006108659; Wed, 19 Jun 2019 21:48:28 -0700 (PDT) Received: from mail-ot1-f41.google.com (mail-ot1-f41.google.com. [209.85.210.41]) by smtp.gmail.com with ESMTPSA id c12sm7392904otn.68.2019.06.19.21.48.27 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jun 2019 21:48:27 -0700 (PDT) Received: by mail-ot1-f41.google.com with SMTP id n5so1467912otk.1 for ; Wed, 19 Jun 2019 21:48:27 -0700 (PDT) X-Received: by 2002:a9d:3ee:: with SMTP id f101mr10192019otf.311.1561006107360; Wed, 19 Jun 2019 21:48:27 -0700 (PDT) MIME-Version: 1.0 References: <20190516032332.56844-1-daoyuan.huang@mediatek.com> <20190516032332.56844-5-daoyuan.huang@mediatek.com> <20190604112039.GA12168@chromium.org> In-Reply-To: <20190604112039.GA12168@chromium.org> From: Alexandre Courbot Date: Thu, 20 Jun 2019 13:48:15 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver To: Tomasz Figa Cc: Daoyuan Huang , Hans Verkuil , Laurent Pinchart , matthias.bgg@gmail.com, Mauro Carvalho Chehab , yuzhao@chromium.org, zwisler@chromium.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, Sean.Cheng@mediatek.com, Sj Huang , christie.yu@mediatek.com, holmes.chiou@mediatek.com, frederic.chen@mediatek.com, Jerry-ch.Chen@mediatek.com, jungo.lin@mediatek.com, Rynn.Wu@mediatek.com, Linux Media Mailing List , srv_heupstream@mediatek.com, devicetree@vger.kernel.org, Ping-Hsun Wu 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, Jun 4, 2019 at 8:20 PM Tomasz Figa wrote: > > + > > + ret = mdp_vpu_get_locked(mdp); > > + if (ret < 0) > > + goto err_load_vpu; > > This shouldn't happen in open(), but rather the latest possible point in > time. If one needs to keep the VPU running for the time of streaming, then > it should be start_streaming. If one can safely turn the VPU off if there is > no frame queued for long time, it should be just in m2m job_run. > > Generally the userspace should be able to > just open an m2m device for querying it, without any side effects like > actually powering on the hardware or grabbing a hardware instance (which > could block some other processes, trying to grab one too). OTOH looking at the code of mdp_vpu_get_locked(), we do the whole rproc_boot and VPU init procedure if we were the only user. So I can understand we want to avoid doing this too often. Maybe mdp_vpu_get_locked() can be reorganized in a better way. I feel like the call to mdp_vpu_register() should be done in probe, and maybe we can use runtime PM (with a reasonable timeout) to control the rproc and VPU init? > > > + > > + mutex_unlock(&mdp->m2m_lock); > > + > > + mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id); > > + > > + return 0; > > + > > +err_load_vpu: > > + mdp_frameparam_release(ctx->curr_param); > > +err_param_init: > > + v4l2_m2m_ctx_release(ctx->m2m_ctx); > > +err_m2m_ctx: > > + v4l2_ctrl_handler_free(&ctx->ctrl_handler); > > + v4l2_fh_del(&ctx->fh); > > +err_ctrls_create: > > + v4l2_fh_exit(&ctx->fh); > > + mutex_unlock(&mdp->m2m_lock); > > +err_lock: > > Incorrect naming of all the error labels here. > > > + kfree(ctx); > > + > > + return ret; > > +} > [snip] > > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f, > > + u32 mdp_color) > > +{ > > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > > + > > + if (MDP_COLOR_IS_RGB(mdp_color)) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + > > + switch (pix_mp->colorspace) { > > + case V4L2_COLORSPACE_JPEG: > > + return MDP_YCBCR_PROFILE_JPEG; > > + case V4L2_COLORSPACE_REC709: > > + case V4L2_COLORSPACE_DCI_P3: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT709; > > + return MDP_YCBCR_PROFILE_BT709; > > + case V4L2_COLORSPACE_BT2020: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT2020; > > + return MDP_YCBCR_PROFILE_BT2020; > > + } > > + /* V4L2_COLORSPACE_SRGB or else */ > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + return MDP_YCBCR_PROFILE_BT601; > > Putting this under the default clause of the switch statement would be > cleaner and the comment wouldn't be needed. > > [snip] > > +struct mdp_frameparam *mdp_frameparam_init(void) > > +{ > > + struct mdp_frameparam *param; > > + struct mdp_frame *frame; > > + > > + param = kzalloc(sizeof(*param), GFP_KERNEL); > > + if (!param) > > + return ERR_PTR(-ENOMEM); > > We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then > wouldn't need any dynamic allocation here anymore. And as a side effect, the > function could be just made void, because it couldn't fail. > > > + > > + INIT_LIST_HEAD(¶m->list); > > + mutex_init(¶m->lock); > > + param->state = 0; > > + param->limit = &mdp_def_limit; > > + param->type = MDP_STREAM_TYPE_UNKNOWN; > > We always seem to use MDP_STREAM_TYPE_BITBLT in this driver. > > > + param->frame_no = 0; > > No need for explicit initialization to 0. > > Best regards, > Tomasz > 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.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 EA751C43613 for ; Thu, 20 Jun 2019 04:48:39 +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 AFEF82147A for ; Thu, 20 Jun 2019 04:48:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="YPg5xg23"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Yl5NlZzQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AFEF82147A 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:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ypnhf2rkuNNOTsAZEgT5/raeSWPRBQA0JVuZKNg2KKI=; b=YPg5xg23kufpWO cuenSj2UZDs6qSCGp7UoJsefp392/xET5Cg5Zi0L6btXdhn/nGCIjssqBXiKCQI4A79OLp1j8fyxW yLGLy6P6Tpe80hM9jQmNcok5tSVBbVLi222cFRg+rT5ZtH8bKtrF38CtwXDW3sA7zZq09uMZAj4bd kZop2VZHx9jbGFjHMfHPPszHlTxR9nE2jv3xWpobhTV1nnyS5CWH5yF0NI2E1Cp8sbYzsBtrr3/I3 x/uNyQIT98adHl4gpxek1WBj9aIB1YOBU1GdKscgEUhmXOSnL7if9Ztn1B8/2EmPE6ss8GTTPLKIg Uv0nPOdFB+LBRkZl0YJw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hdozy-0000y6-QH; Thu, 20 Jun 2019 04:48:34 +0000 Received: from mail-oi1-x241.google.com ([2607:f8b0:4864:20::241]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hdozv-0000x4-F6 for linux-arm-kernel@lists.infradead.org; Thu, 20 Jun 2019 04:48:32 +0000 Received: by mail-oi1-x241.google.com with SMTP id a128so1200030oib.1 for ; Wed, 19 Jun 2019 21:48:30 -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=AJXIcsGFO5lMWZGuEosbW/Dq3JALel5VCKCAbEtYs6Y=; b=Yl5NlZzQPUN5Tci0Gf50JsXP4Oo8dpYdihclnZeT66LEb3dxzRiJhHirLoaUqRkGN3 nd7drf2dBhfS2HA3AMSoOGDADxhZ4nLpQMvB1T2hAahMGJBa0/7+xUJ7aJ0B4ypcW6e5 BSNJ2PwbPrdk+BCyPhpWGOWiUVhuLmDA6DGw0= 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=AJXIcsGFO5lMWZGuEosbW/Dq3JALel5VCKCAbEtYs6Y=; b=MFo8RmZBG0Cfp99ju4oSBsET1z605wvlEw6lenBJCN/e8TdGZNjtYyUiTZg8OeJ1wf 6ePwq13mEgAWu/J1aoHuCWZTM/G3+QGrL+4+mSJ8qMUXBUCdzesEG+qEcEb6Uj2u2TqE Mov5KCRL4/FMfHD6+L8OjPwoUailSppkAuuik8DfEj4ycz0SV0nbtOZ2MXwZywGymDN8 hC/2IYYA67c7x/q+Z+NdSFRihYRYGDmXLAzvQ0afQD1k9aW5OLzp82833PH3hyXvHDwi d8OG1iQ7Pj23SLWKw+d7qpO0vifu4zoD4TTpxdlDzIlamFv2Cr40DkPc/OUNj72q9x8r 0GXA== X-Gm-Message-State: APjAAAU3xSnnmeeBYlP1aNOn6mcusIOnKTbZ8bA91rqBpGdcj1u8lC8s Ed74h47RXvgYjVnDJ/nlfmw4TaywYzLgCQ== X-Google-Smtp-Source: APXvYqxjbxLKdDwNRXuuFnbImBbeAtiMa1kDq/lX10i5rVt4L4jsmdHqbtQVplmVaLCjfR0v9bviLQ== X-Received: by 2002:aca:490d:: with SMTP id w13mr4878291oia.8.1561006108922; Wed, 19 Jun 2019 21:48:28 -0700 (PDT) Received: from mail-ot1-f46.google.com (mail-ot1-f46.google.com. [209.85.210.46]) by smtp.gmail.com with ESMTPSA id b2sm7367193otf.48.2019.06.19.21.48.27 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jun 2019 21:48:28 -0700 (PDT) Received: by mail-ot1-f46.google.com with SMTP id j19so1465379otq.2 for ; Wed, 19 Jun 2019 21:48:27 -0700 (PDT) X-Received: by 2002:a9d:3ee:: with SMTP id f101mr10192019otf.311.1561006107360; Wed, 19 Jun 2019 21:48:27 -0700 (PDT) MIME-Version: 1.0 References: <20190516032332.56844-1-daoyuan.huang@mediatek.com> <20190516032332.56844-5-daoyuan.huang@mediatek.com> <20190604112039.GA12168@chromium.org> In-Reply-To: <20190604112039.GA12168@chromium.org> From: Alexandre Courbot Date: Thu, 20 Jun 2019 13:48:15 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver To: Tomasz Figa X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190619_214831_508893_7EC9946A X-CRM114-Status: GOOD ( 20.78 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Sean.Cheng@mediatek.com, Laurent Pinchart , Rynn.Wu@mediatek.com, christie.yu@mediatek.com, srv_heupstream@mediatek.com, Daoyuan Huang , holmes.chiou@mediatek.com, Jerry-ch.Chen@mediatek.com, jungo.lin@mediatek.com, Sj Huang , yuzhao@chromium.org, Hans Verkuil , Ping-Hsun Wu , zwisler@chromium.org, frederic.chen@mediatek.com, matthias.bgg@gmail.com, linux-mediatek@lists.infradead.org, Mauro Carvalho Chehab , linux-arm-kernel@lists.infradead.org, Linux Media Mailing List 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 On Tue, Jun 4, 2019 at 8:20 PM Tomasz Figa wrote: > > + > > + ret = mdp_vpu_get_locked(mdp); > > + if (ret < 0) > > + goto err_load_vpu; > > This shouldn't happen in open(), but rather the latest possible point in > time. If one needs to keep the VPU running for the time of streaming, then > it should be start_streaming. If one can safely turn the VPU off if there is > no frame queued for long time, it should be just in m2m job_run. > > Generally the userspace should be able to > just open an m2m device for querying it, without any side effects like > actually powering on the hardware or grabbing a hardware instance (which > could block some other processes, trying to grab one too). OTOH looking at the code of mdp_vpu_get_locked(), we do the whole rproc_boot and VPU init procedure if we were the only user. So I can understand we want to avoid doing this too often. Maybe mdp_vpu_get_locked() can be reorganized in a better way. I feel like the call to mdp_vpu_register() should be done in probe, and maybe we can use runtime PM (with a reasonable timeout) to control the rproc and VPU init? > > > + > > + mutex_unlock(&mdp->m2m_lock); > > + > > + mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id); > > + > > + return 0; > > + > > +err_load_vpu: > > + mdp_frameparam_release(ctx->curr_param); > > +err_param_init: > > + v4l2_m2m_ctx_release(ctx->m2m_ctx); > > +err_m2m_ctx: > > + v4l2_ctrl_handler_free(&ctx->ctrl_handler); > > + v4l2_fh_del(&ctx->fh); > > +err_ctrls_create: > > + v4l2_fh_exit(&ctx->fh); > > + mutex_unlock(&mdp->m2m_lock); > > +err_lock: > > Incorrect naming of all the error labels here. > > > + kfree(ctx); > > + > > + return ret; > > +} > [snip] > > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f, > > + u32 mdp_color) > > +{ > > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > > + > > + if (MDP_COLOR_IS_RGB(mdp_color)) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + > > + switch (pix_mp->colorspace) { > > + case V4L2_COLORSPACE_JPEG: > > + return MDP_YCBCR_PROFILE_JPEG; > > + case V4L2_COLORSPACE_REC709: > > + case V4L2_COLORSPACE_DCI_P3: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT709; > > + return MDP_YCBCR_PROFILE_BT709; > > + case V4L2_COLORSPACE_BT2020: > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT2020; > > + return MDP_YCBCR_PROFILE_BT2020; > > + } > > + /* V4L2_COLORSPACE_SRGB or else */ > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + return MDP_YCBCR_PROFILE_BT601; > > Putting this under the default clause of the switch statement would be > cleaner and the comment wouldn't be needed. > > [snip] > > +struct mdp_frameparam *mdp_frameparam_init(void) > > +{ > > + struct mdp_frameparam *param; > > + struct mdp_frame *frame; > > + > > + param = kzalloc(sizeof(*param), GFP_KERNEL); > > + if (!param) > > + return ERR_PTR(-ENOMEM); > > We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then > wouldn't need any dynamic allocation here anymore. And as a side effect, the > function could be just made void, because it couldn't fail. > > > + > > + INIT_LIST_HEAD(¶m->list); > > + mutex_init(¶m->lock); > > + param->state = 0; > > + param->limit = &mdp_def_limit; > > + param->type = MDP_STREAM_TYPE_UNKNOWN; > > We always seem to use MDP_STREAM_TYPE_BITBLT in this driver. > > > + param->frame_no = 0; > > No need for explicit initialization to 0. > > Best regards, > Tomasz > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel