Re: [PATCH 01/10] drm/xe: Allocate dedicated workqueue for SR-IOV workers

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on czw [2023-gru-28 00:58:29 +0100]:
> We plan to use several workers where we might be running long
> operations. Allocate dedicated workqueue to avoid undesired
> interaction with non-virtualized workers.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       |  4 +++
>  drivers/gpu/drm/xe/xe_device_types.h |  2 ++
>  drivers/gpu/drm/xe/xe_sriov.c        | 37 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_sriov.h        |  1 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 86867d42d532..004e65544e8d 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -438,6 +438,10 @@ int xe_device_probe(struct xe_device *xe)
>  
>  	xe_pat_init_early(xe);
>  
> +	err = xe_sriov_init(xe);
> +	if (err)
> +		return err;
> +
>  	xe->info.mem_region_mask = 1;
>  	err = xe_display_init_nommio(xe);
>  	if (err)
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 71f23ac365e6..163d889d407b 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -321,6 +321,8 @@ struct xe_device {
>  	struct {
>  		/** @sriov.__mode: SR-IOV mode (Don't access directly!) */
>  		enum xe_sriov_mode __mode;
> +		/** @sriov.wq: workqueue used by the virtualization workers */
> +		struct workqueue_struct *wq;
>  	} sriov;
>  
>  	/** @clients: drm clients info */
> diff --git a/drivers/gpu/drm/xe/xe_sriov.c b/drivers/gpu/drm/xe/xe_sriov.c
> index 42a0e0c917a0..4960e8f461c8 100644
> --- a/drivers/gpu/drm/xe/xe_sriov.c
> +++ b/drivers/gpu/drm/xe/xe_sriov.c
> @@ -3,6 +3,8 @@
>   * Copyright © 2023 Intel Corporation
>   */
>  
> +#include <drm/drm_managed.h>
> +
>  #include "xe_assert.h"
>  #include "xe_sriov.h"
>  
> @@ -53,3 +55,38 @@ void xe_sriov_probe_early(struct xe_device *xe, bool has_sriov)
>  		drm_info(&xe->drm, "Running in %s mode\n",
>  			 xe_sriov_mode_to_string(xe_device_sriov_mode(xe)));
>  }
> +
> +static void fini_sriov(struct drm_device *drm, void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	destroy_workqueue(xe->sriov.wq);
> +	xe->sriov.wq = NULL;
> +}
> +
> +/**
> + * xe_sriov_init - Initialize SR-IOV specific data.
> + * @xe: the &xe_device to initialize
> + *
> + * While most of the initialization errors would be fatal for the Virtual
> + * Function (VF) device driver, in case of the Physical Function (PF) we
> + * might just limit the PF functionality and continue run as a native
> + * (non-virtualized) driver.
> + *

The description suggests a functionality that has not yet been introduced
in this series.

> + * In this function we just create dedicated workqueue that will be used
> + * by the SR-IOV specific workers.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int xe_sriov_init(struct xe_device *xe)
> +{
> +	if (!IS_SRIOV(xe))
> +		return 0;
> +
> +	xe_assert(xe, !xe->sriov.wq);
> +	xe->sriov.wq = alloc_workqueue("xe-sriov-wq", 0, 0);
> +	if (!xe->sriov.wq)
> +		return -ENOMEM;
> +
> +	return drmm_add_action_or_reset(&xe->drm, fini_sriov, xe);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_sriov.h b/drivers/gpu/drm/xe/xe_sriov.h
> index 5af73a3172b0..1545552162c9 100644
> --- a/drivers/gpu/drm/xe/xe_sriov.h
> +++ b/drivers/gpu/drm/xe/xe_sriov.h
> @@ -13,6 +13,7 @@
>  const char *xe_sriov_mode_to_string(enum xe_sriov_mode mode);
>  
>  void xe_sriov_probe_early(struct xe_device *xe, bool has_sriov);
> +int xe_sriov_init(struct xe_device *xe);
>  
>  static inline enum xe_sriov_mode xe_device_sriov_mode(struct xe_device *xe)
>  {

I would suggest remove the comment I mentioned so as not to confuse others until this
functionality actually appears.
However, the rest of the code is ok for me:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>



> -- 
> 2.25.1
> 

--