| Message ID | 1460654342-6071-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/04/16 18:19, 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. > > 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_guc_submission.c | 37 +++++++++++------------------- > drivers/gpu/drm/i915/intel_guc.h | 1 + > 2 files changed, 14 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index da86bdb..f80f577 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; Not 100% sure of the object lifetimes in GuC, but would it be even simpler to store a pointer to struct struct guc_doorbell_info as guc->doorbell ? There aren't that many call sites true, but kind of looks logical at least from the outside. > > - 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; And the same maybe for this? > 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 (i915_gem_object_pin_map(obj) == NULL) { This should be IS_ERR check. > 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,8 @@ 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); > > + i915_gem_object_unpin_map(obj); > + > drm_gem_object_unreference(&obj->base); > } > > @@ -729,6 +721,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > goto err; > > client->client_obj = obj; > + client->client_base = obj->mapping; It think outside code should not access obj->mapping directly but use what i915_gem_object_pin_map has returned. > + WARN_ON(!client->client_base); And this has already been handled at the i915_gem_object_pin_map call site so I don't think it serves any purpose. > client->wq_offset = GUC_DB_SIZE; > client->wq_size = GUC_WQ_SIZE; > > @@ -841,7 +835,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 +850,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 = obj->mapping; Same as above. I suggest storing the base address in the guc client or somewhere appropriate. Or if objects have separate explicit lifetimes, even if they don't overlap, you could even nest i915_gem_object_pin_map and unpin. Depends what makes the code simpler. > > /* > * The GuC requires a "Golden Context" when it reinitialises > @@ -897,8 +888,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..9ab3564 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -31,6 +31,7 @@ struct drm_i915_gem_request; > > 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; > Regards, Tvrtko
On 15/04/2016 11:04, Tvrtko Ursulin wrote: > > On 14/04/16 18:19, 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. >> >> 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_guc_submission.c | 37 >> +++++++++++------------------- >> drivers/gpu/drm/i915/intel_guc.h | 1 + >> 2 files changed, 14 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index da86bdb..f80f577 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; > > Not 100% sure of the object lifetimes in GuC, but would it be even > simpler to store a pointer to struct struct guc_doorbell_info as > guc->doorbell ? There aren't that many call sites true, but kind of > looks logical at least from the outside. Well probably, but that would be a separate patch. This is just dealing with eliminating the repeated kmap/unmap calls. Also, maybe this helps remind people that these are actually parts of the same object. There's just one allocated, but it encompasses the process descriptor and the doorbell in the first page, and the workqueue in the second and third. >> - 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; > > And the same maybe for this? > >> 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 (i915_gem_object_pin_map(obj) == NULL) { > > This should be IS_ERR check. OK, will update. >> 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,8 @@ 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); >> >> + i915_gem_object_unpin_map(obj); >> + >> drm_gem_object_unreference(&obj->base); >> } >> >> @@ -729,6 +721,8 @@ static struct i915_guc_client >> *guc_client_alloc(struct drm_device *dev, >> goto err; >> >> client->client_obj = obj; >> + client->client_base = obj->mapping; > > It think outside code should not access obj->mapping directly but use > what i915_gem_object_pin_map has returned. No, that would be quite inconvenient. You shouldn't need to hold auxiliary information about an allocated object when you can get that information directly from the object itself. Also, the function that does the pin-and-map doesn't have access to the structure where the address is going to be cached, it just returns the allocated-pinned-and-mapped object. OTOH I have no objection to wrapping it an accessor function/macro. void *i914_gem_object_mapped_addr(object) ? returning NULL if object is not mapped? >> + WARN_ON(!client->client_base); > > And this has already been handled at the i915_gem_object_pin_map call > site so I don't think it serves any purpose. In case the obj->mapping *wasn't* the same value that was returned from pin-and-map and checked. >> client->wq_offset = GUC_DB_SIZE; >> client->wq_size = GUC_WQ_SIZE; >> >> @@ -841,7 +835,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 +850,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 = obj->mapping; > > Same as above. I suggest storing the base address in the guc client or > somewhere appropriate. > > Or if objects have separate explicit lifetimes, even if they don't > overlap, you could even nest i915_gem_object_pin_map and unpin. > Depends what makes the code simpler. All GuC objects have to stay memory-resident and pinned at permanent addresses in the GGTT. For some of them we might not need the kernel mapping all the time, but it's probably simpler to keep it. If we didn't, I would have to change the 'release' code to do the unmap iff it was still mapped at that point. .Dave. >> /* >> * The GuC requires a "Golden Context" when it reinitialises >> @@ -897,8 +888,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..9ab3564 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -31,6 +31,7 @@ struct drm_i915_gem_request; >> >> 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; >> > > Regards, > > Tvrtko
On 15/04/16 12:12, Dave Gordon wrote: > On 15/04/2016 11:04, Tvrtko Ursulin wrote: >> >> On 14/04/16 18:19, 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. >>> >>> 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_guc_submission.c | 37 >>> +++++++++++------------------- >>> drivers/gpu/drm/i915/intel_guc.h | 1 + >>> 2 files changed, 14 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index da86bdb..f80f577 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; >> >> Not 100% sure of the object lifetimes in GuC, but would it be even >> simpler to store a pointer to struct struct guc_doorbell_info as >> guc->doorbell ? There aren't that many call sites true, but kind of >> looks logical at least from the outside. > > Well probably, but that would be a separate patch. This is just dealing > with eliminating the repeated kmap/unmap calls. Okay, just thought it may be easier to do it at once since it looked really straightforward, like same amount of work and cleaner end result. > Also, maybe this helps remind people that these are actually parts of > the same object. There's just one allocated, but it encompasses the > process descriptor and the doorbell in the first page, and the workqueue > in the second and third. I don't think anyone from the outside would care since it is all internal guc code so it is free to define its rules with a nice comment in the relevant structure definition. >>> - 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; >> >> And the same maybe for this? >> >>> 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 (i915_gem_object_pin_map(obj) == NULL) { >> >> This should be IS_ERR check. > > OK, will update. > >>> 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,8 @@ 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); >>> >>> + i915_gem_object_unpin_map(obj); >>> + >>> drm_gem_object_unreference(&obj->base); >>> } >>> >>> @@ -729,6 +721,8 @@ static struct i915_guc_client >>> *guc_client_alloc(struct drm_device *dev, >>> goto err; >>> >>> client->client_obj = obj; >>> + client->client_base = obj->mapping; >> >> It think outside code should not access obj->mapping directly but use >> what i915_gem_object_pin_map has returned. > > No, that would be quite inconvenient. You shouldn't need to hold > auxiliary information about an allocated object when you can get that > information directly from the object itself. You can not unless you break the API layer. obj->mapping is IMHO private to GEM and GuC should not touch it. Even must not IMHO. > Also, the function that does the pin-and-map doesn't have access to the > structure where the address is going to be cached, it just returns the > allocated-pinned-and-mapped object. That sounds like a local issue which can be worked around by storing it in the appropriate GuC data structure, no? > OTOH I have no objection to wrapping it an accessor function/macro. > > void *i914_gem_object_mapped_addr(object) ? > > returning NULL if object is not mapped? I don't feel strongly either way. Question for Chris I suppose. In this particular case I would look to avoid the need for it by storing the address in my own data structure(s), if they have constructors and destructors which start and end with i915_gem_object_pin_map/unmap respectively. >>> + WARN_ON(!client->client_base); >> >> And this has already been handled at the i915_gem_object_pin_map call >> site so I don't think it serves any purpose. > > In case the obj->mapping *wasn't* the same value that was returned from > pin-and-map and checked. Ah guard against touching forbidden parts. :) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index da86bdb..f80f577 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 (i915_gem_object_pin_map(obj) == NULL) { 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,8 @@ 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); + i915_gem_object_unpin_map(obj); + drm_gem_object_unreference(&obj->base); } @@ -729,6 +721,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, goto err; client->client_obj = obj; + client->client_base = obj->mapping; + WARN_ON(!client->client_base); client->wq_offset = GUC_DB_SIZE; client->wq_size = GUC_WQ_SIZE; @@ -841,7 +835,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 +850,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 = obj->mapping; /* * The GuC requires a "Golden Context" when it reinitialises @@ -897,8 +888,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..9ab3564 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -31,6 +31,7 @@ struct drm_i915_gem_request; 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;