Re: [PATCH 2/5] drm/tegra: vic: Load firmware on demand

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

[-- Attachment #1.1: Type: text/plain, Size: 3211 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel