On Wed, Jan 23, 2019 at 03:47:45PM +0300, Dmitry Osipenko wrote:
> 23.01.2019 12:39, Thierry Reding пишет:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
