| Message ID | 1460974509-22616-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote: > With the new i915_gem_obj_pin_map() interface, it makes sense to keep > GuC objects (which are always pinned in memory and in the GGTT anyway) > mapped into kernel address space, rather than mapping and unmapping them > on each access. > > This preliminary patch sets up the pin-and-map for all GuC-specific > objects, and updates the various setup/shutdown functions to use these > long-term mappings rather than doing their own kmap_atomic() calls. > > v2: > Invent & use accessor function i915_object_mapped_address() rather > than accessing obj->mapping directly (Tvrtko Ursulin) > Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin) > Added big comment about struct i915_guc_client & usage of the GEM > object that it describes > > Cc: <tvrtko.ursulin@intel.com> > Signed-off-by: Alex Dai <yu.dai@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++ > drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++------------------- > drivers/gpu/drm/i915/intel_guc.h | 24 +++++++++++++++++++ > 3 files changed, 50 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 85102ad..439f149 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) > void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj); > > /** > + * i915_object_mapped_address - address at which a GEM object is mapped > + * @obj - the object (presumably already mapped into kernel address space) > + * > + * Returns the address at which an object has been mapped by a call to > + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned > + */ > +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj) > +{ > + return obj->pages_pin_count == 0 ? NULL : obj->mapping; > +} No. > @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) > if (i915_gem_obj_is_pinned(obj)) > i915_gem_object_ggtt_unpin(obj); > > + if (i915_object_mapped_address(obj)) > + i915_gem_object_unpin_map(obj); Eh? If you aren't tracking your pin, you can't just randomly free someone elses. > drm_gem_object_unreference(&obj->base); > } > > @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > goto err; > > client->client_obj = obj; > + client->client_base = i915_object_mapped_address(obj); > + WARN_ON(!client->client_base); > client->wq_offset = GUC_DB_SIZE; > client->wq_size = GUC_WQ_SIZE; > > @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc) > struct guc_policies *policies; > struct guc_mmio_reg_state *reg_state; > struct intel_engine_cs *engine; > - struct page *page; > u32 size; > > /* The ads obj includes the struct itself and buffers passed to GuC */ > @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc) > > guc->ads_obj = obj; > } > - > - page = i915_gem_object_get_page(obj, 0); > - ads = kmap(page); > + ads = i915_object_mapped_address(obj); You need to explicitly aknowlege that you are using the memory tracked by the object otherwise it *will* be removed by the shrinker. So what is wrong with using i915_gem_object_pin_map()? -Chris
On 18/04/16 11:25, Chris Wilson wrote: > On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote: >> With the new i915_gem_obj_pin_map() interface, it makes sense to keep >> GuC objects (which are always pinned in memory and in the GGTT anyway) >> mapped into kernel address space, rather than mapping and unmapping them >> on each access. >> >> This preliminary patch sets up the pin-and-map for all GuC-specific >> objects, and updates the various setup/shutdown functions to use these >> long-term mappings rather than doing their own kmap_atomic() calls. >> >> v2: >> Invent & use accessor function i915_object_mapped_address() rather >> than accessing obj->mapping directly (Tvrtko Ursulin) >> Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin) >> Added big comment about struct i915_guc_client & usage of the GEM >> object that it describes >> >> Cc: <tvrtko.ursulin@intel.com> >> Signed-off-by: Alex Dai <yu.dai@intel.com> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++ >> drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++------------------- >> drivers/gpu/drm/i915/intel_guc.h | 24 +++++++++++++++++++ >> 3 files changed, 50 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 85102ad..439f149 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) >> void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj); >> >> /** >> + * i915_object_mapped_address - address at which a GEM object is mapped >> + * @obj - the object (presumably already mapped into kernel address space) >> + * >> + * Returns the address at which an object has been mapped by a call to >> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned >> + */ >> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj) >> +{ >> + return obj->pages_pin_count == 0 ? NULL : obj->mapping; >> +} > > No. The kernel address at which an object is pinned is just as much a property of the object as its GGTT address, so should have a similar function for retrieving it. The pages_pin_count check is there because once that goes to zero, the object doesn't logically /have/ a memory address, even if 'mapping' is still set. Or was it just the name you objected to? >> @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) >> if (i915_gem_obj_is_pinned(obj)) >> i915_gem_object_ggtt_unpin(obj); >> >> + if (i915_object_mapped_address(obj)) >> + i915_gem_object_unpin_map(obj); > > Eh? If you aren't tracking your pin, you can't just randomly free > someone elses. The GuC code exclusively owns this object, so it can't be "someone else's". The pages_pin count here will always be 1 (at present). This was to allow for the possibility that we already chose to release the pin-and-map while leaving the object in the GGTT, in which case the pages_pin_count will be 0. (We don't do this, at present, but see below). >> drm_gem_object_unreference(&obj->base); >> } >> >> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, >> goto err; >> >> client->client_obj = obj; >> + client->client_base = i915_object_mapped_address(obj); >> + WARN_ON(!client->client_base); >> client->wq_offset = GUC_DB_SIZE; >> client->wq_size = GUC_WQ_SIZE; >> >> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc) >> struct guc_policies *policies; >> struct guc_mmio_reg_state *reg_state; >> struct intel_engine_cs *engine; >> - struct page *page; >> u32 size; >> >> /* The ads obj includes the struct itself and buffers passed to GuC */ >> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc) >> >> guc->ads_obj = obj; >> } >> - >> - page = i915_gem_object_get_page(obj, 0); >> - ads = kmap(page); >> + ads = i915_object_mapped_address(obj); > > You need to explicitly aknowlege that you are using the memory tracked > by the object otherwise it *will* be removed by the shrinker. > So what is wrong with using i915_gem_object_pin_map()? > -Chris The object is pinned-and-mapped just once, when it's created, and kept that way until it's destroyed. Calling pin-and-map here would give it a pin count of two, which would be redundant, and require us to unpin-and-map it again at the end of the function - which just seems a waste of time. I also considered dropping the original pin-and-map on the ADS object once this function has finished with it, but decided it wasn't worth the extra work. It has to stay in memory (and in the GGTT) anyway, so it would only save a few pages of vmap space. If we did that, the check on whether the object was still mapped (in gem_release_guc_obj() above) would absolutely be necessary. .Dave.
On Mon, Apr 18, 2016 at 12:28:43PM +0100, Dave Gordon wrote: > On 18/04/16 11:25, Chris Wilson wrote: > >On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote: > >>With the new i915_gem_obj_pin_map() interface, it makes sense to keep > >>GuC objects (which are always pinned in memory and in the GGTT anyway) > >>mapped into kernel address space, rather than mapping and unmapping them > >>on each access. > >> > >>This preliminary patch sets up the pin-and-map for all GuC-specific > >>objects, and updates the various setup/shutdown functions to use these > >>long-term mappings rather than doing their own kmap_atomic() calls. > >> > >>v2: > >> Invent & use accessor function i915_object_mapped_address() rather > >> than accessing obj->mapping directly (Tvrtko Ursulin) > >> Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin) > >> Added big comment about struct i915_guc_client & usage of the GEM > >> object that it describes > >> > >>Cc: <tvrtko.ursulin@intel.com> > >>Signed-off-by: Alex Dai <yu.dai@intel.com> > >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++ > >> drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++------------------- > >> drivers/gpu/drm/i915/intel_guc.h | 24 +++++++++++++++++++ > >> 3 files changed, 50 insertions(+), 24 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>index 85102ad..439f149 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.h > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>@@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) > >> void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj); > >> > >> /** > >>+ * i915_object_mapped_address - address at which a GEM object is mapped > >>+ * @obj - the object (presumably already mapped into kernel address space) > >>+ * > >>+ * Returns the address at which an object has been mapped by a call to > >>+ * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned > >>+ */ > >>+static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj) > >>+{ > >>+ return obj->pages_pin_count == 0 ? NULL : obj->mapping; > >>+} > > > >No. > > The kernel address at which an object is pinned is just as much a > property of the object as its GGTT address, so should have a similar > function for retrieving it. An object doesn't have a GGTT, a vma does. The object has vma. To use the vma, you must pin it. > The pages_pin_count check is there because once that goes to zero, > the object doesn't logically /have/ a memory address, even if > 'mapping' is still set. > > Or was it just the name you objected to? Name and function. obj->mapping is tied into the shinker which can and will run at any time, stealing all locks. Using obj->mapping which holding an actual page reference for yourself is broken. > > >>@@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) > >> if (i915_gem_obj_is_pinned(obj)) > >> i915_gem_object_ggtt_unpin(obj); > >> > >>+ if (i915_object_mapped_address(obj)) > >>+ i915_gem_object_unpin_map(obj); > > > >Eh? If you aren't tracking your pin, you can't just randomly free > >someone elses. > > The GuC code exclusively owns this object, so it can't be "someone > else's". The pages_pin count here will always be 1 (at present). > This was to allow for the possibility that we already chose to > release the pin-and-map while leaving the object in the GGTT, in > which case the pages_pin_count will be 0. (We don't do this, at > present, but see below). That is bad justification for adding unsafe code. > >> drm_gem_object_unreference(&obj->base); > >> } > >> > >>@@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > >> goto err; > >> > >> client->client_obj = obj; > >>+ client->client_base = i915_object_mapped_address(obj); > >>+ WARN_ON(!client->client_base); > >> client->wq_offset = GUC_DB_SIZE; > >> client->wq_size = GUC_WQ_SIZE; > >> > >>@@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc) > >> struct guc_policies *policies; > >> struct guc_mmio_reg_state *reg_state; > >> struct intel_engine_cs *engine; > >>- struct page *page; > >> u32 size; > >> > >> /* The ads obj includes the struct itself and buffers passed to GuC */ > >>@@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc) > >> > >> guc->ads_obj = obj; > >> } > >>- > >>- page = i915_gem_object_get_page(obj, 0); > >>- ads = kmap(page); > >>+ ads = i915_object_mapped_address(obj); > > > >You need to explicitly aknowlege that you are using the memory tracked > >by the object otherwise it *will* be removed by the shrinker. > >So what is wrong with using i915_gem_object_pin_map()? > >-Chris > > The object is pinned-and-mapped just once, when it's created, and > kept that way until it's destroyed. Calling pin-and-map here would > give it a pin count of two, which would be redundant, and require us > to unpin-and-map it again at the end of the function - which just > seems a waste of time. Considering the alternative is an unsound API. Just do it, or pass around the mapping in a local you have already pinned. -Chris
On 18/04/16 12:37, Chris Wilson wrote: > On Mon, Apr 18, 2016 at 12:28:43PM +0100, Dave Gordon wrote: >> On 18/04/16 11:25, Chris Wilson wrote: >>> On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote: >>>> With the new i915_gem_obj_pin_map() interface, it makes sense to keep >>>> GuC objects (which are always pinned in memory and in the GGTT anyway) >>>> mapped into kernel address space, rather than mapping and unmapping them >>>> on each access. >>>> >>>> This preliminary patch sets up the pin-and-map for all GuC-specific >>>> objects, and updates the various setup/shutdown functions to use these >>>> long-term mappings rather than doing their own kmap_atomic() calls. >>>> >>>> v2: >>>> Invent & use accessor function i915_object_mapped_address() rather >>>> than accessing obj->mapping directly (Tvrtko Ursulin) >>>> Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin) >>>> Added big comment about struct i915_guc_client & usage of the GEM >>>> object that it describes >>>> >>>> Cc: <tvrtko.ursulin@intel.com> >>>> Signed-off-by: Alex Dai <yu.dai@intel.com> >>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++ >>>> drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++------------------- >>>> drivers/gpu/drm/i915/intel_guc.h | 24 +++++++++++++++++++ >>>> 3 files changed, 50 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 85102ad..439f149 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) >>>> void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj); >>>> >>>> /** >>>> + * i915_object_mapped_address - address at which a GEM object is mapped >>>> + * @obj - the object (presumably already mapped into kernel address space) >>>> + * >>>> + * Returns the address at which an object has been mapped by a call to >>>> + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned >>>> + */ >>>> +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj) >>>> +{ >>>> + return obj->pages_pin_count == 0 ? NULL : obj->mapping; >>>> +} >>> >>> No. >> >> The kernel address at which an object is pinned is just as much a >> property of the object as its GGTT address, so should have a similar >> function for retrieving it. > > An object doesn't have a GGTT, a vma does. The object has vma. To use > the vma, you must pin it. Well you obviously have a very different usage model in mind. The GuC object lifecycle is: guc_obj::create() { allocate get-pages-into-RAM pin-pages-in-RAM pin-to-gtt kmap } ~guc_obj() { undo all the above } guc_obj::do_stuff() { addr = self.mapped_address() ... // use *addr } where all the code relies on the constructor having set up all mappings once and for all; whereas it looks like you're thinking: do_stuff(gem_obj) { addr = pin-and-map(gem_obj); ... // use *addr unpin-and-map(gem_obj) } where each user of a mapping doesn't know about any others that might exist, and therefore has to manage pincounting. This local-knowledge-only model might be more natural if the function were called something like i915_gem_object_mapping_create() and returned an blob representing a binding between a GEM object and an address space -- in other words, a VMA where the "address space" would be "kernel" rather than GGTT or PPGTT (and the unmap function would take the returned blob, not the GEM object, as the thing-to-be-unmade). Then we could have partial mappings, and mappings with different properties (e.g. caching) coexisting. But the current pin-and-map implements *the* mapping as a singleton property of a GEM object, so it looks far more natural to set it up once and then expect to read its unchanged value at any time thereafter, until the mapping is finally destroyed along with the object. >> The pages_pin_count check is there because once that goes to zero, >> the object doesn't logically /have/ a memory address, even if >> 'mapping' is still set. >> >> Or was it just the name you objected to? > > Name and function. obj->mapping is tied into the shinker which can and > will run at any time, stealing all locks. Using obj->mapping which > holding an actual page reference for yourself is broken. All GuC objects are permanently pinned in RAM and GGTT, and hence immune to the shrinker. If the shrinker does touch them, the GuC will break. >>>> @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) >>>> if (i915_gem_obj_is_pinned(obj)) >>>> i915_gem_object_ggtt_unpin(obj); >>>> >>>> + if (i915_object_mapped_address(obj)) >>>> + i915_gem_object_unpin_map(obj); >>> >>> Eh? If you aren't tracking your pin, you can't just randomly free >>> someone elses. >> >> The GuC code exclusively owns this object, so it can't be "someone >> else's". The pages_pin count here will always be 1 (at present). >> This was to allow for the possibility that we already chose to >> release the pin-and-map while leaving the object in the GGTT, in >> which case the pages_pin_count will be 0. (We don't do this, at >> present, but see below). > > That is bad justification for adding unsafe code. NOT unsafe, because the GuC code holds the only reference to these *private* objects. As I said, no code outside this file will ever access these objects. And the code above is the destructor, so it would be a really bad idea for anyone else to have a pin-and-mapping of the object at this point. >>>> drm_gem_object_unreference(&obj->base); >>>> } >>>> >>>> @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, >>>> goto err; >>>> >>>> client->client_obj = obj; >>>> + client->client_base = i915_object_mapped_address(obj); >>>> + WARN_ON(!client->client_base); >>>> client->wq_offset = GUC_DB_SIZE; >>>> client->wq_size = GUC_WQ_SIZE; >>>> >>>> @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc) >>>> struct guc_policies *policies; >>>> struct guc_mmio_reg_state *reg_state; >>>> struct intel_engine_cs *engine; >>>> - struct page *page; >>>> u32 size; >>>> >>>> /* The ads obj includes the struct itself and buffers passed to GuC */ >>>> @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc) >>>> >>>> guc->ads_obj = obj; >>>> } >>>> - >>>> - page = i915_gem_object_get_page(obj, 0); >>>> - ads = kmap(page); >>>> + ads = i915_object_mapped_address(obj); >>> >>> You need to explicitly aknowlege that you are using the memory tracked >>> by the object otherwise it *will* be removed by the shrinker. >>> So what is wrong with using i915_gem_object_pin_map()? >>> -Chris >> >> The object is pinned-and-mapped just once, when it's created, and >> kept that way until it's destroyed. Calling pin-and-map here would >> give it a pin count of two, which would be redundant, and require us >> to unpin-and-map it again at the end of the function - which just >> seems a waste of time. > > Considering the alternative is an unsound API. Just do it, or pass > around the mapping in a local you have already pinned. > -Chris No, I've decided just not to use the pin-and-map API at all. We really only need direct access to the shared doorbell/process descriptor page, so we can just kmap the single page instead. This is more economical in use of address space, and simpler to keep away from the shrinker. .Dave.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 85102ad..439f149 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj); /** + * i915_object_mapped_address - address at which a GEM object is mapped + * @obj - the object (presumably already mapped into kernel address space) + * + * Returns the address at which an object has been mapped by a call to + * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned + */ +static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj) +{ + return obj->pages_pin_count == 0 ? NULL : obj->mapping; +} + +/** * i915_gem_object_unpin_map - releases an earlier mapping * @obj - the object to unmap * diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index da86bdb..de96306 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc *guc, struct i915_guc_client *client) { struct guc_doorbell_info *doorbell; - void *base; - base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0)); - doorbell = base + client->doorbell_offset; + doorbell = client->client_base + client->doorbell_offset; - doorbell->db_status = 1; + doorbell->db_status = GUC_DOORBELL_ENABLED; doorbell->cookie = 0; - - kunmap_atomic(base); } static int guc_ring_doorbell(struct i915_guc_client *gc) @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct intel_guc *guc, { struct drm_i915_private *dev_priv = guc_to_i915(guc); struct guc_doorbell_info *doorbell; - void *base; i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id); int value; - base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0)); - doorbell = base + client->doorbell_offset; - - doorbell->db_status = 0; + doorbell = client->client_base + client->doorbell_offset; - kunmap_atomic(base); + doorbell->db_status = GUC_DOORBELL_DISABLED; I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID); @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc *guc, struct i915_guc_client *client) { struct guc_process_desc *desc; - void *base; - base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0)); - desc = base + client->proc_desc_offset; + desc = client->client_base + client->proc_desc_offset; memset(desc, 0, sizeof(*desc)); @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc *guc, desc->wq_size_bytes = client->wq_size; desc->wq_status = WQ_STATUS_ACTIVE; desc->priority = client->priority; - - kunmap_atomic(base); } /* @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client, * This is a wrapper to create a gem obj. In order to use it inside GuC, the * object needs to be pinned lifetime. Also we must pin it to gtt space other * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC. + * The object is also pinned & mapped into kernel address space. * * Return: A drm_i915_gem_object if successful, otherwise NULL. */ @@ -620,13 +609,14 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev, if (!obj) return NULL; - if (i915_gem_object_get_pages(obj)) { + if (IS_ERR(i915_gem_object_pin_map(obj))) { drm_gem_object_unreference(&obj->base); return NULL; } if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) { + i915_gem_object_unpin_map(obj); drm_gem_object_unreference(&obj->base); return NULL; } @@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) if (i915_gem_obj_is_pinned(obj)) i915_gem_object_ggtt_unpin(obj); + if (i915_object_mapped_address(obj)) + i915_gem_object_unpin_map(obj); + drm_gem_object_unreference(&obj->base); } @@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, goto err; client->client_obj = obj; + client->client_base = i915_object_mapped_address(obj); + WARN_ON(!client->client_base); client->wq_offset = GUC_DB_SIZE; client->wq_size = GUC_WQ_SIZE; @@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc) struct guc_policies *policies; struct guc_mmio_reg_state *reg_state; struct intel_engine_cs *engine; - struct page *page; u32 size; /* The ads obj includes the struct itself and buffers passed to GuC */ @@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc) guc->ads_obj = obj; } - - page = i915_gem_object_get_page(obj, 0); - ads = kmap(page); + ads = i915_object_mapped_address(obj); /* * The GuC requires a "Golden Context" when it reinitialises @@ -897,8 +889,6 @@ static void guc_create_ads(struct intel_guc *guc) ads->reg_state_buffer = ads->reg_state_addr + sizeof(struct guc_mmio_reg_state); - - kunmap(page); } /* diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 3bb85b1..f2c051e 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -29,8 +29,31 @@ struct drm_i915_gem_request; +/* + * This structure primarily describes the GEM object shared with the GuC. + * The GEM object is held for the entire lifetime of our interaction with + * the GuC, being allocated before the GuC is loaded with its firmware. + * Because there's no way to update the address used by the GuC after + * initialisation, the shared object must stay pinned into the GGTT as + * long as the GuC is in use. We also keep it mapped into kernel address + * space, as it includes shared data that must be updated on every request + * submission. + * + * The single GEM object described here is actually made up of several + * separate areas, as far as the GuC is concerned. The first page includes + * the "process decriptor" which holds sequence data for the doorbell, and + * one cacheline which actually *is* the doorbell; a write to this will + * "ring" the doorbell (i.e. send an interrupt to the GuC). The subsequent + * pages of the client object constitute the work queue (a circular array + * of work items), again described in the process descriptor. + * + * Finally, we also keep a few statistics here, including the number of + * submissions to each engine, and a record of the last submission failure + * (if any). + */ struct i915_guc_client { struct drm_i915_gem_object *client_obj; + void *client_base; /* Mapped address of above */ struct intel_context *owner; struct intel_guc *guc; uint32_t priority; @@ -52,6 +75,7 @@ struct i915_guc_client { uint32_t q_fail; uint32_t b_fail; int retcode; + int spare; /* pad to 32 DWords */ }; enum intel_guc_fw_status {
With the new i915_gem_obj_pin_map() interface, it makes sense to keep GuC objects (which are always pinned in memory and in the GGTT anyway) mapped into kernel address space, rather than mapping and unmapping them on each access. This preliminary patch sets up the pin-and-map for all GuC-specific objects, and updates the various setup/shutdown functions to use these long-term mappings rather than doing their own kmap_atomic() calls. v2: Invent & use accessor function i915_object_mapped_address() rather than accessing obj->mapping directly (Tvrtko Ursulin) Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin) Added big comment about struct i915_guc_client & usage of the GEM object that it describes Cc: <tvrtko.ursulin@intel.com> Signed-off-by: Alex Dai <yu.dai@intel.com> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++++ drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++------------------- drivers/gpu/drm/i915/intel_guc.h | 24 +++++++++++++++++++ 3 files changed, 50 insertions(+), 24 deletions(-)