diff mbox

[7/7] gpu: host1x: Track client version

Message ID 20180518123939.GA9594@ulmo (mailing list archive)
State New, archived
Headers show

Commit Message

Thierry Reding May 18, 2018, 12:39 p.m. UTC
On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
> On 05/17/2018 06:34 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Userspace needs to know the version of the interface implemented by a
> > client so it can create the proper command streams. Allow individual
> > drivers to store this version along with the client so that it can be
> > returned to userspace upon opening a channel.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   include/linux/host1x.h | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> > index 89110d896d72..57d26406bdfd 100644
> > --- a/include/linux/host1x.h
> > +++ b/include/linux/host1x.h
> > @@ -49,6 +49,7 @@ struct host1x_client_ops {
> >    * @dev: pointer to struct device backing this host1x client
> >    * @ops: host1x client operations
> >    * @class: host1x class represented by this client
> > + * @version: interface version implemented by this client
> >    * @channel: host1x channel associated with this client
> >    * @syncpts: array of syncpoints requested for this client
> >    * @num_syncpts: number of syncpoints requested for this client
> > @@ -61,6 +62,8 @@ struct host1x_client {
> >   	const struct host1x_client_ops *ops;
> >   	enum host1x_class class;
> > +	unsigned int version;
> > +
> 
> It doesn't seem to me that this fits here - Host1x doesn't provide any
> userspace interface, TegraDRM does. We will (hopefully) have clients in the
> future that use a different userspace interface, or don't have one at all.
> So this property should be on TegraDRM side instead.

I think the line is somewhat blurry. Technically it is the host1x
clients that define the version of the interface that they provide. By
interface I mean the "class" of the command stream supported. So gr2d
will define what methods are valid to use on a specific version of the
module. The same goes for gr3d and VIC. The hardware module is where
that specification lives.

So if we ever had a client that didn't provide a userspace interface via
Tegra DRM we'd still want to represent what version of the (command
stream) interface is expected. By moving this to drm/tegra, we move the
hardware specific bits into the userspace related parts, so I'm not sure
it is a really good fit.

However, I do understand where you're coming from. Ultimately where
userspace gets involved is at the ABI level that drm/tegra exposes. And
for drivers that don't expose a userspace ABI, they'd likely need to
deal with the version number internally, so storing this in host1x
clients wouldn't fit very well either.

I don't really care all that much how exactly we store the version
information so that it can be reported back to userspace, and moving it
to drm/tegra makes the code somewhat more elegant. I came up with the
below, which is completely untested but should work.

Thierry

--- >8 ---
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 4330d5d48801..5dc02340007e 100644

Comments

Mikko Perttunen May 18, 2018, 12:51 p.m. UTC | #1
On 05/18/2018 03:39 PM, Thierry Reding wrote:
> On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
>> On 05/17/2018 06:34 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Userspace needs to know the version of the interface implemented by a
>>> client so it can create the proper command streams. Allow individual
>>> drivers to store this version along with the client so that it can be
>>> returned to userspace upon opening a channel.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>    include/linux/host1x.h | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
>>> index 89110d896d72..57d26406bdfd 100644
>>> --- a/include/linux/host1x.h
>>> +++ b/include/linux/host1x.h
>>> @@ -49,6 +49,7 @@ struct host1x_client_ops {
>>>     * @dev: pointer to struct device backing this host1x client
>>>     * @ops: host1x client operations
>>>     * @class: host1x class represented by this client
>>> + * @version: interface version implemented by this client
>>>     * @channel: host1x channel associated with this client
>>>     * @syncpts: array of syncpoints requested for this client
>>>     * @num_syncpts: number of syncpoints requested for this client
>>> @@ -61,6 +62,8 @@ struct host1x_client {
>>>    	const struct host1x_client_ops *ops;
>>>    	enum host1x_class class;
>>> +	unsigned int version;
>>> +
>>
>> It doesn't seem to me that this fits here - Host1x doesn't provide any
>> userspace interface, TegraDRM does. We will (hopefully) have clients in the
>> future that use a different userspace interface, or don't have one at all.
>> So this property should be on TegraDRM side instead.
> 
> I think the line is somewhat blurry. Technically it is the host1x
> clients that define the version of the interface that they provide. By
> interface I mean the "class" of the command stream supported. So gr2d
> will define what methods are valid to use on a specific version of the
> module. The same goes for gr3d and VIC. The hardware module is where
> that specification lives.
> 
> So if we ever had a client that didn't provide a userspace interface via
> Tegra DRM we'd still want to represent what version of the (command
> stream) interface is expected. By moving this to drm/tegra, we move the
> hardware specific bits into the userspace related parts, so I'm not sure
> it is a really good fit.

The way I look at it, TegraDRM contains the engine driver 
implementations and should contain all knowledge about those - Host1x 
should know as little as possible about the engines, and as it doesn't 
need to know the interface version, we should keep that information on 
the TegraDRM side. As a comparison, we could think of Host1x as a PCI 
host controller and the engines as connected devices.

> 
> However, I do understand where you're coming from. Ultimately where
> userspace gets involved is at the ABI level that drm/tegra exposes. And
> for drivers that don't expose a userspace ABI, they'd likely need to
> deal with the version number internally, so storing this in host1x
> clients wouldn't fit very well either.
> 
> I don't really care all that much how exactly we store the version
> information so that it can be reported back to userspace, and moving it
> to drm/tegra makes the code somewhat more elegant. I came up with the
> below, which is completely untested but should work.

Yep, this looks good to me.

Thanks!
Mikko

> 
> Thierry
> 
> --- >8 ---
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 4330d5d48801..5dc02340007e 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1389,7 +1389,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data,
>   				break;
>   
>   			args->syncpts = client->base.num_syncpts;
> -			args->version = client->base.version;
> +			args->version = client->version;
>   			args->context = context->id;
>   			break;
>   		}
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 13660f6225a2..f493e0bf2172 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -105,6 +105,7 @@ struct tegra_drm_client {
>   	struct host1x_client base;
>   	struct list_head list;
>   
> +	unsigned int version;
>   	const struct tegra_drm_client_ops *ops;
>   };
>   
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 7cae8252f0fa..165a1cece039 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -196,11 +196,11 @@ static int gr2d_probe(struct platform_device *pdev)
>   	gr2d->client.base.ops = &gr2d_client_ops;
>   	gr2d->client.base.dev = dev;
>   	gr2d->client.base.class = HOST1X_CLASS_GR2D;
> -	gr2d->client.base.version = gr2d->soc->version;
>   	gr2d->client.base.syncpts = syncpts;
>   	gr2d->client.base.num_syncpts = 1;
>   
>   	INIT_LIST_HEAD(&gr2d->client.list);
> +	gr2d->client.version = gr2d->soc->version;
>   	gr2d->client.ops = &gr2d_ops;
>   
>   	err = host1x_client_register(&gr2d->client.base);
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 9bd89916d2ef..0d72e78bd6e8 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -325,11 +325,11 @@ static int gr3d_probe(struct platform_device *pdev)
>   	gr3d->client.base.ops = &gr3d_client_ops;
>   	gr3d->client.base.dev = &pdev->dev;
>   	gr3d->client.base.class = HOST1X_CLASS_GR3D;
> -	gr3d->client.base.version = gr3d->soc->version;
>   	gr3d->client.base.syncpts = syncpts;
>   	gr3d->client.base.num_syncpts = 1;
>   
>   	INIT_LIST_HEAD(&gr3d->client.list);
> +	gr3d->client.version = gr3d->soc->version;
>   	gr3d->client.ops = &gr3d_ops;
>   
>   	err = host1x_client_register(&gr3d->client.base);
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> index b9df467ca71e..9fa77405db01 100644
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -342,12 +342,12 @@ static int vic_probe(struct platform_device *pdev)
>   	vic->client.base.ops = &vic_client_ops;
>   	vic->client.base.dev = dev;
>   	vic->client.base.class = HOST1X_CLASS_VIC;
> -	vic->client.base.version = vic->config->version;
>   	vic->client.base.syncpts = syncpts;
>   	vic->client.base.num_syncpts = 1;
>   	vic->dev = dev;
>   
>   	INIT_LIST_HEAD(&vic->client.list);
> +	vic->client.version = vic->config->version;
>   	vic->client.ops = &vic_ops;
>   
>   	err = host1x_client_register(&vic->client.base);
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 6dc819b1ab64..518864cfbd16 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -62,7 +62,6 @@ struct host1x_client {
>   	const struct host1x_client_ops *ops;
>   
>   	enum host1x_class class;
> -	unsigned int version;
>   
>   	struct host1x_channel *channel;
>   
>
Dmitry Osipenko May 18, 2018, 3:59 p.m. UTC | #2
On 18.05.2018 15:39, Thierry Reding wrote:
> On Fri, May 18, 2018 at 03:21:11PM +0300, Mikko Perttunen wrote:
>> On 05/17/2018 06:34 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Userspace needs to know the version of the interface implemented by a
>>> client so it can create the proper command streams. Allow individual
>>> drivers to store this version along with the client so that it can be
>>> returned to userspace upon opening a channel.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>   include/linux/host1x.h | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
>>> index 89110d896d72..57d26406bdfd 100644
>>> --- a/include/linux/host1x.h
>>> +++ b/include/linux/host1x.h
>>> @@ -49,6 +49,7 @@ struct host1x_client_ops {
>>>    * @dev: pointer to struct device backing this host1x client
>>>    * @ops: host1x client operations
>>>    * @class: host1x class represented by this client
>>> + * @version: interface version implemented by this client
>>>    * @channel: host1x channel associated with this client
>>>    * @syncpts: array of syncpoints requested for this client
>>>    * @num_syncpts: number of syncpoints requested for this client
>>> @@ -61,6 +62,8 @@ struct host1x_client {
>>>   	const struct host1x_client_ops *ops;
>>>   	enum host1x_class class;
>>> +	unsigned int version;
>>> +
>>
>> It doesn't seem to me that this fits here - Host1x doesn't provide any
>> userspace interface, TegraDRM does. We will (hopefully) have clients in the
>> future that use a different userspace interface, or don't have one at all.
>> So this property should be on TegraDRM side instead.
> 
> I think the line is somewhat blurry. Technically it is the host1x
> clients that define the version of the interface that they provide. By
> interface I mean the "class" of the command stream supported. So gr2d
> will define what methods are valid to use on a specific version of the
> module. The same goes for gr3d and VIC. The hardware module is where
> that specification lives.
> 
> So if we ever had a client that didn't provide a userspace interface via
> Tegra DRM we'd still want to represent what version of the (command
> stream) interface is expected. By moving this to drm/tegra, we move the
> hardware specific bits into the userspace related parts, so I'm not sure
> it is a really good fit.
> 
> However, I do understand where you're coming from. Ultimately where
> userspace gets involved is at the ABI level that drm/tegra exposes. And
> for drivers that don't expose a userspace ABI, they'd likely need to
> deal with the version number internally, so storing this in host1x
> clients wouldn't fit very well either.
> 
> I don't really care all that much how exactly we store the version
> information so that it can be reported back to userspace, and moving it
> to drm/tegra makes the code somewhat more elegant. I came up with the
> below, which is completely untested but should work.
> 
> Thierry
> 
> --- >8 ---
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 4330d5d48801..5dc02340007e 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1389,7 +1389,7 @@ static int tegra_open_channel(struct drm_device *drm, void *data,
>  				break;
>  
>  			args->syncpts = client->base.num_syncpts;
> -			args->version = client->base.version;
> +			args->version = client->version;
>  			args->context = context->id;
>  			break;
>  		}
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 13660f6225a2..f493e0bf2172 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -105,6 +105,7 @@ struct tegra_drm_client {
>  	struct host1x_client base;
>  	struct list_head list;
>  
> +	unsigned int version;
>  	const struct tegra_drm_client_ops *ops;
>  };
>  
> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> index 7cae8252f0fa..165a1cece039 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -196,11 +196,11 @@ static int gr2d_probe(struct platform_device *pdev)
>  	gr2d->client.base.ops = &gr2d_client_ops;
>  	gr2d->client.base.dev = dev;
>  	gr2d->client.base.class = HOST1X_CLASS_GR2D;
> -	gr2d->client.base.version = gr2d->soc->version;
>  	gr2d->client.base.syncpts = syncpts;
>  	gr2d->client.base.num_syncpts = 1;
>  
>  	INIT_LIST_HEAD(&gr2d->client.list);
> +	gr2d->client.version = gr2d->soc->version;
>  	gr2d->client.ops = &gr2d_ops;
>  
>  	err = host1x_client_register(&gr2d->client.base);
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 9bd89916d2ef..0d72e78bd6e8 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -325,11 +325,11 @@ static int gr3d_probe(struct platform_device *pdev)
>  	gr3d->client.base.ops = &gr3d_client_ops;
>  	gr3d->client.base.dev = &pdev->dev;
>  	gr3d->client.base.class = HOST1X_CLASS_GR3D;
> -	gr3d->client.base.version = gr3d->soc->version;
>  	gr3d->client.base.syncpts = syncpts;
>  	gr3d->client.base.num_syncpts = 1;
>  
>  	INIT_LIST_HEAD(&gr3d->client.list);
> +	gr3d->client.version = gr3d->soc->version;
>  	gr3d->client.ops = &gr3d_ops;
>  
>  	err = host1x_client_register(&gr3d->client.base);
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> index b9df467ca71e..9fa77405db01 100644
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -342,12 +342,12 @@ static int vic_probe(struct platform_device *pdev)
>  	vic->client.base.ops = &vic_client_ops;
>  	vic->client.base.dev = dev;
>  	vic->client.base.class = HOST1X_CLASS_VIC;
> -	vic->client.base.version = vic->config->version;
>  	vic->client.base.syncpts = syncpts;
>  	vic->client.base.num_syncpts = 1;
>  	vic->dev = dev;
>  
>  	INIT_LIST_HEAD(&vic->client.list);
> +	vic->client.version = vic->config->version;
>  	vic->client.ops = &vic_ops;
>  
>  	err = host1x_client_register(&vic->client.base);
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 6dc819b1ab64..518864cfbd16 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -62,7 +62,6 @@ struct host1x_client {
>  	const struct host1x_client_ops *ops;
>  
>  	enum host1x_class class;
> -	unsigned int version;
>  
>  	struct host1x_channel *channel;
>  
> 

Acked-by: Dmitry Osipenko <digetx@gmail.com>
diff mbox

Patch

--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1389,7 +1389,7 @@  static int tegra_open_channel(struct drm_device *drm, void *data,
 				break;
 
 			args->syncpts = client->base.num_syncpts;
-			args->version = client->base.version;
+			args->version = client->version;
 			args->context = context->id;
 			break;
 		}
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 13660f6225a2..f493e0bf2172 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -105,6 +105,7 @@  struct tegra_drm_client {
 	struct host1x_client base;
 	struct list_head list;
 
+	unsigned int version;
 	const struct tegra_drm_client_ops *ops;
 };
 
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 7cae8252f0fa..165a1cece039 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -196,11 +196,11 @@  static int gr2d_probe(struct platform_device *pdev)
 	gr2d->client.base.ops = &gr2d_client_ops;
 	gr2d->client.base.dev = dev;
 	gr2d->client.base.class = HOST1X_CLASS_GR2D;
-	gr2d->client.base.version = gr2d->soc->version;
 	gr2d->client.base.syncpts = syncpts;
 	gr2d->client.base.num_syncpts = 1;
 
 	INIT_LIST_HEAD(&gr2d->client.list);
+	gr2d->client.version = gr2d->soc->version;
 	gr2d->client.ops = &gr2d_ops;
 
 	err = host1x_client_register(&gr2d->client.base);
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 9bd89916d2ef..0d72e78bd6e8 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -325,11 +325,11 @@  static int gr3d_probe(struct platform_device *pdev)
 	gr3d->client.base.ops = &gr3d_client_ops;
 	gr3d->client.base.dev = &pdev->dev;
 	gr3d->client.base.class = HOST1X_CLASS_GR3D;
-	gr3d->client.base.version = gr3d->soc->version;
 	gr3d->client.base.syncpts = syncpts;
 	gr3d->client.base.num_syncpts = 1;
 
 	INIT_LIST_HEAD(&gr3d->client.list);
+	gr3d->client.version = gr3d->soc->version;
 	gr3d->client.ops = &gr3d_ops;
 
 	err = host1x_client_register(&gr3d->client.base);
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index b9df467ca71e..9fa77405db01 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -342,12 +342,12 @@  static int vic_probe(struct platform_device *pdev)
 	vic->client.base.ops = &vic_client_ops;
 	vic->client.base.dev = dev;
 	vic->client.base.class = HOST1X_CLASS_VIC;
-	vic->client.base.version = vic->config->version;
 	vic->client.base.syncpts = syncpts;
 	vic->client.base.num_syncpts = 1;
 	vic->dev = dev;
 
 	INIT_LIST_HEAD(&vic->client.list);
+	vic->client.version = vic->config->version;
 	vic->client.ops = &vic_ops;
 
 	err = host1x_client_register(&vic->client.base);
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 6dc819b1ab64..518864cfbd16 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -62,7 +62,6 @@  struct host1x_client {
 	const struct host1x_client_ops *ops;
 
 	enum host1x_class class;
-	unsigned int version;
 
 	struct host1x_channel *channel;