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=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 96DBCC433FF for ; Thu, 15 Aug 2019 10:46:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 716F820656 for ; Thu, 15 Aug 2019 10:46:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730834AbfHOKqx (ORCPT ); Thu, 15 Aug 2019 06:46:53 -0400 Received: from mga18.intel.com ([134.134.136.126]:62125 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726027AbfHOKqx (ORCPT ); Thu, 15 Aug 2019 06:46:53 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2019 03:46:27 -0700 X-IronPort-AV: E=Sophos;i="5.64,389,1559545200"; d="scan'208";a="179338295" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2019 03:46:22 -0700 Received: by paasikivi.fi.intel.com (Postfix, from userid 1000) id 091E520F68; Thu, 15 Aug 2019 13:45:16 +0300 (EEST) Date: Thu, 15 Aug 2019 13:45:15 +0300 From: Sakari Ailus To: Tomasz Figa Cc: Helen Koike , "open list:ARM/Rockchip SoC..." , devicetree@vger.kernel.org, Eddie Cai , Mauro Carvalho Chehab , Heiko =?iso-8859-1?Q?St=FCbner?= , Chen Jacob , Jeffy , =?utf-8?B?6ZKf5Lul5bSH?= , Linux Kernel Mailing List , Hans Verkuil , Laurent Pinchart , kernel@collabora.com, Ezequiel Garcia , Linux Media Mailing List , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , Shunqian Zheng , Jacob Chen , Allon Huang Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver Message-ID: <20190815104515.GO6133@paasikivi.fi.intel.com> References: <20190730184256.30338-1-helen.koike@collabora.com> <20190730184256.30338-6-helen.koike@collabora.com> <20190808091406.GQ21370@paasikivi.fi.intel.com> <20190815082422.GM6133@paasikivi.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Thu, Aug 15, 2019 at 07:29:59PM +0900, Tomasz Figa wrote: > On Thu, Aug 15, 2019 at 5:25 PM Sakari Ailus > wrote: > > > > Hi Helen, > > > > On Wed, Aug 14, 2019 at 09:58:05PM -0300, Helen Koike wrote: > > > > ... > > > > > >> +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd, > > > >> + struct v4l2_subdev_pad_config *cfg, > > > >> + struct v4l2_subdev_format *fmt) > > > >> +{ > > > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd); > > > >> + struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev; > > > >> + struct v4l2_mbus_framefmt *mf = &fmt->format; > > > >> + > > > > > > > > Note that for sub-device nodes, the driver is itself responsible for > > > > serialising the access to its data structures. > > > > > > But looking at subdev_do_ioctl_lock(), it seems that it serializes the > > > ioctl calls for subdevs, no? Or I'm misunderstanding something (which is > > > most probably) ? > > > > Good question. I had missed this change --- subdev_do_ioctl_lock() is > > relatively new. But setting that lock is still not possible as the struct > > is allocated in the framework and the device is registered before the > > driver gets hold of it. It's a good idea to provide the same serialisation > > for subdevs as well. > > > > I'll get back to this later. > > > > ... > > > > > >> +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on) > > > > > > > > If you support runtime PM, you shouldn't implement the s_power op. > > > > > > Is is ok to completly remove the usage of runtime PM then? > > > Like this http://ix.io/1RJb ? > > > > Please use runtime PM instead. In the long run we should get rid of the > > s_power op. Drivers themselves know better when the hardware they control > > should be powered on or off. > > > > One also needs to use runtime PM to handle power domains and power > dependencies on auxiliary devices, e.g. IOMMU. > > > > > > > tbh I'm not that familar with runtime PM and I'm not sure what is the > > > difference of it and using s_power op (and Documentation/power/runtime_pm.rst > > > is not being that helpful tbh). > > > > You can find a simple example e.g. in > > drivers/media/platform/atmel/atmel-isi.c . > > > > > > > > > > > > > You'll still need to call s_power on external subdevs though. > > > > > > > >> +{ > > > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd); > > > >> + int ret; > > > >> + > > > >> + v4l2_dbg(1, rkisp1_debug, &isp_dev->v4l2_dev, "s_power: %d\n", on); > > > >> + > > > >> + if (on) { > > > >> + ret = pm_runtime_get_sync(isp_dev->dev); > > > > > > If this is not ok to remove suport for runtime PM, then where should I put > > > the call to pm_runtime_get_sync() if not in this s_power op ? > > > > Basically the runtime_resume and runtime_suspend callbacks are where the > > device power state changes are implemented, and pm_runtime_get_sync and > > pm_runtime_put are how the driver controls the power state. > > > > So you no longer need the s_power() op at all. The op needs to be called on > > the pipeline however, as there are drivers that still use it. > > > > For this driver, I suppose we would _get_sync() when we start > streaming (in the hardware, i.e. we want the ISP to start capturing > frames) and _put() when we stop and the driver shouldn't perform any > access to the hardware when the streaming is not active. Agreed. -- Sakari Ailus sakari.ailus@linux.intel.com