From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) (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 01458217F36 for ; Thu, 17 Jul 2025 06:51:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.168.131 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752735120; cv=none; b=H8p1K69b2fo4Kyl9kPZXJF7ZKTcefsaGhYCHoqzmCuLTml7yDca+OkGabEyZIdSSkNexUI+t+JWZC6bkJeDH9vh/tJVwgE30zAUur7fk1OHJVimMSRYtHKe15aj6sjXqCpgZ49ubRvx7+/2yVnX1joTG1PCjfBFk873cKSPa5rQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752735120; c=relaxed/simple; bh=xTfpokh+eM0H7vAUTJOWN2GUKk3IEDrRn1bH73Hrt1M=; h=From:Date:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dcEPceeIcYjsvTSQXVdRK2JqUKdR9v2fbFxOY22KsK6MHjH5cOfBzcoqv5WbDfIrp98vVkM6LMUly1J5Ju6Co6brLFYrxxFu+gTwE9E1bdjOZkLD1iBYp+gvTK8n7geuIzLFjAhjIOxPYv7I7Oe8+xJT4bU8un5xMRsP7P+lSe0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com; spf=pass smtp.mailfrom=oss.qualcomm.com; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b=JnatKgWr; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b="JnatKgWr" Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 56H4Wgf8000541 for ; Thu, 17 Jul 2025 06:51:58 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qualcomm.com; h= cc:content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to; s=qcppdkim1; bh=q9sPxZbJPv/UCPRDIroL0FL6 33a28OgGsC/135RRF34=; b=JnatKgWrnpG0FqyRAv4PD44/v8jlAIrUHm7I5mwC kVxNwKPXaSbSxYJODfnJzAFn4mg9NeLY8h9dKTRCGp0zofn/JY31K5j3QVeMi686 2CRiTlnpoveFrEWHFIduz6W207nAj10jqsCCqgHcNLQ+zlC9eu8hf0XgAzoYl3xq JHQdSxIz1U8njPCljOtNuYm2KlDF9XFE8gcRIgtUkmhh3AEm5/olvKBR8NfPw0gI GTk/ASYnFzzvC0i40ZxNMWqgdtMv2vOnmasYDA68spdP4591gUnzpM1DQFfPcd6H M34z/bL0u6IxLozqB9sWd3F28Of+L60Fi3T0gRgzwDRkQA== Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 47w5drsmjg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Thu, 17 Jul 2025 06:51:58 +0000 (GMT) Received: by mail-qk1-f198.google.com with SMTP id af79cd13be357-7ceb5b5140eso101268185a.2 for ; Wed, 16 Jul 2025 23:51:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1752735117; x=1753339917; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=q9sPxZbJPv/UCPRDIroL0FL633a28OgGsC/135RRF34=; b=FBdFVy5LHbLK+MTTm4blEq4B2ywiqF99ECrRWp+sP2b1MxWdCEWEZ4Xv/aXzn3o8xv Ve789LMw5juJUIY6y2hjtOYjaoQ7lpQoP0QMmeMDW+2iX6D5QxiGbYsJDmV7G96mCIzk 9khrkUkcCoopKa+gtbEragtPWGP6tdyWzCxtY5Si+PR22IczKh6rRCs77Qq7lnf3y2dD Ai+l+33Rrq1MNbSY6/td125GS+KOHSyxqIm3pxEkpf5aS+wV7KI+UUyKr9tF0JZ34iJU 7ML/OzQ1fA8gYjZh1uqDJ1HHVoDB9FM3o7LSIjw81kajk8lyWZeVAT+n6pBTwkKjSdqa gx/Q== X-Forwarded-Encrypted: i=1; AJvYcCWXHqfat8oJVN+uEMwCdAZdWisnAvJsuU4wCBGu90DrMCKqVuBLv6HnVb1u+GBgbmF83mMUzLpuCpYo@vger.kernel.org X-Gm-Message-State: AOJu0YzrIJrAxvToB7RAnLIfP89UCs+XkkcnaNnUMDXkuho8Bl9xZZRV oiF8Sgv7s8q6QhvbONoSmylwJ+GCE6MNxgUbDXeQJ+A6jLsoDmm6slKUZeqIlycIwu4YqDwCp5L TtMJ5HQtPjvlGZN53jqVWxd4GiJf4m0J3/2K8S8zOzhTNVEHBNBiY2KlBhXkSxKeT X-Gm-Gg: ASbGncvKxB1joCSl/kA8FeIJvhLyGB1YYFP0xjBS+YFlYl32yk5X+yBA0msVpYAX87p qupgHivHzGQkBBP0n8wMDfYr2ESICia/uGucD0gslsW11OM73bV3OOX5EprQIwThmFt0AkZ8XEv /WkuASSwN3k9syRsOkElK+0qb4amO17FMT/x1OreFjNTnzES07HHbfD5aD9Q3tFrvCDKKFG+SQj poT6Zr45KMVog9AS8aOVmYpTFSp1pee48YVa0TTFXeoN+gTqXuQwL886hlDMFsNQ8ljQo3AlI3v fcUibLLPm06bHlC+lqLtB2bioEXx7RiQ4t5r75XUaj9zUmlU1ze6ineretOioEXoQDeBmO5aGN0 = X-Received: by 2002:a05:620a:4606:b0:7e3:2b48:7a83 with SMTP id af79cd13be357-7e342b5bd3cmr750108885a.39.1752735116569; Wed, 16 Jul 2025 23:51:56 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHVfrBEr4ilxky/eDf4VZ8EjlWDAXKMdDdNkfwskeQj3T1yecsUXR+JjMCV9BPPVRtGb8bPIQ== X-Received: by 2002:a05:620a:4606:b0:7e3:2b48:7a83 with SMTP id af79cd13be357-7e342b5bd3cmr750102785a.39.1752735115303; Wed, 16 Jul 2025 23:51:55 -0700 (PDT) Received: from trex (153.red-79-144-197.dynamicip.rima-tde.net. [79.144.197.153]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3b612db7060sm2146179f8f.52.2025.07.16.23.51.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jul 2025 23:51:54 -0700 (PDT) From: Jorge Ramirez X-Google-Original-From: Jorge Ramirez Date: Thu, 17 Jul 2025 08:51:53 +0200 To: Bryan O'Donoghue Cc: Jorge Ramirez-Ortiz , quic_vgarodia@quicinc.com, quic_dikshita@quicinc.com, krzk+dt@kernel.org, konradybcio@kernel.org, mchehab@kernel.org, andersson@kernel.org, conor+dt@kernel.org, amit.kucheria@oss.qualcomm.com, linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 2/7] media: venus: Conditionally register codec nodes based on firmware version Message-ID: References: <20250715204749.2189875-1-jorge.ramirez@oss.qualcomm.com> <20250715204749.2189875-3-jorge.ramirez@oss.qualcomm.com> <2fd0d1a7-70ee-43ac-af84-d2321c40e8f8@linaro.org> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2fd0d1a7-70ee-43ac-af84-d2321c40e8f8@linaro.org> X-Proofpoint-ORIG-GUID: wlgU35N05ziufDodwl11XWbqGmFYgaE9 X-Authority-Analysis: v=2.4 cv=D4xHKuRj c=1 sm=1 tr=0 ts=68789d8e cx=c_pps a=qKBjSQ1v91RyAK45QCPf5w==:117 a=Ki5fnJvzvo7yLsyA0quaxQ==:17 a=kj9zAlcOel0A:10 a=Wb1JkmetP80A:10 a=EUspDBNiAAAA:8 a=QkW7CUjfO7Dj9zUrQigA:9 a=CjuIK1q_8ugA:10 a=NFOGd7dJGGMPyQGDc5-O:22 X-Proofpoint-GUID: wlgU35N05ziufDodwl11XWbqGmFYgaE9 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjUwNzE3MDA1OCBTYWx0ZWRfXxQJmxuOzMYBs tB+6GpA5xdQdyMmpkWVX8MMdp8uqYXzEWup/7jnl1CVWCYawzB+Kd96fdy7j0bwnLltit13j0ui KjOX3okpE5ybdo2TgbOwvUhlJG/xCfPH4p8YUcBxReTrYm7ogI+/1kdbornKbsrmRQvpdPR3LzS 77ALFk9Dk2sABl0OrAS2PjVpZkC3P1gHKniKbiGsxgTS0PX0Q6Il8a8gKwJH2kI4mCxJmyU2VnU 83T1kcA0Xu5oYpDzM/WFxOFRmDWbcfokih291NSE/2UCuSqKvOSjjyrW8Xij18d9xyWlMX3XYNu 8LeZn5h+v+rfsiDOCOlh/8fP2tuMdHEy/+6S+wl5fTuKgmyjktDTpxv7TKPDYhkVIkUNGLo5pKt mzLijfjYNB3+pRuz7G1G4d7jYHWSZXRS+vvPKU4azfLvCHwpJC4g8pa7MUFpX+hzfXWqeSPc X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1099,Hydra:6.1.9,FMLib:17.12.80.40 definitions=2025-07-17_01,2025-07-16_02,2025-03-28_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 adultscore=0 mlxlogscore=999 impostorscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 malwarescore=0 suspectscore=0 bulkscore=0 mlxscore=0 priorityscore=1501 phishscore=0 classifier=spam authscore=0 authtc=n/a authcc= route=outbound adjust=0 reason=mlx scancount=1 engine=8.19.0-2505280000 definitions=main-2507170058 On 17/07/25 00:37:33, Bryan O'Donoghue wrote: > On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote: > > The encoding and decoding capabilities of a VPU can vary depending on the > > firmware version in use. > > > > This commit adds support for platforms with OF_DYNAMIC enabled to > > conditionally skip the creation of codec device nodes at runtime if the > > loaded firmware does not support the corresponding functionality. > > > > Note that the driver becomes aware of the firmware version only after the > > HFI layer has been initialized. > > > > Signed-off-by: Jorge Ramirez-Ortiz > > --- > > drivers/media/platform/qcom/venus/core.c | 76 +++++++++++++++--------- > > drivers/media/platform/qcom/venus/core.h | 8 +++ > > 2 files changed, 57 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > > index 4c049c694d9c..b7d6745b6124 100644 > > --- a/drivers/media/platform/qcom/venus/core.c > > +++ b/drivers/media/platform/qcom/venus/core.c > > @@ -28,6 +28,15 @@ > > #include "pm_helpers.h" > > #include "hfi_venus_io.h" > > +static inline bool venus_fw_supports_codec(struct venus_core *core, > > + const struct venus_min_fw *ver) > > +{ > > + if (!ver) > > + return true; > > + > > + return is_fw_rev_or_newer(core, ver->major, ver->minor, ver->rev); > > +} > > + > > static void venus_coredump(struct venus_core *core) > > { > > struct device *dev; > > @@ -103,7 +112,9 @@ static void venus_sys_error_handler(struct work_struct *work) > > core->state = CORE_UNINIT; > > for (i = 0; i < max_attempts; i++) { > > - if (!pm_runtime_active(core->dev_dec) && !pm_runtime_active(core->dev_enc)) > > + /* Not both nodes might be available */ > > "Neither node available" the latter for preference. what about "One or both nodes may be unavailable" ? > > > + if ((!core->dev_dec || !pm_runtime_active(core->dev_dec)) && > > + (!core->dev_enc || !pm_runtime_active(core->dev_enc))) > > Is this change about registration or is it a fix trying to sneak in under > the radar ? I think this functionality - the ability to enable or disable individual encode/decode nodes based on firmware capabilities - should be standard across multimedia drivers. For example, on the AR50_LITE platform, the _current_ driver/firmware combo does not support encoding as it requires secure buffer handling which is not yet implemented in the kernel (changes to iommu, etc) So, rather than disabling Venus entirely, I think it makes sense to expose the decoder node, which remains fully functional and unaffected by the secure buffer requirement. Hence this commit (so yeah, I am not trying to sneak a fix, I swear!) > > > break; > > msleep(10); > > } > > @@ -202,7 +213,8 @@ static u32 to_v4l2_codec_type(u32 codec) > > } > > } > > -static int venus_enumerate_codecs(struct venus_core *core, u32 type) > > +static int venus_enumerate_codecs(struct venus_core *core, u32 type, > > + const struct venus_min_fw *ver) > > { > > const struct hfi_inst_ops dummy_ops = {}; > > struct venus_inst *inst; > > @@ -213,6 +225,9 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type) > > if (core->res->hfi_version != HFI_VERSION_1XX) > > return 0; > > + if (!venus_fw_supports_codec(core, ver)) > > + return 0; > Its not really a codec you're checking there, its a version. > > The name should reflect that. but the check isn't just about the firmware version: it is about whether the firmware in use supports a specific coded based on the firmware version knowledge built in the driver. so really, while the logic involves a version check, it is really about the codec capability. I could rename it as is_codec_enabled_by_fw() if the current naming is not clear. > > > + > > inst = kzalloc(sizeof(*inst), GFP_KERNEL); > > if (!inst) > > return -ENOMEM; > > @@ -288,14 +303,14 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id) > > #if defined(CONFIG_OF_DYNAMIC) > > static int venus_add_video_core(struct venus_core *core, const char *node_name, > > - const char *compat) > > + const char *compat, const struct venus_min_fw *ver) > > { > > struct of_changeset *ocs = core->ocs; > > struct device *dev = core->dev; > > struct device_node *np, *enp; > > int ret; > > - if (!node_name) > > + if (!node_name || !venus_fw_supports_codec(core, ver)) > > return 0; > > enp = of_find_node_by_name(dev->of_node, node_name); > > @@ -330,11 +345,13 @@ static int venus_add_dynamic_nodes(struct venus_core *core) > > of_changeset_init(core->ocs); > > - ret = venus_add_video_core(core, core->res->dec_nodename, "venus-decoder"); > > + ret = venus_add_video_core(core, core->res->dec_nodename, "venus-decoder", > > + core->res->dec_minfw); > > if (ret) > > goto err; > > - ret = venus_add_video_core(core, core->res->enc_nodename, "venus-encoder"); > > + ret = venus_add_video_core(core, core->res->enc_nodename, "venus-encoder", > > + core->res->enc_minfw); > > if (ret) > > goto err; > > @@ -363,6 +380,9 @@ static void venus_remove_dynamic_nodes(struct venus_core *core) > > #else > > static int venus_add_dynamic_nodes(struct venus_core *core) > > { > > + WARN_ONCE(core->res->enc_minfw || core->res->dec_minfw, > > + "Feature not supported"); > > + > > return 0; > > } > > @@ -432,7 +452,7 @@ static int venus_probe(struct platform_device *pdev) > > IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > "venus", core); > > if (ret) > > - goto err_core_put; > > + goto err_hfi_destroy; > > venus_assign_register_offsets(core); > > @@ -448,19 +468,9 @@ static int venus_probe(struct platform_device *pdev) > > if (ret < 0) > > goto err_runtime_disable; > > - if (core->res->dec_nodename || core->res->enc_nodename) { > > - ret = venus_add_dynamic_nodes(core); > > - if (ret) > > - goto err_runtime_disable; > > - } > > - > > - ret = of_platform_populate(dev->of_node, NULL, NULL, dev); > > - if (ret) > > - goto err_remove_dynamic_nodes; > > - > > ret = venus_firmware_init(core); > > if (ret) > > - goto err_of_depopulate; > > + goto err_runtime_disable; > > ret = venus_boot(core); > > if (ret) > > @@ -474,34 +484,46 @@ static int venus_probe(struct platform_device *pdev) > > if (ret) > > goto err_venus_shutdown; > > - ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_DEC); > > + if (core->res->dec_nodename || core->res->enc_nodename) { > > + ret = venus_add_dynamic_nodes(core); > > + if (ret) > > + goto err_core_deinit; > > + } > > + > > + ret = of_platform_populate(dev->of_node, NULL, NULL, dev); > > if (ret) > > - goto err_core_deinit; > > + goto err_remove_dynamic_nodes; > > + > > + ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_DEC, > > + core->res->dec_minfw); > > + if (ret) > > + goto err_of_depopulate; > > - ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_ENC); > > + ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_ENC, > > + core->res->enc_minfw); > > if (ret) > > - goto err_core_deinit; > > + goto err_of_depopulate; > > ret = pm_runtime_put_sync(dev); > > if (ret) { > > pm_runtime_get_noresume(dev); > > - goto err_core_deinit; > > + goto err_of_depopulate; > > } > > venus_dbgfs_init(core); > > return 0; > > +err_of_depopulate: > > + of_platform_depopulate(dev); > > +err_remove_dynamic_nodes: > > + venus_remove_dynamic_nodes(core); > > err_core_deinit: > > hfi_core_deinit(core, false); > > err_venus_shutdown: > > venus_shutdown(core); > > err_firmware_deinit: > > venus_firmware_deinit(core); > > -err_of_depopulate: > > - of_platform_depopulate(dev); > > -err_remove_dynamic_nodes: > > - venus_remove_dynamic_nodes(core); > > err_runtime_disable: > > pm_runtime_put_noidle(dev); > > pm_runtime_disable(dev); > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > > index 5b1ba1c69adb..3af8386b78be 100644 > > --- a/drivers/media/platform/qcom/venus/core.h > > +++ b/drivers/media/platform/qcom/venus/core.h > > @@ -50,6 +50,12 @@ struct bw_tbl { > > u32 peak_10bit; > > }; > > +struct venus_min_fw { > > + u32 major; > > + u32 minor; > > + u32 rev; > > +}; > > I'd call this venus_firmware_version I guess you right- doing so will enable future extensibility if we need to do some different version checks. ok. > > > + > > enum vpu_version { > > VPU_VERSION_AR50, > > VPU_VERSION_AR50_LITE, > > @@ -92,6 +98,8 @@ struct venus_resources { > > u32 cp_nonpixel_start; > > u32 cp_nonpixel_size; > > const char *fwname; > > + const struct venus_min_fw *enc_minfw; > > + const struct venus_min_fw *dec_minfw; > > and then I'd do as you have done here, indicate that the struct > venus_firmware_version is a *enc_min_fw_ver; ack > > > const char *enc_nodename; > > const char *dec_nodename; > > };