Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp47530ybl; Tue, 28 Jan 2020 18:12:57 -0800 (PST) X-Google-Smtp-Source: APXvYqx9PH8hyaqUSxdFs1b+2WvCb7I4s55wRlj1zrmuGI+1QG2wMDbN32Q7I3f9pjJInem3f40Z X-Received: by 2002:aca:1c1a:: with SMTP id c26mr5036611oic.75.1580263977180; Tue, 28 Jan 2020 18:12:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1580263977; cv=none; d=google.com; s=arc-20160816; b=LVgSzk+d1Xp6YlO++U5jVgq0U9/pyBWlCbnzIzduFhRrrWh/p2Gey8CYukuoPp0Vrf ZXItnYYG5G1941rfDmU/FlPQCZcnSNUvyIy9Drrv5xW1nhtwwgLs4k0Ee5FLqTIsb/Me Ys8KAhQiRTH0EXz21GBDzunaBv4EhQe1ua7OZ7PN8fVFy0a7mq1Pz4hpqhQtFF/oXJAL 7pbW9oclajgH86bf8m7EO4bx8x2TfQ+V5v0schCGR3Y5q1VYofOdCmka20Yh6xRgzns/ aSqAJ9YMi5MICMIu+M23rN7p7q8NAtwDyxaNe08wrZlu8tUjcTp726A7oHgRW3AlB98C VtJg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:dkim-signature:content-language :content-transfer-encoding:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=mjgtQfcvqfc+xgzg1haYqA6j+g6qFySaH3Lr17t/xEA=; b=LepIAPfOuooaMhRhqYysKia37B0glNQo5tNvZLnayIv7z/5fFNKGeb5mVHvbcMSHLH U0FadotnVLyUuP0HlqMt7mTyAKdQRhp56Gdh2mI2aQDx9F+zqrjt2uOTtPbGNKwnXVS9 RcDCOT/jqYwZv7DyJTWJXSxNEH83caqgE/twm3oN/ibWQZlEsVwpx+cOZRKA87tZt5S1 87zxWLDc0Vxk3PGRkKtZv/NnT3rhIW598cyFdSrofFhAE2MqrHPQRms55zwgFDiQrN+A MzEjqlJRqWbP67H/YRK1EwpucICcA0x5in8UkeDhtSwjK7rkgBbXoE2WCNJ1YBbyfLnz tsdA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=q+Zc4DvP; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id v20si380460oia.159.2020.01.28.18.12.44; Tue, 28 Jan 2020 18:12:57 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=q+Zc4DvP; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726504AbgA2CLv (ORCPT + 99 others); Tue, 28 Jan 2020 21:11:51 -0500 Received: from hqnvemgate25.nvidia.com ([216.228.121.64]:8744 "EHLO hqnvemgate25.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726391AbgA2CLu (ORCPT ); Tue, 28 Jan 2020 21:11:50 -0500 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqnvemgate25.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Tue, 28 Jan 2020 18:11:29 -0800 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Tue, 28 Jan 2020 18:11:49 -0800 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Tue, 28 Jan 2020 18:11:49 -0800 Received: from [10.2.164.115] (10.124.1.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Wed, 29 Jan 2020 02:11:48 +0000 Subject: Re: [RFC PATCH v1 4/5] media: tegra: Add Tegra Video input driver for Tegra210 To: Helen Koike , , , , CC: , , , , References: <1580235801-4129-1-git-send-email-skomatineni@nvidia.com> <1580235801-4129-5-git-send-email-skomatineni@nvidia.com> <3cdea635-a9ca-7b9c-3c99-8f489f4d669a@collabora.com> <162488d0-4e74-963a-3366-e4c1f7cf04ca@nvidia.com> <017ca95e-7dd3-2d04-8d84-9047ac4e548b@nvidia.com> <655b9a64-10d7-3fd3-f443-babf33e67b62@collabora.com> From: Sowjanya Komatineni Message-ID: <7265b661-de5a-b0f0-bcdc-1a1d2c03fe57@nvidia.com> Date: Tue, 28 Jan 2020 18:11:47 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <655b9a64-10d7-3fd3-f443-babf33e67b62@collabora.com> X-Originating-IP: [10.124.1.5] X-ClientProxiedBy: HQMAIL111.nvidia.com (172.20.187.18) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1580263889; bh=mjgtQfcvqfc+xgzg1haYqA6j+g6qFySaH3Lr17t/xEA=; h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date: User-Agent:MIME-Version:In-Reply-To:X-Originating-IP: X-ClientProxiedBy:Content-Type:Content-Transfer-Encoding: Content-Language; b=q+Zc4DvP5yH4F1pa3nwVqBJYZV3oLINg5P51V4C4+2FZ8lZRWAMad9aMZ3kUMo5R4 vzAs1E67p1Bog6JuAYZFXXIZdrSfGkOMVarfTel/COiH0vMfakMwnCwxquM8OwRRAD ajlOZOXEUQ8jlfcaY7SqwPAjZu/0b3LTj1ab9T5lR/b8KTpSYTGi9I4bf8GlFL7eDv MKOI52jp4ZvX50oC6/jMiAnug1ZkAciI7gWzTUQt87svcTm8CZmlL0fbombLVp1dxr 6XcqAhCGqHmQVSslA+OUk5sR1uSJCP0bZatD8lqtZuXYTiGCg5zrspY4er8CDZxmnP VMh7DRylDV5mA== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/28/20 5:05 PM, Helen Koike wrote: > External email: Use caution opening links or attachments > > > On 1/28/20 10:49 PM, Sowjanya Komatineni wrote: >> On 1/28/20 2:13 PM, Sowjanya Komatineni wrote: >>> On 1/28/20 1:45 PM, Helen Koike wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> Hi Sowjanya, >>>> >>>> I just took a really quick look, I didn't check the driver in deep, so just some small comments below. >>>> >>>> On 1/28/20 4:23 PM, Sowjanya Komatineni wrote: >>>>> Tegra210 contains a powerful Video Input (VI) hardware controller >>>>> which can support up to 6 MIPI CSI camera sensors. >>>>> >>>>> Each Tegra CSI port can be one-to-one mapped to VI channel and can >>>>> capture from an external camera sensor connected to CSI or from >>>>> built-in test pattern generator. >>>>> >>>>> Tegra210 supports built-in test pattern generator from CSI to VI. >>>>> >>>>> This patch adds a V4L2 media controller and capture driver support >>>>> for Tegra210 built-in CSI to VI test pattern generator. >>>>> >>>>> Signed-off-by: Sowjanya Komatineni >>>> Could you send us the output of media-ctl --print-dot ? So we can view the media topology easily? >>> root@tegra-ubuntu:/home/ubuntu# ./media-ctl --print-dot >>> digraph board { >>> rankdir=TB >>> n00000001 [label="54080000.vi-output-0\n/dev/video0", shape=box, style=filled, fillcolor=yellow] >>> n00000005 [label="54080000.vi-output-1\n/dev/video1", shape=box, style=filled, fillcolor=yellow] >>> n00000009 [label="54080000.vi-output-2\n/dev/video2", shape=box, style=filled, fillcolor=yellow] >>> n0000000d [label="54080000.vi-output-3\n/dev/video3", shape=box, style=filled, fillcolor=yellow] >>> n00000011 [label="54080000.vi-output-4\n/dev/video4", shape=box, style=filled, fillcolor=yellow] >>> n00000015 [label="54080000.vi-output-5\n/dev/video5", shape=box, style=filled, fillcolor=yellow] >>> n00000019 [label="{{} | tpg-0 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n00000019:port0 -> n00000001 >>> n0000001d [label="{{} | tpg-1 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n0000001d:port0 -> n00000005 >>> n00000021 [label="{{} | tpg-2 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n00000021:port0 -> n00000009 >>> n00000025 [label="{{} | tpg-3 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n00000025:port0 -> n0000000d >>> n00000029 [label="{{} | tpg-4 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n00000029:port0 -> n00000011 >>> n0000002d [label="{{} | tpg-5 | { 0}}", shape=Mrecord, style=filled, fillcolor=green] >>> n0000002d:port0 -> n00000015 >>> } >>> >>>>> --- diff --git a/drivers/staging/media/tegra/host1x-video.h b/drivers/staging/media/tegra/host1x-video.h >>>>> new file mode 100644 >>>>> index 000000000000..84d28e6f4362 >>>>> --- /dev/null >>>>> +++ b/drivers/staging/media/tegra/host1x-video.h >>>>> @@ -0,0 +1,33 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>> +/* >>>>> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved. >>>>> + */ >>>>> + >>>>> +#ifndef HOST1X_VIDEO_H >>>>> +#define HOST1X_VIDEO_H 1 >>>>> + >>>>> +#include >>>>> + >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> + >>>>> +#include "tegra-vi.h" >>>>> +#include "csi.h" >>>>> + >>>>> +struct tegra_camera { >>>>> + struct v4l2_device v4l2_dev; >>>>> + struct media_device media_dev; >>>>> + struct device *dev; >>>> You can use cam->media_dev.dev instead of having this pointer. >>>> >> Will fix in v2 >>>>> + struct tegra_vi *vi; >>>>> + struct tegra_csi_device *csi; >>>>> +}; >>>>> + >>>>> + >>>>> +#define to_tegra_channel(vdev) \ >>>>> + container_of(vdev, struct tegra_channel, video) >>>> Why not inline instead of define. Inlines has the advantage of checking types. >> Will change in v2 >>>>> +static int __tegra_channel_try_format(struct tegra_channel *chan, >>>>> + struct v4l2_pix_format *pix, >>>>> + const struct tegra_video_format **vfmt) >>>>> +{ >>>>> + const struct tegra_video_format *fmt_info; >>>>> + struct v4l2_subdev *subdev; >>>>> + struct v4l2_subdev_format fmt; >>>>> + struct v4l2_subdev_pad_config *pad_cfg; >>>>> + >>>>> + subdev = tegra_channel_get_remote_subdev(chan); >>>>> + pad_cfg = v4l2_subdev_alloc_pad_config(subdev); >>>>> + if (!pad_cfg) >>>>> + return -ENOMEM; >>>>> + >>>>> + /* >>>>> + * Retrieve format information and select the default format if the >>>>> + * requested format isn't supported. >>>>> + */ >>>>> + fmt_info = tegra_core_get_format_by_fourcc(chan, pix->pixelformat); >>>>> + if (!fmt_info) { >>>>> + pix->pixelformat = chan->format.pixelformat; >>>>> + pix->colorspace = chan->format.colorspace; >>>>> + fmt_info = tegra_core_get_format_by_fourcc(chan, >>>>> + pix->pixelformat); >>>>> + } >>>>> + >>>>> + /* Change this when start adding interlace format support */ >>>>> + pix->field = V4L2_FIELD_NONE; >>>>> + fmt.which = V4L2_SUBDEV_FORMAT_TRY; >>>>> + fmt.pad = 0; >>>>> + v4l2_fill_mbus_format(&fmt.format, pix, fmt_info->code); >>>>> + v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt); >>>> As fas as I understand, entities formats should be independent, it is up to link_validate >>>> to check formats between entities. >>>> The capture shouldn't change the format of the subdevice. >>>> >> Tegra Built-in TPG on CSI accepts specific TPG sizes and CSI is source and VI is sink. >> >> link validation happens only for sink ends of the link. > And what is the problem with it being on the sink end? > You just need to implement custom link validation in tegra_csi_media_ops that also checks the format > between the capture and the subdevice, no? Unless I missunderstood something here (which is quite possible). > > Examples: > drivers/staging/media/rkisp1/rkisp1-capture.c - rkisp1_capture_link_validate() > drivers/media/pci/intel/ipu3/ipu3-cio2.c - cio2_video_link_validate() > drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c - sun6i_video_link_validate() > > Regards, > Helen > But if we move subdevice side format/size check into its link_validation, any incorrect image size set thru set-fmt-video will be taken and get-fmt-video will also show same as it doesn't validate formats/sizes supported by CSI subdev during this time. link validation happens during pipeline start. So thought to prevent accepting incorrect format/size during set-fmt-video/get-fmt-video. Other than this I don't see any issue moving it to link_validation. >> So with CSI subdev set_fmt sets width/height to default incase if width/height is not from one of the supported sizes. >> >>>>> + >>>>> + v4l2_fill_pix_format(pix, &fmt.format); >>>>> + tegra_channel_fmt_align(chan, &fmt_info->bpp, &pix->width, &pix->height, >>>>> + &pix->bytesperline); >>>>> + pix->sizeimage = pix->bytesperline * pix->height; >>>>> + >>>>> + if (vfmt) >>>>> + *vfmt = fmt_info; >>>>> + >>>>> + v4l2_subdev_free_pad_config(pad_cfg); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int tegra_channel_try_format(struct file *file, void *fh, >>>>> + struct v4l2_format *format) >>>>> +{ >>>>> + struct v4l2_fh *vfh = file->private_data; >>>>> + struct tegra_channel *chan = to_tegra_channel(vfh->vdev); >>>>> + >>>>> + return __tegra_channel_try_format(chan, &format->fmt.pix, NULL); >>>>> +} >>>>> + >>>>> +static int tegra_channel_set_format(struct file *file, void *fh, >>>>> + struct v4l2_format *format) >>>>> +{ >>>>> + struct v4l2_fh *vfh = file->private_data; >>>>> + struct tegra_channel *chan = to_tegra_channel(vfh->vdev); >>>>> + const struct tegra_video_format *info; >>>>> + int ret; >>>>> + struct v4l2_subdev_format fmt; >>>>> + struct v4l2_subdev *subdev; >>>>> + struct v4l2_pix_format *pix = &format->fmt.pix; >>>>> + >>>>> + if (vb2_is_busy(&chan->queue)) >>>>> + return -EBUSY; >>>>> + >>>>> + /* get supported format by try_fmt */ >>>>> + ret = __tegra_channel_try_format(chan, pix, &info); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + subdev = tegra_channel_get_remote_subdev(chan); >>>>> + >>>>> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >>>>> + fmt.pad = 0; >>>>> + v4l2_fill_mbus_format(&fmt.format, pix, info->code); >>>>> + v4l2_subdev_call(subdev, pad, set_fmt, NULL, &fmt); >>>> same here. >>>> >> Calling subdev set_fmt here for the same reason as explained above. >>>>> + >>>>> + v4l2_fill_pix_format(pix, &fmt.format); >>>>> + chan->format = *pix; >>>>> + chan->fmtinfo = info; >>>>> + tegra_channel_update_format(chan, pix->width, >>>>> + pix->height, info->fourcc, >>>>> + &info->bpp, >>>>> + pix->bytesperline); >>>>> + *pix = chan->format; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int tegra_channel_enum_input(struct file *file, void *fh, >>>>> + struct v4l2_input *inp) >>>>> +{ >>>>> + /* Currently driver supports internal TPG only */ >>>>> + if (inp->index != 0) >>>> just >>>> if (inp->index) >>>> >> Will update in v2 >>>>> + return -EINVAL; >>>>> + >>>>> + inp->type = V4L2_INPUT_TYPE_CAMERA; >>>>> + strscpy(inp->name, "Tegra TPG", sizeof(inp->name)); >>>>> + >>>>> + return 0; >>>>> +} >>>>> +static const struct tegra_video_format tegra_default_format = { >>>>> + /* RAW 10 */ >>>>> + TEGRA_VF_RAW10, >>>>> + 10, >>>>> + MEDIA_BUS_FMT_SRGGB10_1X10, >>>>> + {2, 1}, >>>>> + TEGRA_IMAGE_FORMAT_DEF, >>>>> + TEGRA_IMAGE_DT_RAW10, >>>>> + V4L2_PIX_FMT_SRGGB10, >>>>> + "RGRG.. GBGB..", >>>> It would be more readable to do: >>>> >>>> .code = TEGRA_VF_RAW10, >>>> .width = 10, >>>> .code = MEDIA_BUS_FMT_SRGGB10_1X10, >>>> >>>> and so on >> Will update in v2 >>>>> +}; >>>>> + >>>>> +/* >>>>> + * Helper functions >>>>> + */ >>>>> + >>>>> +/** >>>>> + * tegra_core_get_default_format - Get default format >>>>> + * >>>>> + * Return: pointer to the format where the default format needs >>>>> + * to be filled in. >>>>> + */ >>>>> +const struct tegra_video_format *tegra_core_get_default_format(void) >>>>> +{ >>>>> + return &tegra_default_format; >>>>> +} >>>> This is only used in tegra-channel.c, why not to declare it there as static? >>>> >> Will move all video format retrieval helper functions to corresponding file as static in v2 >>>>> + +static struct v4l2_frmsize_discrete tegra_csi_tpg_sizes[] = { >>>>> + {1280, 720}, >>>>> + {1920, 1080}, >>>>> + {3840, 2160}, >>>>> +}; >>>>> + >>>>> +/* >>>>> + * V4L2 Subdevice Pad Operations >>>>> + */ >>>>> +static int tegra_csi_get_format(struct v4l2_subdev *subdev, >>>>> + struct v4l2_subdev_pad_config *cfg, >>>>> + struct v4l2_subdev_format *fmt) >>>>> +{ >>>>> + struct tegra_csi_channel *csi_chan = to_csi_chan(subdev); >>>>> + >>>>> + mutex_lock(&csi_chan->format_lock); >>>> Do you need this lock? I think there is already a serialization in the ioctls in place (to be confirmed). >>>> >> This is on CSI v4l2 subdevice side during format updates >>>>> + memcpy(fmt, &csi_chan->ports->format, >>>>> + sizeof(struct v4l2_mbus_framefmt)); >>>> I would prefer just: >>>> *fmt = *csi_chan->ports->format; >>>> >>>> I think it is easier to read IMHO. >>>> same in tegra_csi_set_format(). >>>> >> Will fix in v2 >>>>> + mutex_unlock(&csi_chan->format_lock); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static void tegra_csi_try_mbus_fmt(struct v4l2_subdev *subdev, >>>>> + struct v4l2_mbus_framefmt *mfmt) >>>>> +{ >>>>> + struct tegra_csi_channel *csi_chan = to_csi_chan(subdev); >>>>> + struct tegra_csi_device *csi = csi_chan->csi; >>>>> + const struct v4l2_frmsize_discrete *sizes; >>>>> + int i, j; >>>> unsigned >>>> >> Will fix in v2 >>>>> + >>>>> + for (i = 0; i < ARRAY_SIZE(tegra_csi_tpg_fmts); i++) { >>>>> + struct v4l2_mbus_framefmt *mbus_fmt = &tegra_csi_tpg_fmts[i]; >>>>> + >>>>> + if (mfmt->code == mbus_fmt->code) { >>>>> + for (j = 0; j < ARRAY_SIZE(tegra_csi_tpg_sizes); j++) { >>>>> + sizes = &tegra_csi_tpg_sizes[j]; >>>>> + if (mfmt->width == sizes->width && >>>>> + mfmt->height == sizes->height) { >>>>> + return; >>>>> + } >>>>> + } >>>>> + } >>>>> + >>>>> + dev_info(csi->dev, "using Tegra default RAW10 video format\n"); >>>>> + } >>>>> + >>>>> + dev_info(csi->dev, "using Tegra default WIDTH X HEIGHT (1920x1080)\n"); >>>>> + memcpy(mfmt, tegra_csi_tpg_fmts, sizeof(struct v4l2_mbus_framefmt)); >>>>> +} >>>>> + >>>>> +