| Message ID | 20190123093951.24908-3-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tegra: Fix IOVA space on Tegra186 and later | expand |
23.01.2019 12:39, Thierry Reding пишет: > From: Thierry Reding <treding@nvidia.com> > > Loading the firmware requires an allocation of IOVA space to make sure > that the VIC's Falcon microcontroller can read the firmware if address > translation via the SMMU is enabled. > > However, the allocation currently happens at a time where the geometry > of an IOMMU domain may not have been initialized yet. This happens for > example on Tegra186 and later where an ARM SMMU is used. Domains which > are created by the ARM SMMU driver postpone the geometry setup until a > device is attached to the domain. This is because IOMMU domains aren't > attached to a specific IOMMU instance at allocation time and hence the > input address space, which defines the geometry, is not known yet. > > Work around this by postponing the firmware load until it is needed at > the time where a channel is opened to the VIC. At this time the shared > IOMMU domain's geometry has been properly initialized. > > As a byproduct this allows the Tegra DRM to be created in the absence > of VIC firmware, since the VIC initialization no longer fails if the > firmware can't be found. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/tegra/vic.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > index d47983deb1cf..afbdc33f49bc 100644 > --- a/drivers/gpu/drm/tegra/vic.c > +++ b/drivers/gpu/drm/tegra/vic.c > @@ -181,13 +181,6 @@ static int vic_init(struct host1x_client *client) > vic->domain = tegra->domain; > } > > - if (!vic->falcon.data) { > - vic->falcon.data = tegra; > - err = falcon_load_firmware(&vic->falcon); > - if (err < 0) > - goto detach; > - } > - > vic->channel = host1x_channel_request(client->dev); > if (!vic->channel) { > err = -ENOMEM; > @@ -256,6 +249,16 @@ static int vic_open_channel(struct tegra_drm_client *client, > if (err < 0) > return err; > > + if (!vic->falcon.data) { > + vic->falcon.data = client->drm; > + > + err = falcon_load_firmware(&vic->falcon); > + if (err < 0) { > + pm_runtime_put(vic->dev); > + return err; > + } > + } > + > err = vic_boot(vic); > if (err < 0) { > pm_runtime_put(vic->dev); > This only moves the firmware data-copying to a later stage and doesn't touch reading out of the firmware file, hence the claim about the "byproduct" is invalid. Please take a look at the patch I posted sometime ago [0] and feel free to use it as a reference. [0] https://patchwork.ozlabs.org/patch/1010389/
On Wed, Jan 23, 2019 at 03:47:45PM +0300, Dmitry Osipenko wrote: > 23.01.2019 12:39, Thierry Reding пишет: > > From: Thierry Reding <treding@nvidia.com> > > > > Loading the firmware requires an allocation of IOVA space to make sure > > that the VIC's Falcon microcontroller can read the firmware if address > > translation via the SMMU is enabled. > > > > However, the allocation currently happens at a time where the geometry > > of an IOMMU domain may not have been initialized yet. This happens for > > example on Tegra186 and later where an ARM SMMU is used. Domains which > > are created by the ARM SMMU driver postpone the geometry setup until a > > device is attached to the domain. This is because IOMMU domains aren't > > attached to a specific IOMMU instance at allocation time and hence the > > input address space, which defines the geometry, is not known yet. > > > > Work around this by postponing the firmware load until it is needed at > > the time where a channel is opened to the VIC. At this time the shared > > IOMMU domain's geometry has been properly initialized. > > > > As a byproduct this allows the Tegra DRM to be created in the absence > > of VIC firmware, since the VIC initialization no longer fails if the > > firmware can't be found. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/gpu/drm/tegra/vic.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > > index d47983deb1cf..afbdc33f49bc 100644 > > --- a/drivers/gpu/drm/tegra/vic.c > > +++ b/drivers/gpu/drm/tegra/vic.c > > @@ -181,13 +181,6 @@ static int vic_init(struct host1x_client *client) > > vic->domain = tegra->domain; > > } > > > > - if (!vic->falcon.data) { > > - vic->falcon.data = tegra; > > - err = falcon_load_firmware(&vic->falcon); > > - if (err < 0) > > - goto detach; > > - } > > - > > vic->channel = host1x_channel_request(client->dev); > > if (!vic->channel) { > > err = -ENOMEM; > > @@ -256,6 +249,16 @@ static int vic_open_channel(struct tegra_drm_client *client, > > if (err < 0) > > return err; > > > > + if (!vic->falcon.data) { > > + vic->falcon.data = client->drm; > > + > > + err = falcon_load_firmware(&vic->falcon); > > + if (err < 0) { > > + pm_runtime_put(vic->dev); > > + return err; > > + } > > + } > > + > > err = vic_boot(vic); > > if (err < 0) { > > pm_runtime_put(vic->dev); > > > > This only moves the firmware data-copying to a later stage and doesn't > touch reading out of the firmware file, hence the claim about the > "byproduct" is invalid. Please take a look at the patch I posted > sometime ago [0] and feel free to use it as a reference. You're right, that hunk ended up in some other patch. And indeed this patch looks pretty much like yours, so I've merged both together (mine hadn't moved things out to a separate function, so I did that now, and mine still reuses the client->drm pointer introduced in an earlier patch to make it easier to pass that around). Will send out v2 of this patch. Thierry
23.01.2019 12:39, Thierry Reding пишет: > From: Thierry Reding <treding@nvidia.com> > > Loading the firmware requires an allocation of IOVA space to make sure > that the VIC's Falcon microcontroller can read the firmware if address > translation via the SMMU is enabled. > > However, the allocation currently happens at a time where the geometry > of an IOMMU domain may not have been initialized yet. This happens for > example on Tegra186 and later where an ARM SMMU is used. Domains which > are created by the ARM SMMU driver postpone the geometry setup until a > device is attached to the domain. This is because IOMMU domains aren't > attached to a specific IOMMU instance at allocation time and hence the > input address space, which defines the geometry, is not known yet. > > Work around this by postponing the firmware load until it is needed at > the time where a channel is opened to the VIC. At this time the shared > IOMMU domain's geometry has been properly initialized. > > As a byproduct this allows the Tegra DRM to be created in the absence > of VIC firmware, since the VIC initialization no longer fails if the > firmware can't be found. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/tegra/vic.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > index d47983deb1cf..afbdc33f49bc 100644 > --- a/drivers/gpu/drm/tegra/vic.c > +++ b/drivers/gpu/drm/tegra/vic.c > @@ -181,13 +181,6 @@ static int vic_init(struct host1x_client *client) > vic->domain = tegra->domain; > } > > - if (!vic->falcon.data) { > - vic->falcon.data = tegra; > - err = falcon_load_firmware(&vic->falcon); > - if (err < 0) > - goto detach; > - } > - > vic->channel = host1x_channel_request(client->dev); > if (!vic->channel) { > err = -ENOMEM; > @@ -256,6 +249,16 @@ static int vic_open_channel(struct tegra_drm_client *client, > if (err < 0) > return err; > > + if (!vic->falcon.data) { > + vic->falcon.data = client->drm; > + > + err = falcon_load_firmware(&vic->falcon); > + if (err < 0) { Also, notice that this hunk misses the "vic->falcon.data = NULL", hence the second channel opening will likely to crash kernel. > + pm_runtime_put(vic->dev); > + return err; > + } > + } > + > err = vic_boot(vic); > if (err < 0) { > pm_runtime_put(vic->dev); >
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index d47983deb1cf..afbdc33f49bc 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -181,13 +181,6 @@ static int vic_init(struct host1x_client *client) vic->domain = tegra->domain; } - if (!vic->falcon.data) { - vic->falcon.data = tegra; - err = falcon_load_firmware(&vic->falcon); - if (err < 0) - goto detach; - } - vic->channel = host1x_channel_request(client->dev); if (!vic->channel) { err = -ENOMEM; @@ -256,6 +249,16 @@ static int vic_open_channel(struct tegra_drm_client *client, if (err < 0) return err; + if (!vic->falcon.data) { + vic->falcon.data = client->drm; + + err = falcon_load_firmware(&vic->falcon); + if (err < 0) { + pm_runtime_put(vic->dev); + return err; + } + } + err = vic_boot(vic); if (err < 0) { pm_runtime_put(vic->dev);