Re: [RFC PATCH v3 02/11] cgroup: Add mechanism to register DRM devices

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Wed, Jun 26, 2019 at 11:05:13AM -0400, Kenny Ho wrote:
> Change-Id: I908ee6975ea0585e4c30eafde4599f87094d8c65
> Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>

Why the separate, explicit registration step? I think a simpler design for
drivers would be that we set up cgroups if there's anything to be
controlled, and then for GEM drivers the basic GEM stuff would be set up
automically (there's really no reason not to I think).

Also tying to the minor is a bit funky, since we have multiple of these.
Need to make sure were at least consistent with whether we use the primary
or render minor - I'd always go with the primary one like you do here.

> ---
>  include/drm/drm_cgroup.h   |  24 ++++++++
>  include/linux/cgroup_drm.h |  10 ++++
>  kernel/cgroup/drm.c        | 116 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 150 insertions(+)
>  create mode 100644 include/drm/drm_cgroup.h
> 
> diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h
> new file mode 100644
> index 000000000000..ddb9eab64360
> --- /dev/null
> +++ b/include/drm/drm_cgroup.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: MIT
> + * Copyright 2019 Advanced Micro Devices, Inc.
> + */
> +#ifndef __DRM_CGROUP_H__
> +#define __DRM_CGROUP_H__
> +
> +#ifdef CONFIG_CGROUP_DRM
> +
> +int drmcgrp_register_device(struct drm_device *device);
> +
> +int drmcgrp_unregister_device(struct drm_device *device);
> +
> +#else
> +static inline int drmcgrp_register_device(struct drm_device *device)
> +{
> +	return 0;
> +}
> +
> +static inline int drmcgrp_unregister_device(struct drm_device *device)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_CGROUP_DRM */
> +#endif /* __DRM_CGROUP_H__ */
> diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h
> index 9928e60037a5..27497f786c93 100644
> --- a/include/linux/cgroup_drm.h
> +++ b/include/linux/cgroup_drm.h
> @@ -6,10 +6,20 @@
>  
>  #ifdef CONFIG_CGROUP_DRM
>  
> +#include <linux/mutex.h>
>  #include <linux/cgroup.h>
> +#include <drm/drm_file.h>
> +
> +/* limit defined per the way drm_minor_alloc operates */
> +#define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)
> +
> +struct drmcgrp_device_resource {
> +	/* for per device stats */
> +};
>  
>  struct drmcgrp {
>  	struct cgroup_subsys_state	css;
> +	struct drmcgrp_device_resource	*dev_resources[MAX_DRM_DEV];
>  };
>  
>  static inline struct drmcgrp *css_drmcgrp(struct cgroup_subsys_state *css)
> diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c
> index 66cb1dda023d..7da6e0d93991 100644
> --- a/kernel/cgroup/drm.c
> +++ b/kernel/cgroup/drm.c
> @@ -1,28 +1,99 @@
>  // SPDX-License-Identifier: MIT
>  // Copyright 2019 Advanced Micro Devices, Inc.
> +#include <linux/export.h>
>  #include <linux/slab.h>
>  #include <linux/cgroup.h>
> +#include <linux/fs.h>
> +#include <linux/seq_file.h>
> +#include <linux/mutex.h>
>  #include <linux/cgroup_drm.h>
> +#include <linux/kernel.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_cgroup.h>
> +
> +static DEFINE_MUTEX(drmcgrp_mutex);
> +
> +struct drmcgrp_device {
> +	struct drm_device	*dev;
> +	struct mutex		mutex;
> +};
> +
> +/* indexed by drm_minor for access speed */
> +static struct drmcgrp_device	*known_drmcgrp_devs[MAX_DRM_DEV];
> +
> +static int max_minor;

Uh no global stuff like this please. Or some explanation in the commit
message why we really cant avoid this.

> +
>  
>  static struct drmcgrp *root_drmcgrp __read_mostly;
>  
>  static void drmcgrp_css_free(struct cgroup_subsys_state *css)
>  {
>  	struct drmcgrp *drmcgrp = css_drmcgrp(css);
> +	int i;
> +
> +	for (i = 0; i <= max_minor; i++) {
> +		if (drmcgrp->dev_resources[i] != NULL)
> +			kfree(drmcgrp->dev_resources[i]);
> +	}
>  
>  	kfree(drmcgrp);
>  }
>  
> +static inline int init_drmcgrp_single(struct drmcgrp *drmcgrp, int minor)
> +{
> +	struct drmcgrp_device_resource *ddr = drmcgrp->dev_resources[minor];
> +
> +	if (ddr == NULL) {
> +		ddr = kzalloc(sizeof(struct drmcgrp_device_resource),
> +			GFP_KERNEL);
> +
> +		if (!ddr)
> +			return -ENOMEM;
> +
> +		drmcgrp->dev_resources[minor] = ddr;
> +	}
> +
> +	/* set defaults here */
> +
> +	return 0;
> +}
> +
> +static inline int init_drmcgrp(struct drmcgrp *drmcgrp, struct drm_device *dev)
> +{
> +	int rc = 0;
> +	int i;
> +
> +	if (dev != NULL) {
> +		rc = init_drmcgrp_single(drmcgrp, dev->primary->index);
> +		return rc;
> +	}
> +
> +	for (i = 0; i <= max_minor; i++) {
> +		rc = init_drmcgrp_single(drmcgrp, i);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct cgroup_subsys_state *
>  drmcgrp_css_alloc(struct cgroup_subsys_state *parent_css)
>  {
>  	struct drmcgrp *parent = css_drmcgrp(parent_css);
>  	struct drmcgrp *drmcgrp;
> +	int rc;
>  
>  	drmcgrp = kzalloc(sizeof(struct drmcgrp), GFP_KERNEL);
>  	if (!drmcgrp)
>  		return ERR_PTR(-ENOMEM);
>  
> +	rc = init_drmcgrp(drmcgrp, NULL);
> +	if (rc) {
> +		drmcgrp_css_free(&drmcgrp->css);
> +		return ERR_PTR(rc);
> +	}
> +
>  	if (!parent)
>  		root_drmcgrp = drmcgrp;
>  
> @@ -40,3 +111,48 @@ struct cgroup_subsys drm_cgrp_subsys = {
>  	.legacy_cftypes	= files,
>  	.dfl_cftypes	= files,
>  };
> +
> +int drmcgrp_register_device(struct drm_device *dev)

Imo this should be done as part of drm_dev_register (maybe only if the
driver has set up a controller or something). Definitely with the
unregister logic below. Also anything used by drivers needs kerneldoc.


> +{
> +	struct drmcgrp_device *ddev;
> +
> +	ddev = kzalloc(sizeof(struct drmcgrp_device), GFP_KERNEL);
> +	if (!ddev)
> +		return -ENOMEM;
> +
> +	ddev->dev = dev;
> +	mutex_init(&ddev->mutex);
> +
> +	mutex_lock(&drmcgrp_mutex);
> +	known_drmcgrp_devs[dev->primary->index] = ddev;
> +	max_minor = max(max_minor, dev->primary->index);
> +	mutex_unlock(&drmcgrp_mutex);
> +
> +	/* init cgroups created before registration (i.e. root cgroup) */
> +	if (root_drmcgrp != NULL) {
> +		struct cgroup_subsys_state *pos;
> +		struct drmcgrp *child;
> +
> +		rcu_read_lock();
> +		css_for_each_descendant_pre(pos, &root_drmcgrp->css) {
> +			child = css_drmcgrp(pos);
> +			init_drmcgrp(child, dev);
> +		}
> +		rcu_read_unlock();

I have no idea, but is this guaranteed to get them all?
-Daniel

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drmcgrp_register_device);
> +
> +int drmcgrp_unregister_device(struct drm_device *dev)
> +{
> +	mutex_lock(&drmcgrp_mutex);
> +
> +	kfree(known_drmcgrp_devs[dev->primary->index]);
> +	known_drmcgrp_devs[dev->primary->index] = NULL;
> +
> +	mutex_unlock(&drmcgrp_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drmcgrp_unregister_device);
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx