From: Sui Jingfeng <[email protected]>
There are numerous members in the struct etnaviv_drm_private, which are
shared by all GPU core. This patch introduces two dedicated functions for
the construction and destruction of the instances of this structure.
The goal is to keep its members from leaking to the outside. The code
needed for error handling can also be simplified.
Cc: Lucas Stach <[email protected]>
Cc: Christian Gmeiner <[email protected]>
Cc: Philipp Zabel <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Daniel Vetter <[email protected]>
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 73 +++++++++++++++++----------
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
2 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index cec005035d0e..6a048be02857 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -24,9 +24,47 @@
#include "etnaviv_perfmon.h"
/*
- * DRM operations:
+ * etnaviv private data construction and destructions:
*/
+static struct etnaviv_drm_private *
+etnaviv_alloc_private(struct device *dev, struct drm_device *drm)
+{
+ struct etnaviv_drm_private *priv;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return ERR_PTR(-ENOMEM);
+
+ priv->drm = drm;
+
+ xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
+
+ mutex_init(&priv->gem_lock);
+ INIT_LIST_HEAD(&priv->gem_list);
+ priv->num_gpus = 0;
+ priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
+ priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
+ if (IS_ERR(priv->cmdbuf_suballoc)) {
+ kfree(priv);
+ dev_err(dev, "Failed to create cmdbuf suballocator\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ return priv;
+}
+
+static void etnaviv_free_private(struct etnaviv_drm_private *priv)
+{
+ if (!priv)
+ return;
+
+ etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
+
+ xa_destroy(&priv->active_contexts);
+
+ kfree(priv);
+}
static void load_gpu(struct drm_device *dev)
{
@@ -511,35 +549,21 @@ static int etnaviv_bind(struct device *dev)
if (IS_ERR(drm))
return PTR_ERR(drm);
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv) {
- dev_err(dev, "failed to allocate private data\n");
- ret = -ENOMEM;
+ priv = etnaviv_alloc_private(dev, drm);
+ if (IS_ERR(priv)) {
+ ret = PTR_ERR(priv);
goto out_put;
}
+
drm->dev_private = priv;
dma_set_max_seg_size(dev, SZ_2G);
- xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
-
- mutex_init(&priv->gem_lock);
- INIT_LIST_HEAD(&priv->gem_list);
- priv->num_gpus = 0;
- priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
-
- priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
- if (IS_ERR(priv->cmdbuf_suballoc)) {
- dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
- ret = PTR_ERR(priv->cmdbuf_suballoc);
- goto out_free_priv;
- }
-
dev_set_drvdata(dev, drm);
ret = component_bind_all(dev, drm);
if (ret < 0)
- goto out_destroy_suballoc;
+ goto out_free_priv;
load_gpu(drm);
@@ -551,10 +575,8 @@ static int etnaviv_bind(struct device *dev)
out_unbind:
component_unbind_all(dev, drm);
-out_destroy_suballoc:
- etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
out_free_priv:
- kfree(priv);
+ etnaviv_free_private(priv);
out_put:
drm_dev_put(drm);
@@ -570,12 +592,9 @@ static void etnaviv_unbind(struct device *dev)
component_unbind_all(dev, drm);
- etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
-
- xa_destroy(&priv->active_contexts);
+ etnaviv_free_private(priv);
drm->dev_private = NULL;
- kfree(priv);
drm_dev_put(drm);
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index b3eb1662e90c..e58f82e698de 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -35,6 +35,7 @@ struct etnaviv_file_private {
};
struct etnaviv_drm_private {
+ struct drm_device *drm;
int num_gpus;
struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
gfp_t shm_gfp_mask;
--
2.25.1
Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <[email protected]>
>
> There are numerous members in the struct etnaviv_drm_private, which are
> shared by all GPU core. This patch introduces two dedicated functions for
> the construction and destruction of the instances of this structure.
> The goal is to keep its members from leaking to the outside. The code
> needed for error handling can also be simplified.
>
> Cc: Lucas Stach <[email protected]>
> Cc: Christian Gmeiner <[email protected]>
> Cc: Philipp Zabel <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 73 +++++++++++++++++----------
> drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
> 2 files changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index cec005035d0e..6a048be02857 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -24,9 +24,47 @@
> #include "etnaviv_perfmon.h"
>
> /*
> - * DRM operations:
> + * etnaviv private data construction and destructions:
> */
> +static struct etnaviv_drm_private *
> +etnaviv_alloc_private(struct device *dev, struct drm_device *drm)
> +{
> + struct etnaviv_drm_private *priv;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return ERR_PTR(-ENOMEM);
> +
> + priv->drm = drm;
That's an unrelated change that you rely on in later patches. If this
is needed at all it needs to be in a separate patch with a explanation
on why it is needed.
Regards,
Lucas
> +
> + xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
> +
> + mutex_init(&priv->gem_lock);
> + INIT_LIST_HEAD(&priv->gem_list);
> + priv->num_gpus = 0;
> + priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>
> + priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
> + if (IS_ERR(priv->cmdbuf_suballoc)) {
> + kfree(priv);
> + dev_err(dev, "Failed to create cmdbuf suballocator\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + return priv;
> +}
> +
> +static void etnaviv_free_private(struct etnaviv_drm_private *priv)
> +{
> + if (!priv)
> + return;
> +
> + etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
> +
> + xa_destroy(&priv->active_contexts);
> +
> + kfree(priv);
> +}
>
> static void load_gpu(struct drm_device *dev)
> {
> @@ -511,35 +549,21 @@ static int etnaviv_bind(struct device *dev)
> if (IS_ERR(drm))
> return PTR_ERR(drm);
>
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> - dev_err(dev, "failed to allocate private data\n");
> - ret = -ENOMEM;
> + priv = etnaviv_alloc_private(dev, drm);
> + if (IS_ERR(priv)) {
> + ret = PTR_ERR(priv);
> goto out_put;
> }
> +
> drm->dev_private = priv;
>
> dma_set_max_seg_size(dev, SZ_2G);
>
> - xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
> -
> - mutex_init(&priv->gem_lock);
> - INIT_LIST_HEAD(&priv->gem_list);
> - priv->num_gpus = 0;
> - priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> -
> - priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
> - if (IS_ERR(priv->cmdbuf_suballoc)) {
> - dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
> - ret = PTR_ERR(priv->cmdbuf_suballoc);
> - goto out_free_priv;
> - }
> -
> dev_set_drvdata(dev, drm);
>
> ret = component_bind_all(dev, drm);
> if (ret < 0)
> - goto out_destroy_suballoc;
> + goto out_free_priv;
>
> load_gpu(drm);
>
> @@ -551,10 +575,8 @@ static int etnaviv_bind(struct device *dev)
>
> out_unbind:
> component_unbind_all(dev, drm);
> -out_destroy_suballoc:
> - etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
> out_free_priv:
> - kfree(priv);
> + etnaviv_free_private(priv);
> out_put:
> drm_dev_put(drm);
>
> @@ -570,12 +592,9 @@ static void etnaviv_unbind(struct device *dev)
>
> component_unbind_all(dev, drm);
>
> - etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
> -
> - xa_destroy(&priv->active_contexts);
> + etnaviv_free_private(priv);
>
> drm->dev_private = NULL;
> - kfree(priv);
>
> drm_dev_put(drm);
> }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index b3eb1662e90c..e58f82e698de 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -35,6 +35,7 @@ struct etnaviv_file_private {
> };
>
> struct etnaviv_drm_private {
> + struct drm_device *drm;
> int num_gpus;
> struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
> gfp_t shm_gfp_mask;
Hi,
On 2023/6/21 17:22, Lucas Stach wrote:
> Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <[email protected]>
>>
>> There are numerous members in the struct etnaviv_drm_private, which are
>> shared by all GPU core. This patch introduces two dedicated functions for
>> the construction and destruction of the instances of this structure.
>> The goal is to keep its members from leaking to the outside. The code
>> needed for error handling can also be simplified.
>>
>> Cc: Lucas Stach <[email protected]>
>> Cc: Christian Gmeiner <[email protected]>
>> Cc: Philipp Zabel <[email protected]>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: Daniel Vetter <[email protected]>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 73 +++++++++++++++++----------
>> drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
>> 2 files changed, 47 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index cec005035d0e..6a048be02857 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -24,9 +24,47 @@
>> #include "etnaviv_perfmon.h"
>>
>> /*
>> - * DRM operations:
>> + * etnaviv private data construction and destructions:
>> */
>> +static struct etnaviv_drm_private *
>> +etnaviv_alloc_private(struct device *dev, struct drm_device *drm)
>> +{
>> + struct etnaviv_drm_private *priv;
>> +
>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + priv->drm = drm;
> That's an unrelated change that you rely on in later patches. If this
> is needed at all it needs to be in a separate patch with a explanation
> on why it is needed.
It helps the etnaviv_drm_unbind() function fetch the pointer to the
struct drm_device,
etnaviv_drm_unbind() became a pure function which don't need to
reference external variables.
The real rationale is that when we made the single 3D GPU core the be
the master device,
We no longer create the virtual master. We lost a place to store the
pointer to
struct drm_device.
We have only one struct device across the whole driver.
There only one dev->driver_data instance in the whole driver,
The etnaviv_gpu_driver_create() function will call dev_set_drvdata(dev, gpu)
function to store the pointer to the instance(struct etnaviv_gpu).
The seat is taken, there is no where to store the pointer to struct
drm_device.
As the drm is intended to be shared by all GPU core,
so, in the end, even for multiple GPU cores case,
this is still good thing, because the driver could fetch the pointer to
the struct drm_device
via 'struct etnaviv_drm_private *', they are something in common, that
is they are both
intended to be shared by the whole program.
> Regards,
> Lucas
>
>> +
>> + xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
>> +
>> + mutex_init(&priv->gem_lock);
>> + INIT_LIST_HEAD(&priv->gem_list);
>> + priv->num_gpus = 0;
>> + priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>>
>> + priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
>> + if (IS_ERR(priv->cmdbuf_suballoc)) {
>> + kfree(priv);
>> + dev_err(dev, "Failed to create cmdbuf suballocator\n");
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + return priv;
>> +}
>> +
>> +static void etnaviv_free_private(struct etnaviv_drm_private *priv)
>> +{
>> + if (!priv)
>> + return;
>> +
>> + etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
>> +
>> + xa_destroy(&priv->active_contexts);
>> +
>> + kfree(priv);
>> +}
>>
>> static void load_gpu(struct drm_device *dev)
>> {
>> @@ -511,35 +549,21 @@ static int etnaviv_bind(struct device *dev)
>> if (IS_ERR(drm))
>> return PTR_ERR(drm);
>>
>> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> - if (!priv) {
>> - dev_err(dev, "failed to allocate private data\n");
>> - ret = -ENOMEM;
>> + priv = etnaviv_alloc_private(dev, drm);
>> + if (IS_ERR(priv)) {
>> + ret = PTR_ERR(priv);
>> goto out_put;
>> }
>> +
>> drm->dev_private = priv;
>>
>> dma_set_max_seg_size(dev, SZ_2G);
>>
>> - xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
>> -
>> - mutex_init(&priv->gem_lock);
>> - INIT_LIST_HEAD(&priv->gem_list);
>> - priv->num_gpus = 0;
>> - priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>> -
>> - priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>> - if (IS_ERR(priv->cmdbuf_suballoc)) {
>> - dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
>> - ret = PTR_ERR(priv->cmdbuf_suballoc);
>> - goto out_free_priv;
>> - }
>> -
>> dev_set_drvdata(dev, drm);
>>
>> ret = component_bind_all(dev, drm);
>> if (ret < 0)
>> - goto out_destroy_suballoc;
>> + goto out_free_priv;
>>
>> load_gpu(drm);
>>
>> @@ -551,10 +575,8 @@ static int etnaviv_bind(struct device *dev)
>>
>> out_unbind:
>> component_unbind_all(dev, drm);
>> -out_destroy_suballoc:
>> - etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
>> out_free_priv:
>> - kfree(priv);
>> + etnaviv_free_private(priv);
>> out_put:
>> drm_dev_put(drm);
>>
>> @@ -570,12 +592,9 @@ static void etnaviv_unbind(struct device *dev)
>>
>> component_unbind_all(dev, drm);
>>
>> - etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
>> -
>> - xa_destroy(&priv->active_contexts);
>> + etnaviv_free_private(priv);
>>
>> drm->dev_private = NULL;
>> - kfree(priv);
>>
>> drm_dev_put(drm);
>> }
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>> index b3eb1662e90c..e58f82e698de 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>> @@ -35,6 +35,7 @@ struct etnaviv_file_private {
>> };
>>
>> struct etnaviv_drm_private {
>> + struct drm_device *drm;
>> int num_gpus;
>> struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
>> gfp_t shm_gfp_mask;
--
Jingfeng