Message ID | 20240528020425.4994-1-nas.chung@chipsnmedia.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Hans Verkuil |
Headers |
Received: from am.mirrors.kernel.org ([147.75.80.249]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-12052-patchwork=linuxtv.org@vger.kernel.org>) id 1sBmCm-0003Ln-1F for patchwork@linuxtv.org; Tue, 28 May 2024 02:04:50 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 12D991F23206 for <patchwork@linuxtv.org>; Tue, 28 May 2024 02:04:46 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 1DFC7BA29; Tue, 28 May 2024 02:04:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chipsnmedia.com header.i=@chipsnmedia.com header.b="AWD8I1CY" X-Original-To: linux-media@vger.kernel.org Received: from SLXP216CU001.outbound.protection.outlook.com (mail-koreacentralazon11020002.outbound.protection.outlook.com [52.101.154.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5278E1C2E; Tue, 28 May 2024 02:04:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=52.101.154.2 ARC-Seal: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716861879; cv=fail; b=ADxbKJ3DbjEgiBCuwl5wNjuAmw92e++YpTucF/u9EFHSEVweHqth+6tYuyJaprtzgsH9aH7yYpsvPWUyC0ZLbZYp03u7nzxYplRRpRaS8amMbiOf9Il/Arhm8kaVDMqCjavjjq8FsXeQd3MU8wkwYL/or2Q9xmMr2DpuU3WHe2s= ARC-Message-Signature: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716861879; c=relaxed/simple; bh=tMyT2sSDxBhxe39F54EPp33kXcjYgwxAYK0MYQbFwWs=; h=From:To:Cc:Subject:Date:Message-Id:Content-Type:MIME-Version; b=WAZAxCM4IyljFueNB2YSXAUTc5uTHYmXef918Ugk7Nw86u40hHor2vXZk4JqcfOBFjamNcvKGnfKTFamHFsemRjmU1epIM2fm+dIW/aFyz1E8711cPRxCvgX6l8STwR+Cnr0FlUNLP0TQXCBwbyhtzSKf59g8+q82YB74J/ICkU= ARC-Authentication-Results: i=2; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=chipsnmedia.com; spf=pass smtp.mailfrom=chipsnmedia.com; dkim=pass (1024-bit key) header.d=chipsnmedia.com header.i=@chipsnmedia.com header.b=AWD8I1CY; arc=fail smtp.client-ip=52.101.154.2 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=chipsnmedia.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chipsnmedia.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=B7H0H11okWGA4Q1t3+sAt/pOZ6syUmxzE+BRH/clal+Let+bkYX2W2kbJS1XAPrFEEfvuS9//CVRytaRw+K0oB6Z16nivbluX0xeex+X5d2vekWatshvz+Yq4enl0mWlT2tsjlzYOKFv0QCYOiSOtymMe8YYyGg5Nu8o/InCV7EmCd1ArAJFfPqZmni0HmzJF8PS2E/f5XxdBFZXpoUV49dkbXsZ0lus71/5xJKAnEJqpRLHBdqq7y+/UxCzTf+u01xkOnq5n8yPYH2qSkEmOtteSCI410r6mvuFz4D9/mogJ5xIXX9D4C7y2yk0zuYt4LqBzSgh/FdUVuS2L3O3fg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=xPmlE/higG1YRC2kVaDvfjD6V+re8TgdqojDHZgGKo8=; b=Ezm00wwu9r+OTp7XCP/0s2La5/ZFvuJM0jQuoK3Ey5sEnS7W/d63DiEz0QOoR5IEadLodTttSW8NlYsqwCP5R1Pir66OlKkMqaEInbTRsKdUr4+C8SxxF9vXNWugfSC2eCKEUa5/ayFBYyY3FgnMwce+J2K7LDcqjQKi/VVzxVRjY/fzSIc7tCkBdndQZMMfHzMgcBOCIxonMBLrS3qUVJ3BAsQtSpUKJdrPu5ooeiLL6ohvcNxT8d7PKumwgBPwez6c9G2sfTDmdftT1KTwQtQffiK7n4QMygGFrh7JehyjIKx6MPZLEpnpX35QXnSTEibUmw+SpnO0acMgXKbQIQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=chipsnmedia.com; dmarc=pass action=none header.from=chipsnmedia.com; dkim=pass header.d=chipsnmedia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chipsnmedia.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=xPmlE/higG1YRC2kVaDvfjD6V+re8TgdqojDHZgGKo8=; b=AWD8I1CYM4Esn+HPyPzZD1I+Funb8hKaiV3UwM3WacD13qL+/+XFXknSFjXTAzFv/HETjEKjKxHcha7E3rGACmrdLKLfNOheY1gbtFhrhzdfyD1qPT0PqW5Jye/5AUtteNVgs59wRQOvXJmIiNYBHS3NuIDqgFsDT+zo2eJ0LrI= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=chipsnmedia.com; Received: from SL2P216MB1246.KORP216.PROD.OUTLOOK.COM (2603:1096:101:a::9) by PS2P216MB1387.KORP216.PROD.OUTLOOK.COM (2603:1096:301:73::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7611.30; Tue, 28 May 2024 02:04:34 +0000 Received: from SL2P216MB1246.KORP216.PROD.OUTLOOK.COM ([fe80::9e3d:ee20:8cc7:3c07]) by SL2P216MB1246.KORP216.PROD.OUTLOOK.COM ([fe80::9e3d:ee20:8cc7:3c07%4]) with mapi id 15.20.7611.030; Tue, 28 May 2024 02:04:34 +0000 From: Nas Chung <nas.chung@chipsnmedia.com> To: mchehab@kernel.org, linux-media@vger.kernel.org, sebastian.fricke@collabora.com, m.tretter@pengutronix.de Cc: linux-kernel@vger.kernel.org, Nas Chung <nas.chung@chipsnmedia.com> Subject: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition Date: Tue, 28 May 2024 11:04:25 +0900 Message-Id: <20240528020425.4994-1-nas.chung@chipsnmedia.com> X-Mailer: git-send-email 2.25.1 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: SL2P216CA0086.KORP216.PROD.OUTLOOK.COM (2603:1096:101:2::19) To SL2P216MB1246.KORP216.PROD.OUTLOOK.COM (2603:1096:101:a::9) Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SL2P216MB1246:EE_|PS2P216MB1387:EE_ X-MS-Office365-Filtering-Correlation-Id: 1613553d-d29f-44d1-96f4-08dc7eba84ef X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230031|1800799015|376005|52116005|366007|38350700005; X-Microsoft-Antispam-Message-Info: wYNTmXOGKOtMce6iEvc3q1qKrsgmj3Dc1C8tdyKc0KkiUf+7yhiPyDs2BYDXp1LNlZurd0G6GP7vZTd8AAautWkPRyYYasVXquygy8iFEwtcxOo7hlAoMlrhwbPVH+e7dXp+DkktGVoDws8xuIdHdxMz6pnZBOyxx9a5C6yAqs0JVuVxvvG6J4Mmm5/WXjaCYUVEPtLieudCUCE+RO5gNe9aR+4QUF/PjAzEUxvKWiLKS07VefQpmzZlhu3QQ18XJMhtB+/rNYX8FM+7/HXuO63pjeGsapKB2XOEBSRKPIMFusElJMybBdGsRQLI8O8xofH80nJT/fP93k4J5VoW7fn74qxqRnRkwo7JAV8y7qLNB8uJGFALJ3TIZRoDbIGdvHOaUHeNbwzjgCJIFT6+dZZ04B16zzL1xlmdw/fg+GOvEJVq7ne/u5ahusdk7wNWgH/uVPFgA0olpZCQFAj+XCAs0nm2qqUQ4uJwHl5yZ9DO5ERUm/JG6lsoDGtUW9NWUConmgqtO1DkmgV5D99L0blBbX2VC0vHtUdcewnNyxpZkG7oAkPieorA+hhktxM+LshqAsSVoUArXPTfIJ/zRfRXRFPEPkcxydNgsfuSxouuv3VJZJK1/ZoPlIvglzONROY183eugSLMuJO8NhjzOrpuQ1DrO+K+1D5Gnf7jzn9c8ZguX2Kru9V84BKO+26eeNW7bxSZCmxc4iIpCUnx9dfLoWWwObQo4pDahWMBj+BWbj2rvYH9ub36hn4EQWPOicjWqeoqndPHns5ZKHmWIZZ+aQkDvDmT1wPm3l46ewpeq8u3RYX9m07qZoncuRcgq5XT1a9ANaHit9/ZaslZm/TwnjzD3tdjYru/S5lSHSmmvWL0Qhz9jA+P0u4MsjzmUn8Ie3CuQ52dQx73ZuYAnK+dnr5urup7uSYTeqBWF2Qr0kzR6fH83euHH+y1BujTfbeCvSVIJmLaTGmdnoSD0Z3z8Rw6iM05FZtjWrl9Dn3Tj4yynyST67lPbAid2gsu3y1povZ/YStudc6H3a+2yMMiLOmyfIKuO1YSP7eO0tTjs04AW9UK2bPg6V6tFqSfNbZPsY4lI1B1BwC3rqCXHoKPOUDJUHRntkpMFc/iX2DWX7ugPf0ECBapAYPQQ5EnPbYW9tf683rWQaDKkN9upgt7/2Inah1S8Kj7QWEGphma6zics7KPqLDyyvYLJ5i1M+Z6qgmEpayCp3OMHREwwbrGnRY2NjoyfaPvZhdDzDrhbOktdM4psRsYSYSO875OMIrztXpLJNvlZdzMpdPrLLZuwMofHZKqrwzy87dMVRYF8bvVbIl7AFKbXrYvB7gkf+3Ut/5rQNTWxMgL8zQsUA== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SL2P216MB1246.KORP216.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFS:(13230031)(1800799015)(376005)(52116005)(366007)(38350700005);DIR:OUT;SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: 23RZm70uVP8AcxFqhEiEbb2e52ksQ0eCxRfnNYVkq5JdWEvzIBKbU06c2BBNlYchLc1OROCK2lFuLhZCUX+nz3Kg2zQmALB2CM2iiB7fJE6sPwdY2n2ohiRp4U8aoIdlMN7OskcY7iHPY2Nb+3IYeN24QsszFYrWL96xvZ8pF26TY61q2JMOnXkWnw7ui2LBh9PhpsEIOJ5Yih2PUXjylnzItJCeMiW5ZdIi32BcazAuQeiw2KAl6lHyHwf0HLoO8FERkfmk5xLDJ7nIVC1KtLvvOOFn3WCPNMhq3yZgjcz1dQB1+knJINvBODiU4rwR1fNYXJdckZiJO3tsW75QE+Sh/v9fhaQ99HvCzdOx58oBTeO+afzqzdG5XsCqhOt9NJOpvZ975YrnlNRWZQZBq4S4Qa9bD8/iI/DM1OdoKLvfyzDgVCsO/ksRfuNPXARUENGCYn3bwmH5rCs+jDkYJvePEbu8z7K6/quUxQq5SHfeN+r2XTqI0p6RzA0r8bMmA2PD7hzoP7zqooD1qt3GocPIx/10yTyMDKs9EsOBRRB4TzFGxw+XQNP9iNIY/PA4SSIRwQ27Uv2kSb7AWmdFsJAVIzSx4m0+j2tMRGGTzjEws8Gw7/HOIrJSxcsZjqDm34xF5ZckzVOcX1yJVqIZsxqIXIi1wZvxvqg2f5t83DsE7G7K52GPKWPVndlJDbPTalq/LWEP0tpjKBw8orXprtXHFE7EBf6CvP+80bLvCE2oVle63eNlpF2lALTGmcLqL7kVlaWaYc4h4idrwrwS5fhG0Ues89rtSUqXGYAkKu/LARAghdKx06n+VBuUuE8w9v4HqLTLfZXJBuCUd2KtWSHbbDReR8McRaA9H4i4wYKLemjpw9fjvanvx16oe/46fKTzI9vCpLU3txfLBiBNAr6EaitgZlBn1L7pvlZbi0Y5ZT+KLb3Q2PrGgqjUrhCa1xHuKiWkZvCg9QeaeFcz5wk0RbDukk7Hremkr1+dij29Fp9BkSBibrYDK+HnSYaBdLrVwxiAJ9dNp9h8VKJXLzOIVZD/vNNQxWkYB8mnr0pZf1yNsX3n8tAaGeWBiizNeioHVehqmDd3j1JMOLPiHS3zxeVjP6lY0M7crQnnKLPP0Yqrg0Q6z8lM4qPic5QmD4QzhvOtYRM86PG2wyMzH6TuLVONOEhfdcLOhTVaS+7P9uAtsJyXLRR5bJjQUvFB16CYOyL1aRrdE4Q4ZlVGoJLSoLCP/yzIuhXNPnHAwVx9LM70/aA3kU39/vwbHvzG6FeCtQREPIWosRwNIK6jRVIr5xfzTjxq8qXvSqd1ANIcvPoaE/DyeNpnuwUJyICEJzcRrDoQELgidk7SSbkrZ/VdPBIpJgI+mKF6+sVefNeSXWmFT1pp7loK1lOLkCJPJp1GuQeghDom60iAJi7Q9FIg+Sa76HvAPKcYBfSI/JjZXjpbN/RI7x3QkattWMDClYNHr9wOel9pe7dg9GRTjUY5JkUwtHOXpgCLNw5TMTHNXd/g9nQ8dN+vG1yPBQMYDhc03Jo1SHMRozuJ5lqeqVxyBQpu+qBbcVme1J+eU4g7mpbfWJFbRRaCgABObCn2+zsPMjujU5HWU5CN/DmWfg== X-OriginatorOrg: chipsnmedia.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1613553d-d29f-44d1-96f4-08dc7eba84ef X-MS-Exchange-CrossTenant-AuthSource: SL2P216MB1246.KORP216.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 02:04:34.0282 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 4d70c8e9-142b-4389-b7f2-fa8a3c68c467 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: CzPOYR7bg8f1s8maAr+Z7A6ST3gW/qfnA93F/vjM2VY+mYz0WXCUwwXIY5DK4j2la46/+hjvHysfbQCrC5zT4rdCKLaJCvF/OfVlEBnAX+A= X-MS-Exchange-Transport-CrossTenantHeadersStamped: PS2P216MB1387 X-LSpam-Score: -2.6 (--) X-LSpam-Report: No, score=-2.6 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DMARC_MISSING=0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_NONE=-0.0001,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no |
Series |
[v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE condition
|
|
Commit Message
Nas Chung
May 28, 2024, 2:04 a.m. UTC
Explicitly compare a buffer type only with valid buffer types,
to avoid matching the buffer type outside of valid buffer
type set.
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
include/uapi/linux/videodev2.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
Comments
On Tue, 28 May 2024 11:04:25 +0900, Nas Chung wrote: > Explicitly compare a buffer type only with valid buffer types, > to avoid matching the buffer type outside of valid buffer > type set. > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> Reviewed-by: Michael Tretter <m.tretter@pengutronix.de> > --- > include/uapi/linux/videodev2.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index fe6b67e83751..fa2b7086e480 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -157,6 +157,10 @@ enum v4l2_buf_type { > V4L2_BUF_TYPE_PRIVATE = 0x80, > }; > > +#define V4L2_TYPE_IS_VALID(type) \ > + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ > + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) > + > #define V4L2_TYPE_IS_MULTIPLANAR(type) \ > ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ > || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > @@ -171,7 +175,8 @@ enum v4l2_buf_type { > || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ > || (type) == V4L2_BUF_TYPE_META_OUTPUT) > > -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) > +#define V4L2_TYPE_IS_CAPTURE(type) \ > + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) > > enum v4l2_tuner_type { > V4L2_TUNER_RADIO = 1, > -- > 2.25.1 > >
On 28/05/2024 04:04, Nas Chung wrote: > Explicitly compare a buffer type only with valid buffer types, > to avoid matching the buffer type outside of valid buffer > type set. > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> > --- > include/uapi/linux/videodev2.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index fe6b67e83751..fa2b7086e480 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -157,6 +157,10 @@ enum v4l2_buf_type { > V4L2_BUF_TYPE_PRIVATE = 0x80, > }; > > +#define V4L2_TYPE_IS_VALID(type) \ > + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ > + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) > + > #define V4L2_TYPE_IS_MULTIPLANAR(type) \ > ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ > || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > @@ -171,7 +175,8 @@ enum v4l2_buf_type { > || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ > || (type) == V4L2_BUF_TYPE_META_OUTPUT) > > -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) > +#define V4L2_TYPE_IS_CAPTURE(type) \ > + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) > > enum v4l2_tuner_type { > V4L2_TUNER_RADIO = 1, This patch introduced this warning: In function 'find_format_by_index', inlined from 'vdec_enum_fmt' at drivers/media/platform/qcom/venus/vdec.c:452:8: drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be used uninitialized [-Wmaybe-uninitialized] 172 | if (k == index && valid) | ~~~~~~~~~~~^~~~~~~~ drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was declared here 157 | bool valid; | ^~~~~ This driver does this: bool valid; if (V4L2_TYPE_IS_OUTPUT(type)) { valid = venus_helper_check_codec(inst, fmt[i].pixfmt); } else if (V4L2_TYPE_IS_CAPTURE(type)) { valid = venus_helper_check_format(inst, fmt[i].pixfmt); With your patch both V4L2_TYPE_IS_OUTPUT(type) and V4L2_TYPE_IS_CAPTURE(type) can return false, something that wasn't possible without this patch. In this driver the fix would just be to drop the second 'if' altogether, so just '} else {'. Since these defines are part of the public API, this change introduces a subtle behavior difference that can affect applications. That said, I do consider this an improvement. I would like some changes, though: 1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum v4l2_buf_type, add a comment saying that V4L2_TYPE_IS_VALID and (for output types) V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all too easy to forget that otherwise. 2) Add a patch fixing the venus/vdec.c code. 3) Something else I noticed, but I think this change should be in a separate patch: V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but that definitely belongs to CAPTURE. Nobody really uses that type anymore, but still, it should be fixed. Regards, Hans
Hi, Hans. >-----Original Message----- >From: Hans Verkuil <hverkuil@xs4all.nl> >Sent: Thursday, May 30, 2024 7:32 PM >To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org; linux- >media@vger.kernel.org; sebastian.fricke@collabora.com; >m.tretter@pengutronix.de >Cc: linux-kernel@vger.kernel.org >Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >condition > >On 28/05/2024 04:04, Nas Chung wrote: >> Explicitly compare a buffer type only with valid buffer types, >> to avoid matching the buffer type outside of valid buffer >> type set. >> >> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >> --- >> include/uapi/linux/videodev2.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/uapi/linux/videodev2.h >b/include/uapi/linux/videodev2.h >> index fe6b67e83751..fa2b7086e480 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -157,6 +157,10 @@ enum v4l2_buf_type { >> V4L2_BUF_TYPE_PRIVATE = 0x80, >> }; >> >> +#define V4L2_TYPE_IS_VALID(type) \ >> + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >> + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >> + >> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> @@ -171,7 +175,8 @@ enum v4l2_buf_type { >> || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ >> || (type) == V4L2_BUF_TYPE_META_OUTPUT) >> >> -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) >> +#define V4L2_TYPE_IS_CAPTURE(type) \ >> + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >> >> enum v4l2_tuner_type { >> V4L2_TUNER_RADIO = 1, > >This patch introduced this warning: > >In function 'find_format_by_index', > inlined from 'vdec_enum_fmt' at >drivers/media/platform/qcom/venus/vdec.c:452:8: >drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be >used uninitialized [-Wmaybe-uninitialized] > 172 | if (k == index && valid) > | ~~~~~~~~~~~^~~~~~~~ >drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': >drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was >declared here > 157 | bool valid; > | ^~~~~ > >This driver does this: > > bool valid; > > if (V4L2_TYPE_IS_OUTPUT(type)) { > valid = venus_helper_check_codec(inst, fmt[i].pixfmt); > } else if (V4L2_TYPE_IS_CAPTURE(type)) { > valid = venus_helper_check_format(inst, fmt[i].pixfmt); > >With your patch both V4L2_TYPE_IS_OUTPUT(type) and >V4L2_TYPE_IS_CAPTURE(type) >can return false, something that wasn't possible without this patch. > >In this driver the fix would just be to drop the second 'if' altogether, >so just >'} else {'. > >Since these defines are part of the public API, this change introduces a >subtle >behavior difference that can affect applications. Thank you for a detailed description. > >That said, I do consider this an improvement. > >I would like some changes, though: > >1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum >v4l2_buf_type, > add a comment saying that V4L2_TYPE_IS_VALID and (for output types) > V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all > too easy to forget that otherwise. >2) Add a patch fixing the venus/vdec.c code. 1), 2) I will address both in v3. >3) Something else I noticed, but I think this change should be in a >separate patch: > V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but >that > definitely belongs to CAPTURE. Nobody really uses that type anymore, >but still, > it should be fixed. 3) Would you prefer this modification to be included as a separate patch in the v3 of this patch series, or should it be submitted as a new standalone patch ? Thanks. Nas. > >Regards, > > Hans
Hey Nas, just before you send out V3 ... On 28.05.2024 11:04, Nas Chung wrote: >Explicitly compare a buffer type only with valid buffer types, >to avoid matching the buffer type outside of valid buffer >type set. s/matching the buffer type outside of valid buffer type set/ matching a buffer type outside of the valid buffer type set/ Regards, Sebastian >Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >--- > include/uapi/linux/videodev2.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > >diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >index fe6b67e83751..fa2b7086e480 100644 >--- a/include/uapi/linux/videodev2.h >+++ b/include/uapi/linux/videodev2.h >@@ -157,6 +157,10 @@ enum v4l2_buf_type { > V4L2_BUF_TYPE_PRIVATE = 0x80, > }; > >+#define V4L2_TYPE_IS_VALID(type) \ >+ ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >+ && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >+ > #define V4L2_TYPE_IS_MULTIPLANAR(type) \ > ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ > || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >@@ -171,7 +175,8 @@ enum v4l2_buf_type { > || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ > || (type) == V4L2_BUF_TYPE_META_OUTPUT) > >-#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) >+#define V4L2_TYPE_IS_CAPTURE(type) \ >+ (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) > > enum v4l2_tuner_type { > V4L2_TUNER_RADIO = 1, >-- >2.25.1 >
On 31/05/2024 09:03, Nas Chung wrote: > Hi, Hans. > >> -----Original Message----- >> From: Hans Verkuil <hverkuil@xs4all.nl> >> Sent: Thursday, May 30, 2024 7:32 PM >> To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org; linux- >> media@vger.kernel.org; sebastian.fricke@collabora.com; >> m.tretter@pengutronix.de >> Cc: linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >> condition >> >> On 28/05/2024 04:04, Nas Chung wrote: >>> Explicitly compare a buffer type only with valid buffer types, >>> to avoid matching the buffer type outside of valid buffer >>> type set. >>> >>> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >>> --- >>> include/uapi/linux/videodev2.h | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/uapi/linux/videodev2.h >> b/include/uapi/linux/videodev2.h >>> index fe6b67e83751..fa2b7086e480 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -157,6 +157,10 @@ enum v4l2_buf_type { >>> V4L2_BUF_TYPE_PRIVATE = 0x80, >>> }; >>> >>> +#define V4L2_TYPE_IS_VALID(type) \ >>> + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >>> + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >>> + >>> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >>> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >>> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>> @@ -171,7 +175,8 @@ enum v4l2_buf_type { >>> || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ >>> || (type) == V4L2_BUF_TYPE_META_OUTPUT) >>> >>> -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) >>> +#define V4L2_TYPE_IS_CAPTURE(type) \ >>> + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >>> >>> enum v4l2_tuner_type { >>> V4L2_TUNER_RADIO = 1, >> >> This patch introduced this warning: >> >> In function 'find_format_by_index', >> inlined from 'vdec_enum_fmt' at >> drivers/media/platform/qcom/venus/vdec.c:452:8: >> drivers/media/platform/qcom/venus/vdec.c:172:32: warning: 'valid' may be >> used uninitialized [-Wmaybe-uninitialized] >> 172 | if (k == index && valid) >> | ~~~~~~~~~~~^~~~~~~~ >> drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_enum_fmt': >> drivers/media/platform/qcom/venus/vdec.c:157:22: note: 'valid' was >> declared here >> 157 | bool valid; >> | ^~~~~ >> >> This driver does this: >> >> bool valid; >> >> if (V4L2_TYPE_IS_OUTPUT(type)) { >> valid = venus_helper_check_codec(inst, fmt[i].pixfmt); >> } else if (V4L2_TYPE_IS_CAPTURE(type)) { >> valid = venus_helper_check_format(inst, fmt[i].pixfmt); >> >> With your patch both V4L2_TYPE_IS_OUTPUT(type) and >> V4L2_TYPE_IS_CAPTURE(type) >> can return false, something that wasn't possible without this patch. >> >> In this driver the fix would just be to drop the second 'if' altogether, >> so just >> '} else {'. >> >> Since these defines are part of the public API, this change introduces a >> subtle >> behavior difference that can affect applications. > > Thank you for a detailed description. > >> >> That said, I do consider this an improvement. >> >> I would like some changes, though: >> >> 1) Just after "V4L2_BUF_TYPE_META_OUTPUT = 14," in enum >> v4l2_buf_type, >> add a comment saying that V4L2_TYPE_IS_VALID and (for output types) >> V4L2_TYPE_IS_OUTPUT must be updated if a new type is added. It's all >> too easy to forget that otherwise. > >> 2) Add a patch fixing the venus/vdec.c code. > > 1), 2) I will address both in v3. > >> 3) Something else I noticed, but I think this change should be in a >> separate patch: >> V4L2_TYPE_IS_OUTPUT() returns true for V4L2_BUF_TYPE_VIDEO_OVERLAY, but >> that >> definitely belongs to CAPTURE. Nobody really uses that type anymore, >> but still, >> it should be fixed. > > 3) Would you prefer this modification to be included as a separate patch > in the v3 of this patch series, or should it be submitted as a new > standalone patch ? Separate patch in this series, please. Regards, Hans > > Thanks. > Nas. > >> >> Regards, >> >> Hans >
Hi Sebastian. >-----Original Message----- >From: Sebastian Fricke <sebastian.fricke@collabora.com> >Sent: Friday, May 31, 2024 4:26 PM >To: Nas Chung <nas.chung@chipsnmedia.com> >Cc: mchehab@kernel.org; linux-media@vger.kernel.org; >m.tretter@pengutronix.de; linux-kernel@vger.kernel.org >Subject: Re: [PATCH v2] media: uapi: v4l: Change V4L2_TYPE_IS_CAPTURE >condition > >Hey Nas, > >just before you send out V3 ... > >On 28.05.2024 11:04, Nas Chung wrote: >>Explicitly compare a buffer type only with valid buffer types, >>to avoid matching the buffer type outside of valid buffer >>type set. > >s/matching the buffer type outside of valid buffer type set/ > matching a buffer type outside of the valid buffer type set/ Thank you for the review! I will fix it in v3. Thanks. Nas. > >Regards, >Sebastian > >>Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com> >>--- >> include/uapi/linux/videodev2.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >>diff --git a/include/uapi/linux/videodev2.h >b/include/uapi/linux/videodev2.h >>index fe6b67e83751..fa2b7086e480 100644 >>--- a/include/uapi/linux/videodev2.h >>+++ b/include/uapi/linux/videodev2.h >>@@ -157,6 +157,10 @@ enum v4l2_buf_type { >> V4L2_BUF_TYPE_PRIVATE = 0x80, >> }; >> >>+#define V4L2_TYPE_IS_VALID(type) \ >>+ ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ >>+ && (type) <= V4L2_BUF_TYPE_META_OUTPUT) >>+ >> #define V4L2_TYPE_IS_MULTIPLANAR(type) \ >> ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ >> || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>@@ -171,7 +175,8 @@ enum v4l2_buf_type { >> || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ >> || (type) == V4L2_BUF_TYPE_META_OUTPUT) >> >>-#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) >>+#define V4L2_TYPE_IS_CAPTURE(type) \ >>+ (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) >> >> enum v4l2_tuner_type { >> V4L2_TUNER_RADIO = 1, >>-- >>2.25.1 >>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index fe6b67e83751..fa2b7086e480 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -157,6 +157,10 @@ enum v4l2_buf_type { V4L2_BUF_TYPE_PRIVATE = 0x80, }; +#define V4L2_TYPE_IS_VALID(type) \ + ((type) >= V4L2_BUF_TYPE_VIDEO_CAPTURE \ + && (type) <= V4L2_BUF_TYPE_META_OUTPUT) + #define V4L2_TYPE_IS_MULTIPLANAR(type) \ ((type) == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE \ || (type) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) @@ -171,7 +175,8 @@ enum v4l2_buf_type { || (type) == V4L2_BUF_TYPE_SDR_OUTPUT \ || (type) == V4L2_BUF_TYPE_META_OUTPUT) -#define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type)) +#define V4L2_TYPE_IS_CAPTURE(type) \ + (V4L2_TYPE_IS_VALID(type) && !V4L2_TYPE_IS_OUTPUT(type)) enum v4l2_tuner_type { V4L2_TUNER_RADIO = 1,