diff mbox series

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

Message ID 20240612173213.42827-4-sebastian.reichel@collabora.com (mailing list archive)
State Superseded
Headers
Series RK3588 VEPU121/VPU121 support |

Commit Message

Sebastian Reichel June 12, 2024, 5:15 p.m. UTC 
  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@collabora.com>
---
 .../media/platform/verisilicon/hantro_drv.c   | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Heiko Stübner June 12, 2024, 6:08 p.m. UTC | #1 
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@collabora.com>
> ---
>  .../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 ;-)


Heiko
Sebastian Reichel June 12, 2024, 10:44 p.m. UTC | #2 
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@collabora.com>
> > ---
> >  .../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
Heiko Stübner June 13, 2024, 8:01 a.m. UTC | #3 
Am Donnerstag, 13. Juni 2024, 00:44:38 CEST schrieb 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@collabora.com>
> > > ---
> > >  .../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.

I meant, you're doing hantro_disable_multicore() here also for everyone
else (i.MX etc), hence I'd like that to be a separate commit in this
series like:

----- 8< ------
media: hantro: Disable multi-core handling for the time being

The VSI doc for the Hantro codec describes the grouping of up to 4 instances.
The kernel currently doesn't handle multi-core processing .... foo bar ....
----- 8< ------

And then add rk3588 support on top of that.


Heiko
diff mbox series

Patch

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;
+
 	/*
 	 * Support for nxp,imx8mq-vpu is kept for backwards compatibility
 	 * but it's deprecated. Please update your DTS file to use