Re: [PATCH v5 3/5] media: hantro: Add RK3588 VEPU121 support

[Sebastian Reichel]

Hi,

On Wed, Jun 12, 2024 at 08:08:51PM GMT, Heiko Stübner wrote:
> Am Mittwoch, 12. Juni 2024, 19:15:43 CEST schrieb Sebastian Reichel:
> > Avoid exposing each of the 4 Hantro H1 cores separately to userspace.
> > For now just expose the first one.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > ---
> >  .../media/platform/verisilicon/hantro_drv.c   | 38 +++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> > index 34b123dafd89..b722a20c5fe3 100644
> > --- a/drivers/media/platform/verisilicon/hantro_drv.c
> > +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> > @@ -722,6 +722,7 @@ static const struct of_device_id of_hantro_match[] = {
> >  	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> >  	{ .compatible = "rockchip,rk3568-vepu", .data = &rk3568_vepu_variant, },
> >  	{ .compatible = "rockchip,rk3568-vpu", .data = &rk3568_vpu_variant, },
> > +	{ .compatible = "rockchip,rk3588-vepu121", .data = &rk3568_vpu_variant, },
> >  	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
> >  #endif
> >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > @@ -992,6 +993,39 @@ static const struct media_device_ops hantro_m2m_media_ops = {
> >  	.req_queue = v4l2_m2m_request_queue,
> >  };
> >  
> > +/*
> > + * Some SoCs, like RK3588 have multiple identical Hantro cores, but the
> > + * kernel is currently missing support for multi-core handling. Exposing
> > + * separate devices for each core to userspace is bad, since that does
> > + * not allow scheduling tasks properly (and creates ABI). With this workaround
> > + * the driver will only probe for the first core and early exit for the other
> > + * cores. Once the driver gains multi-core support, the same technique
> > + * for detecting the main core can be used to cluster all cores together.
> > + */
> > +static int hantro_disable_multicore(struct hantro_dev *vpu)
> > +{
> > +	const char *compatible;
> > +	struct device_node *node;
> > +	int ret;
> > +
> > +	/* Intentionally ignores the fallback strings */
> > +	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* first compatible node found from the root node is considered the main core */
> > +	node = of_find_compatible_node(NULL, NULL, compatible);
> > +	if (!node)
> > +		return -EINVAL; /* broken DT? */
> > +
> > +	if (vpu->dev->of_node != node) {
> > +		dev_info(vpu->dev, "missing multi-core support, ignoring this instance\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int hantro_probe(struct platform_device *pdev)
> >  {
> >  	const struct of_device_id *match;
> > @@ -1011,6 +1045,10 @@ static int hantro_probe(struct platform_device *pdev)
> >  	match = of_match_node(of_hantro_match, pdev->dev.of_node);
> >  	vpu->variant = match->data;
> >  
> > +	ret = hantro_disable_multicore(vpu);
> > +	if (ret)
> > +		return ret;
> > +
> 
> I think this might be better as two patches?
> 
> As this patch stands, the disable-multicore handling is done for _all_
> hantro variants, so part of me wants this to be labeled as such.
> 
> The whole reasoning is completely ok, but somehow having this under
> the "add rk3588" umbrella feels strange ;-)

I can do that, but the 'rockchip,rk3588-vepu121' part is only needed
because of the multicore handling. If the kernel already had this bit
in the past, the RK3568 compatible could be used for RK3588 (as a
fallback compatible), just like for VPU121.

-- Sebastian

Attachment: signature.asc
Description: PGP signature