linux-kernel - Re: [PATCH v2 7/7] media: platform: qcom/iris: add sm8650 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <noowenzvhkcmx7cmwbnqqepuabown6taznmuuomw6lp7mo6tam@zihcgcjsmt73>
Date: Fri, 7 Mar 2025 10:38:05 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, 
	Vikash Garodia <quic_vgarodia@...cinc.com>, Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 7/7] media: platform: qcom/iris: add sm8650 support

On Thu, Mar 06, 2025 at 06:46:06PM +0530, Dikshita Agarwal wrote:
> 
> 
> On 3/6/2025 12:35 AM, Neil Armstrong wrote:
> > Add support for the SM8650 platform by re-using the SM8550
> > definitions and using the vpu33 ops.
> > 
> > The SM8650/vpu33 requires more reset lines, but the H.284
> > decoder capabilities are identical.
> > 
> > Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
> > ---
> >  .../platform/qcom/iris/iris_platform_common.h      |  1 +
> >  .../platform/qcom/iris/iris_platform_sm8550.c      | 64 ++++++++++++++++++++++
> >  drivers/media/platform/qcom/iris/iris_probe.c      |  4 ++
> >  3 files changed, 69 insertions(+)
> > 
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > index fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > @@ -35,6 +35,7 @@ enum pipe_type {
> >  
> >  extern struct iris_platform_data sm8250_data;
> >  extern struct iris_platform_data sm8550_data;
> > +extern struct iris_platform_data sm8650_data;
> >  
> >  enum platform_clk_type {
> >  	IRIS_AXI_CLK,
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> > index 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> > @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = {
> >  
> >  static const char * const sm8550_clk_reset_table[] = { "bus" };
> >  
> > +static const char * const sm8650_clk_reset_table[] = { "bus", "core" };
> > +
> > +static const char * const sm8650_controller_reset_table[] = { "xo" };
> > +
> >  static const struct bw_info sm8550_bw_table_dec[] = {
> >  	{ ((4096 * 2160) / 256) * 60, 1608000 },
> >  	{ ((4096 * 2160) / 256) * 30,  826000 },
> > @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = {
> >  	.dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
> >  	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
> >  };
> > +
> > +/*
> > + * Shares most of SM8550 data except:
> > + * - vpu_ops to iris_vpu33_ops
> > + * - clk_rst_tbl to sm8650_clk_reset_table
> > + * - controller_rst_tbl to sm8650_controller_reset_table
> > + * - fwname to "qcom/vpu/vpu33_p4.mbn"
> > + */
> > +struct iris_platform_data sm8650_data = {
[...]
> > +
> > +	.dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl,
> > +	.dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl),
> > +	.dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
> > +	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl),
> > +};
> This approach looks good to me, reusing the platform data like this keeps
> the code cleaner and avoids duplication. I think this is a good way to
> handle the differences while sharing the common parts.

Just to share some thoughts. We had this kind of data-sharing in the DPU
driver. It looked good in the beginning, but at some point we found
ourselves stuck with the platform data being named semi-randomly
(following the first-added SoC instead of the first-in-family).

Modifying "catalog" data became troublesome as it was no longer clear,
which chipsets are going to be affected by the change. So, after some
thought we ended up duplicating data all over the catalog files for the
sake of them being easy to modify. There are some ideas on how to
simplify that, but for now we have (almost) full data set for each SoC.

For example, imagine somebody adding sm8450 support and sm8450 reusing
sm8550 data. It would be a bit troublesome to remember that changing
sm8550 data would affect sm8450. And maybe some of the SAR, SA or
QCM/QCS platforms. I think you see the point.

If you are to explore the data sharing solution, I'd suggest exploring
an idea similar to the DPU catalog: start naming each of the platform
files with some kind of generation-like ID. In case of DPU it was easy
as each DPU instance has unique version. For the Iris devices it might
be as easy as vpu30_sm8550, vpu33_sm8650, etc. It might make it easier
to make assumptions and derive common pieces of data.

-- 
With best wishes
Dmitry