| Message ID | 20180518123939.GA9594@ulmo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > >
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>
--- 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;