* [PATCH 0/4] drm/i915: updates to GuC doorbell handling
@ 2016-06-08 10:55 Dave Gordon
2016-06-08 10:55 ` [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info Dave Gordon
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Dave Gordon @ 2016-06-08 10:55 UTC (permalink / raw)
To: intel-gfx
The Linux hibernate/resume sequence involves booting one kernel, and
then replacing(!) its in-memory image with that of the previously
hibernated system. This can lead to inconsistencies in the state of
the hardware, in particular where a driver does not or cannot reset
it to a well-defined initial state during resume.
For i915, the issue is that the doorbell hardware is not reset when
the GuC is reset; also, the driver *cannot* directly reprogram it:
only the GuC can do that. So this set of patches first reorganises
the doorbell handling, and then (in the last patch of the set)
ensures that the doorbell hardware is fully (re-)initialised when
the GuC is (re-)loaded.
Dave Gordon (4):
drm/i915/guc: add doorbell map to debugfs/i915_guc_info
drm/i915/guc: move guc_ring_doorbell() nearer to callsite
drm/i915/guc: refactor doorbell management code
drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission
drivers/gpu/drm/i915/i915_debugfs.c | 9 ++
drivers/gpu/drm/i915/i915_guc_submission.c | 232 ++++++++++++++++++-----------
2 files changed, 154 insertions(+), 87 deletions(-)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon @ 2016-06-08 10:55 ` Dave Gordon 2016-06-08 12:32 ` Tvrtko Ursulin 2016-06-08 10:55 ` [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite Dave Gordon ` (3 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Dave Gordon @ 2016-06-08 10:55 UTC (permalink / raw) To: intel-gfx To properly verify the driver->doorbell->GuC functionality, validation needs to know how the driver has assigned the doorbell cache lines and registers, so make them visible through debugfs. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index e4f2c55..fbb4b16 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2560,6 +2560,7 @@ static int i915_guc_info(struct seq_file *m, void *data) struct i915_guc_client client = {}; struct intel_engine_cs *engine; u64 total = 0; + int i; if (!HAS_GUC_SCHED(dev_priv)) return 0; @@ -2574,6 +2575,14 @@ static int i915_guc_info(struct seq_file *m, void *data) mutex_unlock(&dev->struct_mutex); + seq_printf(m, "Doorbell map:\n"); + BUILD_BUG_ON(ARRAY_SIZE(guc.doorbell_bitmap) % 4); + for (i = 0; i < ARRAY_SIZE(guc.doorbell_bitmap) - 3; i += 4) + seq_printf(m, "\t%016lx %016lx %016lx %016lx\n", + guc.doorbell_bitmap[i], guc.doorbell_bitmap[i+1], + guc.doorbell_bitmap[i+2], guc.doorbell_bitmap[i+3]); + seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc.db_cacheline); + seq_printf(m, "GuC total action count: %llu\n", guc.action_count); seq_printf(m, "GuC action failure count: %u\n", guc.action_fail); seq_printf(m, "GuC last action command: 0x%x\n", guc.action_cmd); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info 2016-06-08 10:55 ` [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info Dave Gordon @ 2016-06-08 12:32 ` Tvrtko Ursulin 0 siblings, 0 replies; 10+ messages in thread From: Tvrtko Ursulin @ 2016-06-08 12:32 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 08/06/16 11:55, Dave Gordon wrote: > To properly verify the driver->doorbell->GuC functionality, validation > needs to know how the driver has assigned the doorbell cache lines and > registers, so make them visible through debugfs. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index e4f2c55..fbb4b16 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2560,6 +2560,7 @@ static int i915_guc_info(struct seq_file *m, void *data) > struct i915_guc_client client = {}; > struct intel_engine_cs *engine; > u64 total = 0; > + int i; > > if (!HAS_GUC_SCHED(dev_priv)) > return 0; > @@ -2574,6 +2575,14 @@ static int i915_guc_info(struct seq_file *m, void *data) > > mutex_unlock(&dev->struct_mutex); > > + seq_printf(m, "Doorbell map:\n"); > + BUILD_BUG_ON(ARRAY_SIZE(guc.doorbell_bitmap) % 4); > + for (i = 0; i < ARRAY_SIZE(guc.doorbell_bitmap) - 3; i += 4) > + seq_printf(m, "\t%016lx %016lx %016lx %016lx\n", Bitmap is unsigned long so 32-bit or 64-bit depending on the kernel build. Which makes the format wrong for 32-bit. And the ARRAY_SIZE will be different as well. So I think output should be made consistent between the two. Probably either pick u32 or u64 and dump it like that. #define DECLARE_BITMAP(name,bits) \ unsigned long name[BITS_TO_LONGS(bits)] DECLARE_BITMAP(doorbell_bitmap, GUC_MAX_DOORBELLS); > + guc.doorbell_bitmap[i], guc.doorbell_bitmap[i+1], > + guc.doorbell_bitmap[i+2], guc.doorbell_bitmap[i+3]); > + seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc.db_cacheline); > + > seq_printf(m, "GuC total action count: %llu\n", guc.action_count); > seq_printf(m, "GuC action failure count: %u\n", guc.action_fail); > seq_printf(m, "GuC last action command: 0x%x\n", guc.action_cmd); > Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon 2016-06-08 10:55 ` [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info Dave Gordon @ 2016-06-08 10:55 ` Dave Gordon 2016-06-08 12:47 ` Tvrtko Ursulin 2016-06-08 10:55 ` [PATCH 3/4] drm/i915/guc: refactor doorbell management code Dave Gordon ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Dave Gordon @ 2016-06-08 10:55 UTC (permalink / raw) To: intel-gfx Just code movement, no actual change to the function. This is in preparation for the next patch, which will reorganise all the other doorbell code, but doesn't change this function. So let's shuffle it down near its caller rather than leaving it mixed in with the setup code. Unlike the doorbell management code, this function is somewhat time-critical, so putting it near its caller may even yield a tiny performance improvement. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 110 ++++++++++++++--------------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 2db1182..7510841 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -185,61 +185,6 @@ static void guc_init_doorbell(struct intel_guc *guc, doorbell->cookie = 0; } -static int guc_ring_doorbell(struct i915_guc_client *gc) -{ - struct guc_process_desc *desc; - union guc_doorbell_qw db_cmp, db_exc, db_ret; - union guc_doorbell_qw *db; - int attempt = 2, ret = -EAGAIN; - - desc = gc->client_base + gc->proc_desc_offset; - - /* Update the tail so it is visible to GuC */ - desc->tail = gc->wq_tail; - - /* current cookie */ - db_cmp.db_status = GUC_DOORBELL_ENABLED; - db_cmp.cookie = gc->cookie; - - /* cookie to be updated */ - db_exc.db_status = GUC_DOORBELL_ENABLED; - db_exc.cookie = gc->cookie + 1; - if (db_exc.cookie == 0) - db_exc.cookie = 1; - - /* pointer of current doorbell cacheline */ - db = gc->client_base + gc->doorbell_offset; - - while (attempt--) { - /* lets ring the doorbell */ - db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db, - db_cmp.value_qw, db_exc.value_qw); - - /* if the exchange was successfully executed */ - if (db_ret.value_qw == db_cmp.value_qw) { - /* db was successfully rung */ - gc->cookie = db_exc.cookie; - ret = 0; - break; - } - - /* XXX: doorbell was lost and need to acquire it again */ - if (db_ret.db_status == GUC_DOORBELL_DISABLED) - break; - - DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n", - db_cmp.cookie, db_ret.cookie); - - /* update the cookie to newly read cookie from GuC */ - db_cmp.cookie = db_ret.cookie; - db_exc.cookie = db_ret.cookie + 1; - if (db_exc.cookie == 0) - db_exc.cookie = 1; - } - - return ret; -} - static void guc_disable_doorbell(struct intel_guc *guc, struct i915_guc_client *client) { @@ -543,6 +488,61 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc, kunmap_atomic(base); } +static int guc_ring_doorbell(struct i915_guc_client *gc) +{ + struct guc_process_desc *desc; + union guc_doorbell_qw db_cmp, db_exc, db_ret; + union guc_doorbell_qw *db; + int attempt = 2, ret = -EAGAIN; + + desc = gc->client_base + gc->proc_desc_offset; + + /* Update the tail so it is visible to GuC */ + desc->tail = gc->wq_tail; + + /* current cookie */ + db_cmp.db_status = GUC_DOORBELL_ENABLED; + db_cmp.cookie = gc->cookie; + + /* cookie to be updated */ + db_exc.db_status = GUC_DOORBELL_ENABLED; + db_exc.cookie = gc->cookie + 1; + if (db_exc.cookie == 0) + db_exc.cookie = 1; + + /* pointer of current doorbell cacheline */ + db = gc->client_base + gc->doorbell_offset; + + while (attempt--) { + /* lets ring the doorbell */ + db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db, + db_cmp.value_qw, db_exc.value_qw); + + /* if the exchange was successfully executed */ + if (db_ret.value_qw == db_cmp.value_qw) { + /* db was successfully rung */ + gc->cookie = db_exc.cookie; + ret = 0; + break; + } + + /* XXX: doorbell was lost and need to acquire it again */ + if (db_ret.db_status == GUC_DOORBELL_DISABLED) + break; + + DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n", + db_cmp.cookie, db_ret.cookie); + + /* update the cookie to newly read cookie from GuC */ + db_cmp.cookie = db_ret.cookie; + db_exc.cookie = db_ret.cookie + 1; + if (db_exc.cookie == 0) + db_exc.cookie = 1; + } + + return ret; +} + /** * i915_guc_submit() - Submit commands through GuC * @rq: request associated with the commands -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite 2016-06-08 10:55 ` [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite Dave Gordon @ 2016-06-08 12:47 ` Tvrtko Ursulin 0 siblings, 0 replies; 10+ messages in thread From: Tvrtko Ursulin @ 2016-06-08 12:47 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 08/06/16 11:55, Dave Gordon wrote: > Just code movement, no actual change to the function. This is in > preparation for the next patch, which will reorganise all the other > doorbell code, but doesn't change this function. So let's shuffle it > down near its caller rather than leaving it mixed in with the setup > code. Unlike the doorbell management code, this function is somewhat > time-critical, so putting it near its caller may even yield a tiny > performance improvement. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 110 ++++++++++++++--------------- > 1 file changed, 55 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 2db1182..7510841 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -185,61 +185,6 @@ static void guc_init_doorbell(struct intel_guc *guc, > doorbell->cookie = 0; > } > > -static int guc_ring_doorbell(struct i915_guc_client *gc) > -{ > - struct guc_process_desc *desc; > - union guc_doorbell_qw db_cmp, db_exc, db_ret; > - union guc_doorbell_qw *db; > - int attempt = 2, ret = -EAGAIN; > - > - desc = gc->client_base + gc->proc_desc_offset; > - > - /* Update the tail so it is visible to GuC */ > - desc->tail = gc->wq_tail; > - > - /* current cookie */ > - db_cmp.db_status = GUC_DOORBELL_ENABLED; > - db_cmp.cookie = gc->cookie; > - > - /* cookie to be updated */ > - db_exc.db_status = GUC_DOORBELL_ENABLED; > - db_exc.cookie = gc->cookie + 1; > - if (db_exc.cookie == 0) > - db_exc.cookie = 1; > - > - /* pointer of current doorbell cacheline */ > - db = gc->client_base + gc->doorbell_offset; > - > - while (attempt--) { > - /* lets ring the doorbell */ > - db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db, > - db_cmp.value_qw, db_exc.value_qw); > - > - /* if the exchange was successfully executed */ > - if (db_ret.value_qw == db_cmp.value_qw) { > - /* db was successfully rung */ > - gc->cookie = db_exc.cookie; > - ret = 0; > - break; > - } > - > - /* XXX: doorbell was lost and need to acquire it again */ > - if (db_ret.db_status == GUC_DOORBELL_DISABLED) > - break; > - > - DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n", > - db_cmp.cookie, db_ret.cookie); > - > - /* update the cookie to newly read cookie from GuC */ > - db_cmp.cookie = db_ret.cookie; > - db_exc.cookie = db_ret.cookie + 1; > - if (db_exc.cookie == 0) > - db_exc.cookie = 1; > - } > - > - return ret; > -} > - > static void guc_disable_doorbell(struct intel_guc *guc, > struct i915_guc_client *client) > { > @@ -543,6 +488,61 @@ static void guc_add_workqueue_item(struct i915_guc_client *gc, > kunmap_atomic(base); > } > > +static int guc_ring_doorbell(struct i915_guc_client *gc) > +{ > + struct guc_process_desc *desc; > + union guc_doorbell_qw db_cmp, db_exc, db_ret; > + union guc_doorbell_qw *db; > + int attempt = 2, ret = -EAGAIN; > + > + desc = gc->client_base + gc->proc_desc_offset; > + > + /* Update the tail so it is visible to GuC */ > + desc->tail = gc->wq_tail; > + > + /* current cookie */ > + db_cmp.db_status = GUC_DOORBELL_ENABLED; > + db_cmp.cookie = gc->cookie; > + > + /* cookie to be updated */ > + db_exc.db_status = GUC_DOORBELL_ENABLED; > + db_exc.cookie = gc->cookie + 1; > + if (db_exc.cookie == 0) > + db_exc.cookie = 1; > + > + /* pointer of current doorbell cacheline */ > + db = gc->client_base + gc->doorbell_offset; > + > + while (attempt--) { > + /* lets ring the doorbell */ > + db_ret.value_qw = atomic64_cmpxchg((atomic64_t *)db, > + db_cmp.value_qw, db_exc.value_qw); > + > + /* if the exchange was successfully executed */ > + if (db_ret.value_qw == db_cmp.value_qw) { > + /* db was successfully rung */ > + gc->cookie = db_exc.cookie; > + ret = 0; > + break; > + } > + > + /* XXX: doorbell was lost and need to acquire it again */ > + if (db_ret.db_status == GUC_DOORBELL_DISABLED) > + break; > + > + DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n", > + db_cmp.cookie, db_ret.cookie); > + > + /* update the cookie to newly read cookie from GuC */ > + db_cmp.cookie = db_ret.cookie; > + db_exc.cookie = db_ret.cookie + 1; > + if (db_exc.cookie == 0) > + db_exc.cookie = 1; > + } > + > + return ret; > +} > + > /** > * i915_guc_submit() - Submit commands through GuC > * @rq: request associated with the commands > Diffstat is correct, did not read the individual lines. :) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] drm/i915/guc: refactor doorbell management code 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon 2016-06-08 10:55 ` [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info Dave Gordon 2016-06-08 10:55 ` [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite Dave Gordon @ 2016-06-08 10:55 ` Dave Gordon 2016-06-08 13:11 ` Tvrtko Ursulin 2016-06-08 10:55 ` [PATCH 4/4] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission Dave Gordon 2016-06-08 11:41 ` ✓ Ro.CI.BAT: success for drm/i915: updates to GuC doorbell handling Patchwork 4 siblings, 1 reply; 10+ messages in thread From: Dave Gordon @ 2016-06-08 10:55 UTC (permalink / raw) To: intel-gfx During a hibernate/resume cycle, the driver, the GuC, and the doorbell hardware can end up in inconsistent states. This patch refactors the driver's handling and tracking of doorbells, in preparation for a later one which will resolve the issue. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 87 ++++++++++++++++++------------ 1 file changed, 53 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 7510841..eef67c8 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, * client object which contains the page being used for the doorbell */ -static void guc_init_doorbell(struct intel_guc *guc, - struct i915_guc_client *client) +static int guc_update_doorbell_id(struct i915_guc_client *client, + struct guc_doorbell_info *doorbell, + u16 new_id) { - struct guc_doorbell_info *doorbell; + struct sg_table *sg = client->guc->ctx_pool_obj->pages; + void *doorbell_bitmap = client->guc->doorbell_bitmap; + struct guc_context_desc desc; + size_t len; + + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID && + test_bit(client->doorbell_id, doorbell_bitmap)) { + /* Deactivate the old doorbell */ + doorbell->db_status = GUC_DOORBELL_DISABLED; + (void)host2guc_release_doorbell(client->guc, client); + clear_bit(client->doorbell_id, doorbell_bitmap); + } - doorbell = client->client_base + client->doorbell_offset; + /* Update the GuC's idea of the doorbell ID */ + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), + sizeof(desc) * client->ctx_index); + if (len != sizeof(desc)) + return -EFAULT; + desc.db_id = new_id; + len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), + sizeof(desc) * client->ctx_index); + if (len != sizeof(desc)) + return -EFAULT; + + client->doorbell_id = new_id; + if (new_id == GUC_INVALID_DOORBELL_ID) + return 0; + /* Activate the new doorbell */ + set_bit(client->doorbell_id, doorbell_bitmap); doorbell->db_status = GUC_DOORBELL_ENABLED; doorbell->cookie = 0; + return host2guc_allocate_doorbell(client->guc, client); } -static void guc_disable_doorbell(struct intel_guc *guc, - struct i915_guc_client *client) +static void guc_init_doorbell(struct intel_guc *guc, + struct i915_guc_client *client, + uint16_t db_id) { - struct drm_i915_private *dev_priv = guc_to_i915(guc); struct guc_doorbell_info *doorbell; - i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id); - int value; doorbell = client->client_base + client->doorbell_offset; - doorbell->db_status = GUC_DOORBELL_DISABLED; - - I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID); + guc_update_doorbell_id(client, doorbell, db_id); +} - value = I915_READ(drbreg); - WARN_ON((value & GEN8_DRB_VALID) != 0); +static void guc_disable_doorbell(struct intel_guc *guc, + struct i915_guc_client *client) +{ + struct guc_doorbell_info *doorbell; - I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0); - I915_WRITE(drbreg, 0); + doorbell = client->client_base + client->doorbell_offset; + guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID); /* XXX: wait for any interrupts */ /* XXX: wait for workqueue to drain */ @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority) if (id == end) id = GUC_INVALID_DOORBELL_ID; else - bitmap_set(guc->doorbell_bitmap, id, 1); + set_bit(id, guc->doorbell_bitmap); DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n", hi_pri ? "high" : "normal", id); @@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority) return id; } -static void release_doorbell(struct intel_guc *guc, uint16_t id) -{ - bitmap_clear(guc->doorbell_bitmap, id, 1); -} - /* * Initialise the process descriptor shared with the GuC firmware. */ @@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev, */ if (client->client_base) { - uint16_t db_id = client->doorbell_id; - /* * If we got as far as setting up a doorbell, make sure * we shut it down before unmapping & deallocating the @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev, * GuC that we've finished with it, finally deallocate * it in our bitmap */ - if (db_id != GUC_INVALID_DOORBELL_ID) { + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) guc_disable_doorbell(guc, client); - if (test_bit(db_id, guc->doorbell_bitmap)) - host2guc_release_doorbell(guc, client); - release_doorbell(guc, db_id); - } kunmap(kmap_to_page(client->client_base)); } @@ -706,6 +722,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc *guc = &dev_priv->guc; struct drm_i915_gem_object *obj; + uint16_t db_id; client = kzalloc(sizeof(*client), GFP_KERNEL); if (!client) @@ -746,22 +763,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, else client->proc_desc_offset = (GUC_DB_SIZE / 2); - client->doorbell_id = assign_doorbell(guc, client->priority); - if (client->doorbell_id == GUC_INVALID_DOORBELL_ID) + db_id = assign_doorbell(guc, client->priority); + if (db_id == GUC_INVALID_DOORBELL_ID) /* XXX: evict a doorbell instead */ goto err; guc_init_proc_desc(guc, client); guc_init_ctx_desc(guc, client); - guc_init_doorbell(guc, client); + guc_init_doorbell(guc, client, db_id); /* XXX: Any cache flushes needed? General domain mgmt calls? */ if (host2guc_allocate_doorbell(guc, client)) goto err; - DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n", - priority, client, client->ctx_index, client->doorbell_id); + DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n", + priority, client, client->ctx_index); + DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n", + client->doorbell_id, client->doorbell_offset); return client; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] drm/i915/guc: refactor doorbell management code 2016-06-08 10:55 ` [PATCH 3/4] drm/i915/guc: refactor doorbell management code Dave Gordon @ 2016-06-08 13:11 ` Tvrtko Ursulin 2016-06-10 11:37 ` Dave Gordon 0 siblings, 1 reply; 10+ messages in thread From: Tvrtko Ursulin @ 2016-06-08 13:11 UTC (permalink / raw) To: Dave Gordon, intel-gfx On 08/06/16 11:55, Dave Gordon wrote: > During a hibernate/resume cycle, the driver, the GuC, and the doorbell > hardware can end up in inconsistent states. This patch refactors the > driver's handling and tracking of doorbells, in preparation for a later > one which will resolve the issue. Perhaps a few word on the goal, method and result of refactoring. Would be good for documentation and easier review. > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 87 ++++++++++++++++++------------ > 1 file changed, 53 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 7510841..eef67c8 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct intel_guc *guc, > * client object which contains the page being used for the doorbell > */ > > -static void guc_init_doorbell(struct intel_guc *guc, > - struct i915_guc_client *client) > +static int guc_update_doorbell_id(struct i915_guc_client *client, > + struct guc_doorbell_info *doorbell, > + u16 new_id) > { > - struct guc_doorbell_info *doorbell; > + struct sg_table *sg = client->guc->ctx_pool_obj->pages; > + void *doorbell_bitmap = client->guc->doorbell_bitmap; > + struct guc_context_desc desc; > + size_t len; > + > + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID && > + test_bit(client->doorbell_id, doorbell_bitmap)) { > + /* Deactivate the old doorbell */ > + doorbell->db_status = GUC_DOORBELL_DISABLED; > + (void)host2guc_release_doorbell(client->guc, client); > + clear_bit(client->doorbell_id, doorbell_bitmap); > + } > > - doorbell = client->client_base + client->doorbell_offset; > + /* Update the GuC's idea of the doorbell ID */ > + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), > + sizeof(desc) * client->ctx_index); > + if (len != sizeof(desc)) > + return -EFAULT; > + desc.db_id = new_id; > + len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), > + sizeof(desc) * client->ctx_index); > + if (len != sizeof(desc)) > + return -EFAULT; > + > + client->doorbell_id = new_id; > + if (new_id == GUC_INVALID_DOORBELL_ID) > + return 0; > > + /* Activate the new doorbell */ > + set_bit(client->doorbell_id, doorbell_bitmap); > doorbell->db_status = GUC_DOORBELL_ENABLED; > doorbell->cookie = 0; > + return host2guc_allocate_doorbell(client->guc, client); A bunch of new code here which wasn't done on doorbell init before. Populating the ctx_pool_obj and hust2guc call. I think the commit needs to explain what is happening here. Maybe it is mostly a matter of copying over some text from the cover letter, but ideally it should explain what it is doing more precisely. > } > > -static void guc_disable_doorbell(struct intel_guc *guc, > - struct i915_guc_client *client) > +static void guc_init_doorbell(struct intel_guc *guc, > + struct i915_guc_client *client, > + uint16_t db_id) > { > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct guc_doorbell_info *doorbell; > - i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id); > - int value; > > doorbell = client->client_base + client->doorbell_offset; > > - doorbell->db_status = GUC_DOORBELL_DISABLED; > - > - I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID); > + guc_update_doorbell_id(client, doorbell, db_id); > +} This function does not end up doing anything now, just a pass-through to guc_update_doorbell_id. What happens to the write to drbreg ? It is completely gone and also from the init doorbell path together with the write to another register. > > - value = I915_READ(drbreg); > - WARN_ON((value & GEN8_DRB_VALID) != 0); > +static void guc_disable_doorbell(struct intel_guc *guc, > + struct i915_guc_client *client) > +{ > + struct guc_doorbell_info *doorbell; > > - I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0); > - I915_WRITE(drbreg, 0); > + doorbell = client->client_base + client->doorbell_offset; > + guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID); > > /* XXX: wait for any interrupts */ > /* XXX: wait for workqueue to drain */ > @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority) > if (id == end) > id = GUC_INVALID_DOORBELL_ID; > else > - bitmap_set(guc->doorbell_bitmap, id, 1); > + set_bit(id, guc->doorbell_bitmap); set_bit is atomic - is there a reason it is required here? Actually the same question for the clear_bit above, where it was bitmap_clear before. > > DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n", > hi_pri ? "high" : "normal", id); > @@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority) > return id; > } > > -static void release_doorbell(struct intel_guc *guc, uint16_t id) > -{ > - bitmap_clear(guc->doorbell_bitmap, id, 1); > -} > - > /* > * Initialise the process descriptor shared with the GuC firmware. > */ > @@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev, > */ > > if (client->client_base) { > - uint16_t db_id = client->doorbell_id; > - > /* > * If we got as far as setting up a doorbell, make sure > * we shut it down before unmapping & deallocating the > @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev, > * GuC that we've finished with it, finally deallocate > * it in our bitmap > */ > - if (db_id != GUC_INVALID_DOORBELL_ID) { > + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) > guc_disable_doorbell(guc, client); > - if (test_bit(db_id, guc->doorbell_bitmap)) > - host2guc_release_doorbell(guc, client); > - release_doorbell(guc, db_id); > - } > > kunmap(kmap_to_page(client->client_base)); > } > @@ -706,6 +722,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc *guc = &dev_priv->guc; > struct drm_i915_gem_object *obj; > + uint16_t db_id; > > client = kzalloc(sizeof(*client), GFP_KERNEL); > if (!client) > @@ -746,22 +763,24 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > else > client->proc_desc_offset = (GUC_DB_SIZE / 2); > > - client->doorbell_id = assign_doorbell(guc, client->priority); > - if (client->doorbell_id == GUC_INVALID_DOORBELL_ID) > + db_id = assign_doorbell(guc, client->priority); > + if (db_id == GUC_INVALID_DOORBELL_ID) > /* XXX: evict a doorbell instead */ > goto err; > > guc_init_proc_desc(guc, client); > guc_init_ctx_desc(guc, client); > - guc_init_doorbell(guc, client); > + guc_init_doorbell(guc, client, db_id); > > /* XXX: Any cache flushes needed? General domain mgmt calls? */ > > if (host2guc_allocate_doorbell(guc, client)) > goto err; > > - DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n", > - priority, client, client->ctx_index, client->doorbell_id); > + DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n", > + priority, client, client->ctx_index); > + DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n", > + client->doorbell_id, client->doorbell_offset); > > return client; > > Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] drm/i915/guc: refactor doorbell management code 2016-06-08 13:11 ` Tvrtko Ursulin @ 2016-06-10 11:37 ` Dave Gordon 0 siblings, 0 replies; 10+ messages in thread From: Dave Gordon @ 2016-06-10 11:37 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx On 08/06/16 14:11, Tvrtko Ursulin wrote: > > On 08/06/16 11:55, Dave Gordon wrote: >> During a hibernate/resume cycle, the driver, the GuC, and the doorbell >> hardware can end up in inconsistent states. This patch refactors the >> driver's handling and tracking of doorbells, in preparation for a later >> one which will resolve the issue. > > Perhaps a few word on the goal, method and result of refactoring. Would > be good for documentation and easier review. The refactoring is exactly that; there's very little new code. All we're doing is centralising all doorbell management into just one function and turning the (outermost) old functions into wrappers for the single function. There are three resources to be managed: 1. Cachelines: a single line within the client-object's page 0 is snooped for writes by the host. 2. Doorbell registers: each defines a cacheline to be snooped. 3. Bitmap: tracks which registers are in use. The doorbell setup/teardown protocol starts with: 1. Pick a cacheline: select_doorbell_cacheline() 2. Find an available doorbell (register): assign_doorbell() (These values are passed to the GuC via the shared context descriptor; this part of the sequence remains unchanged). 3. Update the bitmap to reflect registers-in-use 4. Prepare the cacheline for use by setting its status to ENABLED 5. Ask the GuC to program the doorbell to snoop the cacheline and of course teardown is very similar: 6. Set the cacheline to DISABLED 7. Ask the GuC to reprogram the doorbell to stop snooping 8. Record that the doorbell is not in use. Operations 6-8 (guc_disable_doorbell(), host2guc_release_doorbell(), and release_doorbell()) were called in sequence from guc_client_free(), but are now moved into the teardown part of the common function. Steps 4-5 (guc_init_doorbell() and host2guc_allocate_doorbell()) were similarly done as sequential steps in guc_client_alloc(), but since it turns out we don't need to do them separately they're now collected into the setup part of the common function. >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 87 >> ++++++++++++++++++------------ >> 1 file changed, 53 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 7510841..eef67c8 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct >> intel_guc *guc, >> * client object which contains the page being used for the doorbell >> */ >> >> -static void guc_init_doorbell(struct intel_guc *guc, >> - struct i915_guc_client *client) >> +static int guc_update_doorbell_id(struct i915_guc_client *client, >> + struct guc_doorbell_info *doorbell, >> + u16 new_id) >> { >> - struct guc_doorbell_info *doorbell; >> + struct sg_table *sg = client->guc->ctx_pool_obj->pages; >> + void *doorbell_bitmap = client->guc->doorbell_bitmap; >> + struct guc_context_desc desc; >> + size_t len; >> + >> + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID && >> + test_bit(client->doorbell_id, doorbell_bitmap)) { >> + /* Deactivate the old doorbell */ >> + doorbell->db_status = GUC_DOORBELL_DISABLED; >> + (void)host2guc_release_doorbell(client->guc, client); >> + clear_bit(client->doorbell_id, doorbell_bitmap); >> + } >> >> - doorbell = client->client_base + client->doorbell_offset; >> + /* Update the GuC's idea of the doorbell ID */ >> + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), >> + sizeof(desc) * client->ctx_index); >> + if (len != sizeof(desc)) >> + return -EFAULT; >> + desc.db_id = new_id; >> + len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc), >> + sizeof(desc) * client->ctx_index); >> + if (len != sizeof(desc)) >> + return -EFAULT; >> + >> + client->doorbell_id = new_id; >> + if (new_id == GUC_INVALID_DOORBELL_ID) >> + return 0; >> >> + /* Activate the new doorbell */ >> + set_bit(client->doorbell_id, doorbell_bitmap); >> doorbell->db_status = GUC_DOORBELL_ENABLED; >> doorbell->cookie = 0; >> + return host2guc_allocate_doorbell(client->guc, client); > > A bunch of new code here which wasn't done on doorbell init before. > Populating the ctx_pool_obj and hust2guc call. I think the commit needs > to explain what is happening here. The only new code (and new capability) is the block /* Update the GuC's idea of the doorbell ID */ i.e. we can now *change* the doorbell (register) used by an existing client, whereas previously it was set once for the entire lifetime of the client. We will use this new feature in the next patch. The host2guc call is not new, but it was previously called separately after the return from guc_init_doorbell(). > Maybe it is mostly a matter of copying over some text from the cover > letter, but ideally it should explain what it is doing more precisely. I'll put some of this reply into the commit message >> } >> >> -static void guc_disable_doorbell(struct intel_guc *guc, >> - struct i915_guc_client *client) >> +static void guc_init_doorbell(struct intel_guc *guc, >> + struct i915_guc_client *client, >> + uint16_t db_id) >> { >> - struct drm_i915_private *dev_priv = guc_to_i915(guc); >> struct guc_doorbell_info *doorbell; >> - i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id); >> - int value; >> >> doorbell = client->client_base + client->doorbell_offset; >> >> - doorbell->db_status = GUC_DOORBELL_DISABLED; >> - >> - I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID); >> + guc_update_doorbell_id(client, doorbell, db_id); >> +} > > This function does not end up doing anything now, just a pass-through to > guc_update_doorbell_id. Correct. It's retained to make it clear that here we're initialising a newly-created client with a doorbell assignment for the first time, as opposed to updating an existing doorbell assignment. Similarly we retain guc_disable_doorbell() as an alias for changing the doorbell ID (back) to INVALID. > What happens to the write to drbreg ? It is completely gone and also > from the init doorbell path together with the write to another register. Those writes were useless. Turns out those registers are not writeable by the CPU; they can *only* be updated by the GuC! >> - value = I915_READ(drbreg); >> - WARN_ON((value & GEN8_DRB_VALID) != 0); >> +static void guc_disable_doorbell(struct intel_guc *guc, >> + struct i915_guc_client *client) >> +{ >> + struct guc_doorbell_info *doorbell; >> >> - I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0); >> - I915_WRITE(drbreg, 0); >> + doorbell = client->client_base + client->doorbell_offset; >> + guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID); >> >> /* XXX: wait for any interrupts */ >> /* XXX: wait for workqueue to drain */ >> @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc >> *guc, uint32_t priority) >> if (id == end) >> id = GUC_INVALID_DOORBELL_ID; >> else >> - bitmap_set(guc->doorbell_bitmap, id, 1); >> + set_bit(id, guc->doorbell_bitmap); > > set_bit is atomic - is there a reason it is required here? > > Actually the same question for the clear_bit above, where it was > bitmap_clear before. The bitmap_set/clear functions don't have a comparable TEST operator, so we have to use test_bit(). The others were just changed for symmetry. We don't care about atomic or not, and this is rarely executed (setup/teardown) code so we don't worry about performance either. Actually, though, I'd expect the one-bit functions set_bit() and clear_bit() to be MORE efficient than the general multibit operations bitmap_set/clear. They should get inlined too :) We could use __set/clear_bit() if we really don't want the LOCK. New version later ... .Dave. >> DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n", >> hi_pri ? "high" : "normal", id); >> @@ -259,11 +286,6 @@ static uint16_t assign_doorbell(struct intel_guc >> *guc, uint32_t priority) >> return id; >> } >> >> -static void release_doorbell(struct intel_guc *guc, uint16_t id) >> -{ >> - bitmap_clear(guc->doorbell_bitmap, id, 1); >> -} >> - >> /* >> * Initialise the process descriptor shared with the GuC firmware. >> */ >> @@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev, >> */ >> >> if (client->client_base) { >> - uint16_t db_id = client->doorbell_id; >> - >> /* >> * If we got as far as setting up a doorbell, make sure >> * we shut it down before unmapping & deallocating the >> @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev, >> * GuC that we've finished with it, finally deallocate >> * it in our bitmap >> */ >> - if (db_id != GUC_INVALID_DOORBELL_ID) { >> + if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) >> guc_disable_doorbell(guc, client); >> - if (test_bit(db_id, guc->doorbell_bitmap)) >> - host2guc_release_doorbell(guc, client); >> - release_doorbell(guc, db_id); >> - } >> >> kunmap(kmap_to_page(client->client_base)); >> } >> @@ -706,6 +722,7 @@ static struct i915_guc_client >> *guc_client_alloc(struct drm_device *dev, >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_guc *guc = &dev_priv->guc; >> struct drm_i915_gem_object *obj; >> + uint16_t db_id; >> >> client = kzalloc(sizeof(*client), GFP_KERNEL); >> if (!client) >> @@ -746,22 +763,24 @@ static struct i915_guc_client >> *guc_client_alloc(struct drm_device *dev, >> else >> client->proc_desc_offset = (GUC_DB_SIZE / 2); >> >> - client->doorbell_id = assign_doorbell(guc, client->priority); >> - if (client->doorbell_id == GUC_INVALID_DOORBELL_ID) >> + db_id = assign_doorbell(guc, client->priority); >> + if (db_id == GUC_INVALID_DOORBELL_ID) >> /* XXX: evict a doorbell instead */ >> goto err; >> >> guc_init_proc_desc(guc, client); >> guc_init_ctx_desc(guc, client); >> - guc_init_doorbell(guc, client); >> + guc_init_doorbell(guc, client, db_id); >> >> /* XXX: Any cache flushes needed? General domain mgmt calls? */ >> >> if (host2guc_allocate_doorbell(guc, client)) >> goto err; >> >> - DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id >> %u\n", >> - priority, client, client->ctx_index, client->doorbell_id); >> + DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n", >> + priority, client, client->ctx_index); >> + DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n", >> + client->doorbell_id, client->doorbell_offset); >> >> return client; >> >> > > Regards, > Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/4] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon ` (2 preceding siblings ...) 2016-06-08 10:55 ` [PATCH 3/4] drm/i915/guc: refactor doorbell management code Dave Gordon @ 2016-06-08 10:55 ` Dave Gordon 2016-06-08 11:41 ` ✓ Ro.CI.BAT: success for drm/i915: updates to GuC doorbell handling Patchwork 4 siblings, 0 replies; 10+ messages in thread From: Dave Gordon @ 2016-06-08 10:55 UTC (permalink / raw) To: intel-gfx During a hibernate/resume cycle, the whole system is reset, including the GuC and the doorbell hardware. Then the system is booted up, drivers are loaded, etc -- the GuC firmware may be loaded and set running at this point. But then, the booted kernel is replaced by the hibernated image, and this resumed kernel will also try to reload the GuC firmware (which will fail, because the GuC firmware area is write-once). To recover, we reset the GuC and try again (which should work). But this GuC reset *doesn't* also reset the doorbell hardware, so it can be left in a state inconsistent with that assumed by the driver and the GuC. It would be better if the GuC reset also cleared all doorbell state, but that's not how the hardware currently works; also, the driver cannot directly reprogram the doorbell hardware (only the GuC can do that). So this patch cycles through all doorbells, assigning and releasing each in turn, so that all the doorbell hardware is left in a consistent state, no matter how it was programmed by the previously-running kernel and/or GuC firmware. v2: don't use kmap_atomic() now that client page 0 is kept mapped. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 43 ++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index eef67c8..7aba25b 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -220,7 +220,6 @@ static void guc_init_doorbell(struct intel_guc *guc, struct guc_doorbell_info *doorbell; doorbell = client->client_base + client->doorbell_offset; - guc_update_doorbell_id(client, doorbell, db_id); } @@ -702,6 +701,46 @@ static void guc_client_free(struct drm_device *dev, kfree(client); } +/* + * Borrow the first client to set up & tear down every doorbell + * in turn, to ensure that all doorbell h/w is (re)initialised. + */ +static void guc_init_doorbell_hw(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + struct i915_guc_client *client = guc->execbuf_client; + struct guc_doorbell_info *doorbell; + uint16_t db_id, i; + int ret; + + doorbell = client->client_base + client->doorbell_offset; + db_id = client->doorbell_id; + + for (i = 0; i < GUC_MAX_DOORBELLS; ++i) { + i915_reg_t drbreg = GEN8_DRBREGL(i); + u32 value = I915_READ(drbreg); + + ret = guc_update_doorbell_id(client, doorbell, i); + + if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret) + DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n", + drbreg.reg, value, ret); + } + + /* Restore to original value */ + guc_update_doorbell_id(client, doorbell, db_id); + + for (i = 0; i < GUC_MAX_DOORBELLS; ++i) { + i915_reg_t drbreg = GEN8_DRBREGL(i); + u32 value = I915_READ(drbreg); + + if ((value & GUC_DOORBELL_ENABLED) && (i != db_id)) + DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n", + drbreg.reg, value); + + } +} + /** * guc_client_alloc() - Allocate an i915_guc_client * @dev: drm device @@ -971,8 +1010,8 @@ int i915_guc_submission_enable(struct drm_device *dev) } guc->execbuf_client = client; - host2guc_sample_forcewake(guc, client); + guc_init_doorbell_hw(guc); return 0; } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✓ Ro.CI.BAT: success for drm/i915: updates to GuC doorbell handling 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon ` (3 preceding siblings ...) 2016-06-08 10:55 ` [PATCH 4/4] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission Dave Gordon @ 2016-06-08 11:41 ` Patchwork 4 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2016-06-08 11:41 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx == Series Details == Series: drm/i915: updates to GuC doorbell handling URL : https://patchwork.freedesktop.org/series/8441/ State : success == Summary == Series 8441v1 drm/i915: updates to GuC doorbell handling http://patchwork.freedesktop.org/api/1.0/series/8441/revisions/1/mbox fi-bdw-i7-5557u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 fi-skl-i5-6260u total:209 pass:198 dwarn:0 dfail:0 fail:0 skip:11 fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 skip:25 fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 skip:39 ro-bdw-i5-5250u total:183 pass:172 dwarn:0 dfail:0 fail:0 skip:10 ro-bsw-n3050 total:208 pass:167 dwarn:0 dfail:0 fail:2 skip:39 ro-byt-n2820 total:208 pass:168 dwarn:0 dfail:0 fail:3 skip:37 ro-hsw-i3-4010u total:208 pass:185 dwarn:0 dfail:0 fail:0 skip:23 ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 skip:57 ro-ivb-i7-3770 total:183 pass:154 dwarn:0 dfail:0 fail:0 skip:28 ro-ivb2-i7-3770 total:183 pass:158 dwarn:0 dfail:0 fail:0 skip:24 ro-snb-i7-2620M total:183 pass:151 dwarn:0 dfail:0 fail:0 skip:31 fi-hsw-i7-4770k failed to connect after reboot ro-bdw-i7-5557U failed to connect after reboot ro-bdw-i7-5600u failed to connect after reboot ro-hsw-i7-4770r failed to connect after reboot Results at /archive/results/CI_IGT_test/RO_Patchwork_1137/ 131a54b drm-intel-nightly: 2016y-06m-07d-19h-46m-33s UTC integration manifest d22be7d4 drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission b7d2be1 drm/i915/guc: refactor doorbell management code e566931 drm/i915/guc: move guc_ring_doorbell() nearer to callsite cd5f059 drm/i915/guc: add doorbell map to debugfs/i915_guc_info _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-10 11:37 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-08 10:55 [PATCH 0/4] drm/i915: updates to GuC doorbell handling Dave Gordon 2016-06-08 10:55 ` [PATCH 1/4] drm/i915/guc: add doorbell map to debugfs/i915_guc_info Dave Gordon 2016-06-08 12:32 ` Tvrtko Ursulin 2016-06-08 10:55 ` [PATCH 2/4] drm/i915/guc: move guc_ring_doorbell() nearer to callsite Dave Gordon 2016-06-08 12:47 ` Tvrtko Ursulin 2016-06-08 10:55 ` [PATCH 3/4] drm/i915/guc: refactor doorbell management code Dave Gordon 2016-06-08 13:11 ` Tvrtko Ursulin 2016-06-10 11:37 ` Dave Gordon 2016-06-08 10:55 ` [PATCH 4/4] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission Dave Gordon 2016-06-08 11:41 ` ✓ Ro.CI.BAT: success for drm/i915: updates to GuC doorbell handling Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.