Re: [PATCH 3/4] drm/i915/uc: Place uC firmware in upper range of GGTT

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Fernando Pacheco (2019-04-09 23:53:08)
> 
> On 4/9/19 2:53 PM, Chris Wilson wrote:
> > Quoting Fernando Pacheco (2019-04-09 22:31:01)
> >> +int intel_uc_fw_ggtt_pin(struct intel_uc_fw *uc_fw,
> >> +                        struct i915_ggtt *ggtt, u64 start)
> >> +{
> >> +       struct drm_i915_gem_object *obj = uc_fw->obj;
> >> +       int err;
> >> +
> >> +       err = i915_gem_object_set_to_gtt_domain(obj, false);
> > Currently requires struct_mutex, and is not required as we can ensure
> > the pages are flushed on creation.
> 
> My intent was to maintain what was being done before
> but just doing it earlier.
> 
> But if it's not required..

More so that it's illegal in the global reset context :)

There are patches on the list that remove the struct_mutex for this
operation (eek, no, ignore that you aren't allowed to take that lock
inside reset either!), but with a bit of care we shouldn't need the
set-to-gtt-domain at all as we can do the flush trivially (if it's even
required).

> >> +       if (err) {
> >> +               DRM_DEBUG_DRIVER("%s fw set-domain err=%d\n",
> >> +                                intel_uc_fw_type_repr(uc_fw->type), err);
> >> +               return err;
> >> +       }
> >> +
> >> +       err = i915_gem_object_pin_pages(obj);
> > I'm pretty sure we don't need to pin the pages here, as the caller
> > should be holding the pages already for the duration of the bind.
> >
> > So I think this should just reduce to the ggtt bind.
> 
> I might be misunderstanding, so could you please clarify
> what you mean by "should be holding"? Are you stating
> that the caller already holds the pages?

To copy the firmware image into the pages, those pages must exist. To
prevent those pages disappearing, we must have kept them around (i.e
pinned). So we know by construction of the uc_fw object we always have
the pages, and could skip bumping the pin-count around the xfer.
Therefore we only need to bind the existing firmware pages into the GGTT
to perform the dma xfer (after satisfying ourselves that the pages are
indeed flushed).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx