Re: [PATCH v5 18/21] drm/tegra: Allocate per-engine channel in core code

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On Mon, Jan 11, 2021 at 03:00:16PM +0200, Mikko Perttunen wrote:
> To avoid duplication, allocate the per-engine shared channel in the
> core code instead. Once MLOCKs are implemented on Host1x side, we
> can also update this to avoid allocating a shared channel when
> MLOCKs are enabled.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 11 +++++++++++
>  drivers/gpu/drm/tegra/drm.h |  4 ++++
>  2 files changed, 15 insertions(+)

It'd be helpful if the commit message explained what these per-engine
shared channels are used for.

> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index cd81b52a9e06..afd3f143c5e0 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -880,6 +880,14 @@ static const struct drm_driver tegra_drm_driver = {
>  int tegra_drm_register_client(struct tegra_drm *tegra,
>  			      struct tegra_drm_client *client)
>  {
> +	/*
> +	 * When MLOCKs are implemented, change to allocate a shared channel
> +	 * only when MLOCKs are disabled.
> +	 */
> +	client->shared_channel = host1x_channel_request(&client->base);
> +	if (!client->shared_channel)
> +		return -EBUSY;
> +
>  	mutex_lock(&tegra->clients_lock);
>  	list_add_tail(&client->list, &tegra->clients);
>  	client->drm = tegra;
> @@ -896,6 +904,9 @@ int tegra_drm_unregister_client(struct tegra_drm *tegra,
>  	client->drm = NULL;
>  	mutex_unlock(&tegra->clients_lock);
>  
> +	if (client->shared_channel)
> +		host1x_channel_put(client->shared_channel);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index f38de08e0c95..0f38f159aa8e 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -87,8 +87,12 @@ struct tegra_drm_client {
>  	struct list_head list;
>  	struct tegra_drm *drm;
>  
> +	/* Set by driver */
>  	unsigned int version;
>  	const struct tegra_drm_client_ops *ops;
> +
> +	/* Set by TegraDRM core */
> +	struct host1x_channel *shared_channel;

Perhaps reorder this so that the core-initialized fields are closer to
the top and the client-initialized fields are closer to the bottom? That
seems like a more natural order.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]