* [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
@ 2017-02-02 15:27 Oscar Mateo
2017-02-02 23:52 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-02 15:27 UTC (permalink / raw)
To: intel-gfx; +Cc: Deepak S
From: Michal Wajdeczko <michal.wajdeczko@intel.com>
The GuC descriptor is big in size. If we use local definition of
guc_desc we have a chance to overflow stack. Use allocated one.
v2: Rebased
v3: Split
v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
1 file changed, 57 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8ced9e2..b4f14f3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
struct sg_table *sg = guc->ctx_pool_vma->pages;
void *doorbell_bitmap = guc->doorbell_bitmap;
struct guc_doorbell_info *doorbell;
- struct guc_context_desc desc;
+ struct guc_context_desc *desc;
size_t len;
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
doorbell = client->vaddr + client->doorbell_offset;
if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
@@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
}
/* 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))
+ len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+ sizeof(*desc) * client->ctx_index);
+ if (len != sizeof(*desc)) {
+ kfree(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))
+ }
+
+ 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)) {
+ kfree(desc);
return -EFAULT;
+ }
+
+ kfree(desc);
client->doorbell_id = new_id;
if (new_id == GUC_INVALID_DOORBELL_ID)
@@ -229,30 +240,33 @@ static void guc_proc_desc_init(struct intel_guc *guc,
* This descriptor tells the GuC where (in GGTT space) to find the important
* data structures relating to this client (doorbell, process descriptor,
* write queue, etc).
+ *
+ * Returns: 0 on success, negative error code on failure.
*/
-
-static void guc_ctx_desc_init(struct intel_guc *guc,
+static int guc_ctx_desc_init(struct intel_guc *guc,
struct i915_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
- struct guc_context_desc desc;
+ struct guc_context_desc *desc;
struct sg_table *sg;
unsigned int tmp;
u32 gfx_addr;
- memset(&desc, 0, sizeof(desc));
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
- desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
- desc.context_id = client->ctx_index;
- desc.priority = client->priority;
- desc.db_id = client->doorbell_id;
+ desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+ desc->context_id = client->ctx_index;
+ desc->priority = client->priority;
+ desc->db_id = client->doorbell_id;
for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
struct intel_context *ce = &ctx->engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
- struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+ struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
/* TODO: We have a design issue to be solved here. Only when we
* receive the first batch, we know which engine is used by the
@@ -277,50 +291,56 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
- desc.engines_used |= (1 << guc_engine_id);
+ desc->engines_used |= (1 << guc_engine_id);
}
DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
- client->engines, desc.engines_used);
- WARN_ON(desc.engines_used == 0);
+ client->engines, desc->engines_used);
+ WARN_ON(desc->engines_used == 0);
/*
* The doorbell, process descriptor, and workqueue are all parts
* of the client object, which the GuC will reference via the GGTT
*/
gfx_addr = guc_ggtt_offset(client->vma);
- desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+ desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
client->doorbell_offset;
- desc.db_trigger_cpu =
- (uintptr_t)client->vaddr + client->doorbell_offset;
- desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
- desc.process_desc = gfx_addr + client->proc_desc_offset;
- desc.wq_addr = gfx_addr + client->wq_offset;
- desc.wq_size = client->wq_size;
+ desc->db_trigger_cpu = (uintptr_t)client->vaddr +
+ client->doorbell_offset;
+ desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+ desc->process_desc = gfx_addr + client->proc_desc_offset;
+ desc->wq_addr = gfx_addr + client->wq_offset;
+ desc->wq_size = client->wq_size;
/*
* XXX: Take LRCs from an existing context if this is not an
* IsKMDCreatedContext client
*/
- desc.desc_private = (uintptr_t)client;
+ desc->desc_private = (uintptr_t)client;
/* Pool context is pinned already */
sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ sg_pcopy_from_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+ sizeof(*desc) * client->ctx_index);
+
+ kfree(desc);
+ return 0;
}
static void guc_ctx_desc_fini(struct intel_guc *guc,
struct i915_guc_client *client)
{
- struct guc_context_desc desc;
+ struct guc_context_desc *desc;
struct sg_table *sg;
- memset(&desc, 0, sizeof(desc));
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return;
sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ sg_pcopy_from_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
+ sizeof(*desc) * client->ctx_index);
+ kfree(desc);
}
/**
@@ -751,7 +771,9 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
client->proc_desc_offset = (GUC_DB_SIZE / 2);
guc_proc_desc_init(guc, client);
- guc_ctx_desc_init(guc, client);
+
+ if (guc_ctx_desc_init(guc, client) < 0)
+ goto err;
/* For runtime client allocation we need to enable the doorbell. Not
* required yet for the static execbuf_client as this special kernel
@@ -1040,5 +1062,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
return intel_guc_send(guc, data, ARRAY_SIZE(data));
}
-
-
--
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] 15+ messages in thread* ✓ Fi.CI.BAT: success for drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-02 15:27 [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor Oscar Mateo
@ 2017-02-02 23:52 ` Patchwork
2017-02-03 7:33 ` [PATCH] " Chris Wilson
2017-02-09 18:52 ` ✓ Fi.CI.BAT: success for drm/i915/guc: Dynamically alloc GuC descriptor (rev2) Patchwork
2 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-02-02 23:52 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: Dynamically alloc GuC descriptor
URL : https://patchwork.freedesktop.org/series/19022/
State : success
== Summary ==
Series 19022v1 drm/i915/guc: Dynamically alloc GuC descriptor
https://patchwork.freedesktop.org/api/1.0/series/19022/revisions/1/mbox/
fi-bdw-5557u total:247 pass:233 dwarn:0 dfail:0 fail:0 skip:14
fi-bsw-n3050 total:247 pass:208 dwarn:0 dfail:0 fail:0 skip:39
fi-bxt-j4205 total:247 pass:225 dwarn:0 dfail:0 fail:0 skip:22
fi-byt-j1900 total:247 pass:220 dwarn:0 dfail:0 fail:0 skip:27
fi-byt-n2820 total:247 pass:216 dwarn:0 dfail:0 fail:0 skip:31
fi-hsw-4770 total:247 pass:228 dwarn:0 dfail:0 fail:0 skip:19
fi-hsw-4770r total:247 pass:228 dwarn:0 dfail:0 fail:0 skip:19
fi-ivb-3520m total:247 pass:226 dwarn:0 dfail:0 fail:0 skip:21
fi-ivb-3770 total:247 pass:226 dwarn:0 dfail:0 fail:0 skip:21
fi-skl-6260u total:247 pass:234 dwarn:0 dfail:0 fail:0 skip:13
fi-skl-6700hq total:247 pass:227 dwarn:0 dfail:0 fail:0 skip:20
fi-skl-6700k total:247 pass:222 dwarn:4 dfail:0 fail:0 skip:21
fi-skl-6770hq total:247 pass:234 dwarn:0 dfail:0 fail:0 skip:13
fi-snb-2520m total:247 pass:216 dwarn:0 dfail:0 fail:0 skip:31
fi-snb-2600 total:247 pass:215 dwarn:0 dfail:0 fail:0 skip:32
fi-bxt-t5700 failed to collect. IGT log at Patchwork_3683/fi-bxt-t5700/igt.log
0f01216949002d20b9dc6d300c82df5ffa59e9a7 drm-tip: 2017y-02m-02d-19h-49m-15s UTC integration manifest
3cca7cd drm/i915/guc: Dynamically alloc GuC descriptor
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3683/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-02 15:27 [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor Oscar Mateo
2017-02-02 23:52 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-02-03 7:33 ` Chris Wilson
2017-02-07 8:55 ` Oscar Mateo
2017-02-09 18:52 ` ✓ Fi.CI.BAT: success for drm/i915/guc: Dynamically alloc GuC descriptor (rev2) Patchwork
2 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-03 7:33 UTC (permalink / raw)
To: Oscar Mateo; +Cc: Deepak S, intel-gfx
On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> The GuC descriptor is big in size. If we use local definition of
> guc_desc we have a chance to overflow stack. Use allocated one.
>
> v2: Rebased
> v3: Split
> v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
> 1 file changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 8ced9e2..b4f14f3 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> struct sg_table *sg = guc->ctx_pool_vma->pages;
> void *doorbell_bitmap = guc->doorbell_bitmap;
> struct guc_doorbell_info *doorbell;
> - struct guc_context_desc desc;
> + struct guc_context_desc *desc;
> size_t len;
>
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return -ENOMEM;
> +
> doorbell = client->vaddr + client->doorbell_offset;
>
> if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> @@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> }
>
> /* 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))
> + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
> + sizeof(*desc) * client->ctx_index);
This is silly. You are creating a pointer using kmalloc to copy into a
pointer created using alloc_page. Just write directly into the backing
store.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-03 7:33 ` [PATCH] " Chris Wilson
@ 2017-02-07 8:55 ` Oscar Mateo
2017-02-07 17:25 ` Chris Wilson
0 siblings, 1 reply; 15+ messages in thread
From: Oscar Mateo @ 2017-02-07 8:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: Deepak S, intel-gfx
On 02/02/2017 11:33 PM, Chris Wilson wrote:
> On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> The GuC descriptor is big in size. If we use local definition of
>> guc_desc we have a chance to overflow stack. Use allocated one.
>>
>> v2: Rebased
>> v3: Split
>> v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
>>
>> Signed-off-by: Deepak S <deepak.s@intel.com>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
>> 1 file changed, 57 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 8ced9e2..b4f14f3 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>> struct sg_table *sg = guc->ctx_pool_vma->pages;
>> void *doorbell_bitmap = guc->doorbell_bitmap;
>> struct guc_doorbell_info *doorbell;
>> - struct guc_context_desc desc;
>> + struct guc_context_desc *desc;
>> size_t len;
>>
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc)
>> + return -ENOMEM;
>> +
>> doorbell = client->vaddr + client->doorbell_offset;
>>
>> if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
>> @@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>> }
>>
>> /* 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))
>> + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
>> + sizeof(*desc) * client->ctx_index);
> This is silly. You are creating a pointer using kmalloc to copy into a
> pointer created using alloc_page. Just write directly into the backing
> store.
> -Chris
>
I guess I deserve this for not digging any deeper. From what I can see,
the backing store is an array of 1024 context descriptors. If the whole
context descriptor fell in one page, I could kmap_atomic only that. As
it is, I would need to vmap a couple of pages to make sure I always get
a complete pointer to guc_context_desc. Would you be happy with that?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-07 8:55 ` Oscar Mateo
@ 2017-02-07 17:25 ` Chris Wilson
2017-02-07 9:37 ` Oscar Mateo
0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-07 17:25 UTC (permalink / raw)
To: Oscar Mateo; +Cc: Deepak S, intel-gfx
On Tue, Feb 07, 2017 at 12:55:21AM -0800, Oscar Mateo wrote:
>
>
> On 02/02/2017 11:33 PM, Chris Wilson wrote:
> >On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
> >>From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>
> >>The GuC descriptor is big in size. If we use local definition of
> >>guc_desc we have a chance to overflow stack. Use allocated one.
> >>
> >>v2: Rebased
> >>v3: Split
> >>v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
> >>
> >>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> >>---
> >> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
> >> 1 file changed, 57 insertions(+), 37 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>index 8ced9e2..b4f14f3 100644
> >>--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >>+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>@@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> >> struct sg_table *sg = guc->ctx_pool_vma->pages;
> >> void *doorbell_bitmap = guc->doorbell_bitmap;
> >> struct guc_doorbell_info *doorbell;
> >>- struct guc_context_desc desc;
> >>+ struct guc_context_desc *desc;
> >> size_t len;
> >>+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> >>+ if (!desc)
> >>+ return -ENOMEM;
> >>+
> >> doorbell = client->vaddr + client->doorbell_offset;
> >> if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> >>@@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> >> }
> >> /* 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))
> >>+ len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
> >>+ sizeof(*desc) * client->ctx_index);
> >This is silly. You are creating a pointer using kmalloc to copy into a
> >pointer created using alloc_page. Just write directly into the backing
> >store.
>
> I guess I deserve this for not digging any deeper. From what I can
> see, the backing store is an array of 1024 context descriptors. If
> the whole context descriptor fell in one page, I could kmap_atomic
> only that. As it is, I would need to vmap a couple of pages to make
> sure I always get a complete pointer to guc_context_desc. Would you
> be happy with that?
One of the suggested usecases for i915_gem_object_pin_map() was this code.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-07 17:25 ` Chris Wilson
@ 2017-02-07 9:37 ` Oscar Mateo
2017-02-07 20:49 ` Chris Wilson
0 siblings, 1 reply; 15+ messages in thread
From: Oscar Mateo @ 2017-02-07 9:37 UTC (permalink / raw)
To: Chris Wilson; +Cc: Deepak S, intel-gfx
On 02/07/2017 09:25 AM, Chris Wilson wrote:
> On Tue, Feb 07, 2017 at 12:55:21AM -0800, Oscar Mateo wrote:
>>
>> On 02/02/2017 11:33 PM, Chris Wilson wrote:
>>> On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
>>>> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>
>>>> The GuC descriptor is big in size. If we use local definition of
>>>> guc_desc we have a chance to overflow stack. Use allocated one.
>>>>
>>>> v2: Rebased
>>>> v3: Split
>>>> v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
>>>>
>>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
>>>> 1 file changed, 57 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 8ced9e2..b4f14f3 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>>>> struct sg_table *sg = guc->ctx_pool_vma->pages;
>>>> void *doorbell_bitmap = guc->doorbell_bitmap;
>>>> struct guc_doorbell_info *doorbell;
>>>> - struct guc_context_desc desc;
>>>> + struct guc_context_desc *desc;
>>>> size_t len;
>>>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>>>> + if (!desc)
>>>> + return -ENOMEM;
>>>> +
>>>> doorbell = client->vaddr + client->doorbell_offset;
>>>> if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
>>>> @@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>>>> }
>>>> /* 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))
>>>> + len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
>>>> + sizeof(*desc) * client->ctx_index);
>>> This is silly. You are creating a pointer using kmalloc to copy into a
>>> pointer created using alloc_page. Just write directly into the backing
>>> store.
>> I guess I deserve this for not digging any deeper. From what I can
>> see, the backing store is an array of 1024 context descriptors. If
>> the whole context descriptor fell in one page, I could kmap_atomic
>> only that. As it is, I would need to vmap a couple of pages to make
>> sure I always get a complete pointer to guc_context_desc. Would you
>> be happy with that?
> One of the suggested usecases for i915_gem_object_pin_map() was this code.
> -Chris
I considered it, but with the current interface that would mean vmapping
the whole thing (something like 70 pages). Isn't that a bit overkill?
I know you are going to say it wastes memory, but (KISS) what about if I
make guc_context_desc part of i915_guc_client, to be used for sg_pcopy
operations?.
Although I am getting the vibe that you have discussed the sg_pcopy
thing in the past, and this is not only about avoiding potential stack
overflows. Am I right?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor
2017-02-07 9:37 ` Oscar Mateo
@ 2017-02-07 20:49 ` Chris Wilson
2017-02-09 10:23 ` [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-07 20:49 UTC (permalink / raw)
To: Oscar Mateo; +Cc: Deepak S, intel-gfx
On Tue, Feb 07, 2017 at 01:37:52AM -0800, Oscar Mateo wrote:
>
>
> On 02/07/2017 09:25 AM, Chris Wilson wrote:
> >On Tue, Feb 07, 2017 at 12:55:21AM -0800, Oscar Mateo wrote:
> >>
> >>On 02/02/2017 11:33 PM, Chris Wilson wrote:
> >>>On Thu, Feb 02, 2017 at 07:27:45AM -0800, Oscar Mateo wrote:
> >>>>From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>>>
> >>>>The GuC descriptor is big in size. If we use local definition of
> >>>>guc_desc we have a chance to overflow stack. Use allocated one.
> >>>>
> >>>>v2: Rebased
> >>>>v3: Split
> >>>>v4: Handle ENOMEM, cover all uses of guc_context_desc, use kzalloc (Oscar)
> >>>>
> >>>>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>>>Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>>>Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> >>>>---
> >>>> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++++++------------
> >>>> 1 file changed, 57 insertions(+), 37 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>>>index 8ced9e2..b4f14f3 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>>>@@ -102,9 +102,13 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> >>>> struct sg_table *sg = guc->ctx_pool_vma->pages;
> >>>> void *doorbell_bitmap = guc->doorbell_bitmap;
> >>>> struct guc_doorbell_info *doorbell;
> >>>>- struct guc_context_desc desc;
> >>>>+ struct guc_context_desc *desc;
> >>>> size_t len;
> >>>>+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> >>>>+ if (!desc)
> >>>>+ return -ENOMEM;
> >>>>+
> >>>> doorbell = client->vaddr + client->doorbell_offset;
> >>>> if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> >>>>@@ -116,15 +120,22 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> >>>> }
> >>>> /* 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))
> >>>>+ len = sg_pcopy_to_buffer(sg->sgl, sg->nents, desc, sizeof(*desc),
> >>>>+ sizeof(*desc) * client->ctx_index);
> >>>This is silly. You are creating a pointer using kmalloc to copy into a
> >>>pointer created using alloc_page. Just write directly into the backing
> >>>store.
> >>I guess I deserve this for not digging any deeper. From what I can
> >>see, the backing store is an array of 1024 context descriptors. If
> >>the whole context descriptor fell in one page, I could kmap_atomic
> >>only that. As it is, I would need to vmap a couple of pages to make
> >>sure I always get a complete pointer to guc_context_desc. Would you
> >>be happy with that?
> >One of the suggested usecases for i915_gem_object_pin_map() was this code.
> >-Chris
>
> I considered it, but with the current interface that would mean
> vmapping the whole thing (something like 70 pages). Isn't that a bit
> overkill?
The whole object is pinned into memory and occupies aperture space, and
all will be used at some point. Keeping a small vmap is not a huge cost
for a reasonably frequently used object.
> I know you are going to say it wastes memory, but (KISS) what about
> if I make guc_context_desc part of i915_guc_client, to be used for
> sg_pcopy operations?.
> Although I am getting the vibe that you have discussed the sg_pcopy
> thing in the past, and this is not only about avoiding potential
> stack overflows. Am I right?
More that I have an abhorence for scatterlist (since it appears so high
on profiles). At the very least use i915_gem_object_get_page() as that
will use the radixtree for a fast lookup.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-07 20:49 ` Chris Wilson
@ 2017-02-09 10:23 ` Oscar Mateo
2017-02-09 21:01 ` Chris Wilson
2017-02-15 12:37 ` Joonas Lahtinen
0 siblings, 2 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-09 10:23 UTC (permalink / raw)
To: intel-gfx
From: Michal Wajdeczko <michal.wajdeczko@intel.com>
The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.
Also, Chris abhors scatterlists :)
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 83 +++++++++++++-----------------
drivers/gpu/drm/i915/intel_uc.h | 1 +
2 files changed, 37 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8ced9e2..0065b38 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -99,11 +99,9 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
struct i915_guc_client *client,
u16 new_id)
{
- struct sg_table *sg = guc->ctx_pool_vma->pages;
void *doorbell_bitmap = guc->doorbell_bitmap;
struct guc_doorbell_info *doorbell;
- struct guc_context_desc desc;
- size_t len;
+ struct guc_context_desc *desc;
doorbell = client->vaddr + client->doorbell_offset;
@@ -116,15 +114,8 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
}
/* 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;
+ desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
+ desc->db_id = new_id;
client->doorbell_id = new_id;
if (new_id == GUC_INVALID_DOORBELL_ID)
@@ -230,29 +221,27 @@ static void guc_proc_desc_init(struct intel_guc *guc,
* data structures relating to this client (doorbell, process descriptor,
* write queue, etc).
*/
-
static void guc_ctx_desc_init(struct intel_guc *guc,
struct i915_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
- struct guc_context_desc desc;
- struct sg_table *sg;
+ struct guc_context_desc *desc;
unsigned int tmp;
u32 gfx_addr;
- memset(&desc, 0, sizeof(desc));
+ desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
- desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
- desc.context_id = client->ctx_index;
- desc.priority = client->priority;
- desc.db_id = client->doorbell_id;
+ desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+ desc->context_id = client->ctx_index;
+ desc->priority = client->priority;
+ desc->db_id = client->doorbell_id;
for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
struct intel_context *ce = &ctx->engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
- struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+ struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
/* TODO: We have a design issue to be solved here. Only when we
* receive the first batch, we know which engine is used by the
@@ -277,50 +266,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
- desc.engines_used |= (1 << guc_engine_id);
+ desc->engines_used |= (1 << guc_engine_id);
}
DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
- client->engines, desc.engines_used);
- WARN_ON(desc.engines_used == 0);
+ client->engines, desc->engines_used);
+ WARN_ON(desc->engines_used == 0);
/*
* The doorbell, process descriptor, and workqueue are all parts
* of the client object, which the GuC will reference via the GGTT
*/
gfx_addr = guc_ggtt_offset(client->vma);
- desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+ desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
client->doorbell_offset;
- desc.db_trigger_cpu =
- (uintptr_t)client->vaddr + client->doorbell_offset;
- desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
- desc.process_desc = gfx_addr + client->proc_desc_offset;
- desc.wq_addr = gfx_addr + client->wq_offset;
- desc.wq_size = client->wq_size;
+ desc->db_trigger_cpu = (uintptr_t)client->vaddr +
+ client->doorbell_offset;
+ desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+ desc->process_desc = gfx_addr + client->proc_desc_offset;
+ desc->wq_addr = gfx_addr + client->wq_offset;
+ desc->wq_size = client->wq_size;
/*
* XXX: Take LRCs from an existing context if this is not an
* IsKMDCreatedContext client
*/
- desc.desc_private = (uintptr_t)client;
-
- /* Pool context is pinned already */
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc->desc_private = (uintptr_t)client;
}
static void guc_ctx_desc_fini(struct intel_guc *guc,
struct i915_guc_client *client)
{
- struct guc_context_desc desc;
- struct sg_table *sg;
+ struct guc_context_desc *desc;
- memset(&desc, 0, sizeof(desc));
-
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
+ memset(desc, 0, sizeof(*desc));
}
/**
@@ -876,6 +856,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
const size_t gemsize = round_up(poolsize, PAGE_SIZE);
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
+ void *vaddr;
if (!HAS_GUC_SCHED(dev_priv))
return 0;
@@ -895,6 +876,13 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
return PTR_ERR(vma);
guc->ctx_pool_vma = vma;
+
+ vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+ if (IS_ERR(vaddr))
+ goto err;
+
+ guc->ctx_pool_vaddr = vaddr;
+
ida_init(&guc->ctx_ids);
intel_guc_log_create(guc);
guc_addon_create(guc);
@@ -983,8 +971,11 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
i915_vma_unpin_and_release(&guc->ads_vma);
i915_vma_unpin_and_release(&guc->log.vma);
- if (guc->ctx_pool_vma)
+ if (guc->ctx_pool_vaddr) {
ida_destroy(&guc->ctx_ids);
+ i915_gem_object_unpin_map(guc->ctx_pool_vma->obj);
+ }
+
i915_vma_unpin_and_release(&guc->ctx_pool_vma);
}
@@ -1040,5 +1031,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
return intel_guc_send(guc, data, ARRAY_SIZE(data));
}
-
-
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 8a33a46..29ee373 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -155,6 +155,7 @@ struct intel_guc {
struct i915_vma *ads_vma;
struct i915_vma *ctx_pool_vma;
+ void *ctx_pool_vaddr;
struct ida ctx_ids;
struct i915_guc_client *execbuf_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] 15+ messages in thread* Re: [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-09 10:23 ` [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
@ 2017-02-09 21:01 ` Chris Wilson
2017-02-13 16:05 ` Oscar Mateo
2017-02-15 12:37 ` Joonas Lahtinen
1 sibling, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-09 21:01 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Thu, Feb 09, 2017 at 02:23:39AM -0800, Oscar Mateo wrote:
> @@ -116,15 +114,8 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> }
>
> /* 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;
> + desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
Repeated quite a few times.
static inline struct guc_context_desc *
guc_context_desc(struct intel_guc *guc,
struct i915_guc_client *client)
{
return (guc->ctx_pool_vaddr +
sizeof(struct guc_context_desc) * client->ctx_index);
Or your preference
}
Lots of little nitpicks I could make. *summons Joonas.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-09 21:01 ` Chris Wilson
@ 2017-02-13 16:05 ` Oscar Mateo
0 siblings, 0 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-13 16:05 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On 02/09/2017 01:01 PM, Chris Wilson wrote:
> On Thu, Feb 09, 2017 at 02:23:39AM -0800, Oscar Mateo wrote:
>> @@ -116,15 +114,8 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>> }
>>
>> /* 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;
>> + desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
> Repeated quite a few times.
>
> static inline struct guc_context_desc *
> guc_context_desc(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> return (guc->ctx_pool_vaddr +
> sizeof(struct guc_context_desc) * client->ctx_index);
>
> Or your preference
> }
It's repeated only three times, just one line long and it doesn't look
like we are going to need more instances, so I didn't see the need to
make it common code, but I'm OK with changing it.
> Lots of little nitpicks I could make. *summons Joonas.
> -Chris
Adding Joonas in CC, so that the summon spell is more effective.
-- Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-09 10:23 ` [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
2017-02-09 21:01 ` Chris Wilson
@ 2017-02-15 12:37 ` Joonas Lahtinen
2017-02-16 14:15 ` [PATCH v2] " Oscar Mateo
1 sibling, 1 reply; 15+ messages in thread
From: Joonas Lahtinen @ 2017-02-15 12:37 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On to, 2017-02-09 at 02:23 -0800, Oscar Mateo wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> The GuC descriptor is big in size. If we use a local definition of
> guc_desc we have a chance to overflow stack, so avoid it.
>
> Also, Chris abhors scatterlists :)
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
<SNIP>
> @@ -116,15 +114,8 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
> }
>
> /* 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;
> + desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;
As Chris mentioned, could use a __get_doorbell_desc helper for this.
> @@ -895,6 +876,13 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> return PTR_ERR(vma);
>
> guc->ctx_pool_vma = vma;
> +
> + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> + if (IS_ERR(vaddr))
After onion teardown added to this function, propagate errors;
err = ERR_PTR(vaddr);
> + goto err;
> +
> + guc->ctx_pool_vaddr = vaddr;
> +
> @@ -983,8 +971,11 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> i915_vma_unpin_and_release(&guc->ads_vma);
> i915_vma_unpin_and_release(&guc->log.vma);
>
> - if (guc->ctx_pool_vma)
> + if (guc->ctx_pool_vaddr) {
> ida_destroy(&guc->ctx_ids);
> + i915_gem_object_unpin_map(guc->ctx_pool_vma->obj);
> + }
This converted to unconditional chain of teardown.
> +
> i915_vma_unpin_and_release(&guc->ctx_pool_vma);
> }
>
> @@ -1040,5 +1031,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>
> > return intel_guc_send(guc, data, ARRAY_SIZE(data));
> }
> -
> -
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 8a33a46..29ee373 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -155,6 +155,7 @@ struct intel_guc {
>
> struct i915_vma *ads_vma;
> struct i915_vma *ctx_pool_vma;
s/ctx_pool_vma/ctx_pool/ would bring out the dependency explicitly.
> + void *ctx_pool_vaddr;
> struct ida ctx_ids;
>
> struct i915_guc_client *execbuf_client;
This needs a rebase on top of my cleanup, but is very welcome
improvement.
The types of variables are still wild, but not introduced by this
change.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-15 12:37 ` Joonas Lahtinen
@ 2017-02-16 14:15 ` Oscar Mateo
2017-02-16 14:18 ` Oscar Mateo
2017-02-16 22:50 ` Chris Wilson
0 siblings, 2 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-16 14:15 UTC (permalink / raw)
To: intel-gfx
The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.
Also, Chris abhors scatterlists :)
v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++----------------
drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
drivers/gpu/drm/i915/intel_uc.h | 3 +-
3 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3ea2c71..f7d9897 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+{
+ return client->guc->ctx_pool_vaddr +
+ sizeof(struct guc_context_desc) * client->ctx_index;
+}
+
/*
* Initialise, update, or clear doorbell data shared with the GuC
*
@@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
{
- struct sg_table *sg = client->guc->ctx_pool_vma->pages;
- struct guc_context_desc desc;
- size_t len;
+ struct guc_context_desc *desc;
/* 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;
+ desc = __get_context_desc(client);
+ desc->db_id = new_id;
return 0;
}
@@ -270,29 +266,27 @@ static void guc_proc_desc_init(struct intel_guc *guc,
* data structures relating to this client (doorbell, process descriptor,
* write queue, etc).
*/
-
static void guc_ctx_desc_init(struct intel_guc *guc,
struct i915_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
- struct guc_context_desc desc;
- struct sg_table *sg;
+ struct guc_context_desc *desc;
unsigned int tmp;
u32 gfx_addr;
- memset(&desc, 0, sizeof(desc));
+ desc = __get_context_desc(client);
- desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
- desc.context_id = client->ctx_index;
- desc.priority = client->priority;
- desc.db_id = client->doorbell_id;
+ desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+ desc->context_id = client->ctx_index;
+ desc->priority = client->priority;
+ desc->db_id = client->doorbell_id;
for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
struct intel_context *ce = &ctx->engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
- struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+ struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
/* TODO: We have a design issue to be solved here. Only when we
* receive the first batch, we know which engine is used by the
@@ -317,50 +311,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
- desc.engines_used |= (1 << guc_engine_id);
+ desc->engines_used |= (1 << guc_engine_id);
}
DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
- client->engines, desc.engines_used);
- WARN_ON(desc.engines_used == 0);
+ client->engines, desc->engines_used);
+ WARN_ON(desc->engines_used == 0);
/*
* The doorbell, process descriptor, and workqueue are all parts
* of the client object, which the GuC will reference via the GGTT
*/
gfx_addr = guc_ggtt_offset(client->vma);
- desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+ desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+ client->doorbell_offset;
+ desc->db_trigger_cpu = (uintptr_t)client->vaddr +
client->doorbell_offset;
- desc.db_trigger_cpu =
- (uintptr_t)client->vaddr + client->doorbell_offset;
- desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
- desc.process_desc = gfx_addr + client->proc_desc_offset;
- desc.wq_addr = gfx_addr + client->wq_offset;
- desc.wq_size = client->wq_size;
+ desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+ desc->process_desc = gfx_addr + client->proc_desc_offset;
+ desc->wq_addr = gfx_addr + client->wq_offset;
+ desc->wq_size = client->wq_size;
/*
* XXX: Take LRCs from an existing context if this is not an
* IsKMDCreatedContext client
*/
- desc.desc_private = (uintptr_t)client;
-
- /* Pool context is pinned already */
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc->desc_private = (uintptr_t)client;
}
static void guc_ctx_desc_fini(struct intel_guc *guc,
struct i915_guc_client *client)
{
- struct guc_context_desc desc;
- struct sg_table *sg;
-
- memset(&desc, 0, sizeof(desc));
+ struct guc_context_desc *desc;
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc = __get_context_desc(client);
+ memset(desc, 0, sizeof(*desc));
}
/**
@@ -913,6 +898,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
const size_t gemsize = round_up(poolsize, PAGE_SIZE);
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
+ void *vaddr;
if (!HAS_GUC_SCHED(dev_priv))
return 0;
@@ -924,14 +910,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
if (!i915.enable_guc_submission)
return 0; /* not enabled */
- if (guc->ctx_pool_vma)
+ if (guc->ctx_pool)
return 0; /* already allocated */
vma = intel_guc_allocate_vma(guc, gemsize);
if (IS_ERR(vma))
return PTR_ERR(vma);
- guc->ctx_pool_vma = vma;
+ guc->ctx_pool = vma;
+
+ vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+ if (IS_ERR(vaddr))
+ goto err;
+
+ guc->ctx_pool_vaddr = vaddr;
+
ida_init(&guc->ctx_ids);
intel_guc_log_create(guc);
guc_addon_create(guc);
@@ -1023,9 +1016,12 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
i915_vma_unpin_and_release(&guc->ads_vma);
i915_vma_unpin_and_release(&guc->log.vma);
- if (guc->ctx_pool_vma)
+ if (guc->ctx_pool_vaddr) {
ida_destroy(&guc->ctx_ids);
- i915_vma_unpin_and_release(&guc->ctx_pool_vma);
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+ }
+
+ i915_vma_unpin_and_release(&guc->ctx_pool);
}
/**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9885f76..22f882d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -221,7 +221,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
/* If GuC submission is enabled, set up additional parameters here */
if (i915.enable_guc_submission) {
- u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
+ u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
pgs >>= PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c80f470..511b96b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -153,7 +153,8 @@ struct intel_guc {
bool interrupts_enabled;
struct i915_vma *ads_vma;
- struct i915_vma *ctx_pool_vma;
+ struct i915_vma *ctx_pool;
+ void *ctx_pool_vaddr;
struct ida ctx_ids;
struct i915_guc_client *execbuf_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] 15+ messages in thread* Re: [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-16 14:15 ` [PATCH v2] " Oscar Mateo
@ 2017-02-16 14:18 ` Oscar Mateo
2017-02-16 22:50 ` Chris Wilson
1 sibling, 0 replies; 15+ messages in thread
From: Oscar Mateo @ 2017-02-16 14:18 UTC (permalink / raw)
To: intel-gfx
Onion teardown in a separate patch (since it addresses a separate problem).
On 02/16/2017 06:15 AM, Oscar Mateo wrote:
> The GuC descriptor is big in size. If we use a local definition of
> guc_desc we have a chance to overflow stack, so avoid it.
>
> Also, Chris abhors scatterlists :)
>
> v2: Rebased, helper function to retrieve the context descriptor,
> s/ctx_pool_vma/ctx_pool/
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++++++++++++++----------------
> drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> drivers/gpu/drm/i915/intel_uc.h | 3 +-
> 3 files changed, 48 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 3ea2c71..f7d9897 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> }
>
> +static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
> +{
> + return client->guc->ctx_pool_vaddr +
> + sizeof(struct guc_context_desc) * client->ctx_index;
> +}
> +
> /*
> * Initialise, update, or clear doorbell data shared with the GuC
> *
> @@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
>
> static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
> {
> - struct sg_table *sg = client->guc->ctx_pool_vma->pages;
> - struct guc_context_desc desc;
> - size_t len;
> + struct guc_context_desc *desc;
>
> /* 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;
> + desc = __get_context_desc(client);
> + desc->db_id = new_id;
>
> return 0;
> }
> @@ -270,29 +266,27 @@ static void guc_proc_desc_init(struct intel_guc *guc,
> * data structures relating to this client (doorbell, process descriptor,
> * write queue, etc).
> */
> -
> static void guc_ctx_desc_init(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_engine_cs *engine;
> struct i915_gem_context *ctx = client->owner;
> - struct guc_context_desc desc;
> - struct sg_table *sg;
> + struct guc_context_desc *desc;
> unsigned int tmp;
> u32 gfx_addr;
>
> - memset(&desc, 0, sizeof(desc));
> + desc = __get_context_desc(client);
>
> - desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
> - desc.context_id = client->ctx_index;
> - desc.priority = client->priority;
> - desc.db_id = client->doorbell_id;
> + desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
> + desc->context_id = client->ctx_index;
> + desc->priority = client->priority;
> + desc->db_id = client->doorbell_id;
>
> for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
> struct intel_context *ce = &ctx->engine[engine->id];
> uint32_t guc_engine_id = engine->guc_id;
> - struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
> + struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
>
> /* TODO: We have a design issue to be solved here. Only when we
> * receive the first batch, we know which engine is used by the
> @@ -317,50 +311,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
> lrc->ring_next_free_location = lrc->ring_begin;
> lrc->ring_current_tail_pointer_value = 0;
>
> - desc.engines_used |= (1 << guc_engine_id);
> + desc->engines_used |= (1 << guc_engine_id);
> }
>
> DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
> - client->engines, desc.engines_used);
> - WARN_ON(desc.engines_used == 0);
> + client->engines, desc->engines_used);
> + WARN_ON(desc->engines_used == 0);
>
> /*
> * The doorbell, process descriptor, and workqueue are all parts
> * of the client object, which the GuC will reference via the GGTT
> */
> gfx_addr = guc_ggtt_offset(client->vma);
> - desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
> + desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
> + client->doorbell_offset;
> + desc->db_trigger_cpu = (uintptr_t)client->vaddr +
> client->doorbell_offset;
> - desc.db_trigger_cpu =
> - (uintptr_t)client->vaddr + client->doorbell_offset;
> - desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
> - desc.process_desc = gfx_addr + client->proc_desc_offset;
> - desc.wq_addr = gfx_addr + client->wq_offset;
> - desc.wq_size = client->wq_size;
> + desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
> + desc->process_desc = gfx_addr + client->proc_desc_offset;
> + desc->wq_addr = gfx_addr + client->wq_offset;
> + desc->wq_size = client->wq_size;
>
> /*
> * XXX: Take LRCs from an existing context if this is not an
> * IsKMDCreatedContext client
> */
> - desc.desc_private = (uintptr_t)client;
> -
> - /* Pool context is pinned already */
> - sg = guc->ctx_pool_vma->pages;
> - sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> - sizeof(desc) * client->ctx_index);
> + desc->desc_private = (uintptr_t)client;
> }
>
> static void guc_ctx_desc_fini(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> - struct guc_context_desc desc;
> - struct sg_table *sg;
> -
> - memset(&desc, 0, sizeof(desc));
> + struct guc_context_desc *desc;
>
> - sg = guc->ctx_pool_vma->pages;
> - sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> - sizeof(desc) * client->ctx_index);
> + desc = __get_context_desc(client);
> + memset(desc, 0, sizeof(*desc));
> }
>
> /**
> @@ -913,6 +898,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> const size_t gemsize = round_up(poolsize, PAGE_SIZE);
> struct intel_guc *guc = &dev_priv->guc;
> struct i915_vma *vma;
> + void *vaddr;
>
> if (!HAS_GUC_SCHED(dev_priv))
> return 0;
> @@ -924,14 +910,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> if (!i915.enable_guc_submission)
> return 0; /* not enabled */
>
> - if (guc->ctx_pool_vma)
> + if (guc->ctx_pool)
> return 0; /* already allocated */
>
> vma = intel_guc_allocate_vma(guc, gemsize);
> if (IS_ERR(vma))
> return PTR_ERR(vma);
>
> - guc->ctx_pool_vma = vma;
> + guc->ctx_pool = vma;
> +
> + vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> + if (IS_ERR(vaddr))
> + goto err;
> +
> + guc->ctx_pool_vaddr = vaddr;
> +
> ida_init(&guc->ctx_ids);
> intel_guc_log_create(guc);
> guc_addon_create(guc);
> @@ -1023,9 +1016,12 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> i915_vma_unpin_and_release(&guc->ads_vma);
> i915_vma_unpin_and_release(&guc->log.vma);
>
> - if (guc->ctx_pool_vma)
> + if (guc->ctx_pool_vaddr) {
> ida_destroy(&guc->ctx_ids);
> - i915_vma_unpin_and_release(&guc->ctx_pool_vma);
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> + }
> +
> + i915_vma_unpin_and_release(&guc->ctx_pool);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 9885f76..22f882d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -221,7 +221,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
>
> /* If GuC submission is enabled, set up additional parameters here */
> if (i915.enable_guc_submission) {
> - u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
> + u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
> u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
>
> pgs >>= PAGE_SHIFT;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index c80f470..511b96b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -153,7 +153,8 @@ struct intel_guc {
> bool interrupts_enabled;
>
> struct i915_vma *ads_vma;
> - struct i915_vma *ctx_pool_vma;
> + struct i915_vma *ctx_pool;
> + void *ctx_pool_vaddr;
> struct ida ctx_ids;
>
> struct i915_guc_client *execbuf_client;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-02-16 14:15 ` [PATCH v2] " Oscar Mateo
2017-02-16 14:18 ` Oscar Mateo
@ 2017-02-16 22:50 ` Chris Wilson
1 sibling, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-02-16 22:50 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Thu, Feb 16, 2017 at 06:15:05AM -0800, Oscar Mateo wrote:
> static void guc_ctx_desc_init(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_engine_cs *engine;
> struct i915_gem_context *ctx = client->owner;
> - struct guc_context_desc desc;
> - struct sg_table *sg;
> + struct guc_context_desc *desc;
> unsigned int tmp;
> u32 gfx_addr;
>
> - memset(&desc, 0, sizeof(desc));
> + desc = __get_context_desc(client);
Do you want to make the assumption that these are zeroed-on-create
objects? We could switch to using non-swappable (internal) objects that
are not cleared on create. i.e.
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4a752f2b6e24..4128e8937b45 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -650,7 +650,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
struct i915_vma *vma;
int ret;
- obj = i915_gem_object_create(dev_priv, size);
+ obj = i915_gem_object_create_internal(dev_priv, size);
if (IS_ERR(obj))
return ERR_CAST(obj);
Or do we write the entire desc? It doesn't look like we do, but do we do
enough?
Other than that potential booby trap for later,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/guc: Dynamically alloc GuC descriptor (rev2)
2017-02-02 15:27 [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor Oscar Mateo
2017-02-02 23:52 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-02-03 7:33 ` [PATCH] " Chris Wilson
@ 2017-02-09 18:52 ` Patchwork
2 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-02-09 18:52 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: Dynamically alloc GuC descriptor (rev2)
URL : https://patchwork.freedesktop.org/series/19022/
State : success
== Summary ==
Series 19022v2 drm/i915/guc: Dynamically alloc GuC descriptor
https://patchwork.freedesktop.org/api/1.0/series/19022/revisions/2/mbox/
fi-bdw-5557u total:252 pass:238 dwarn:0 dfail:0 fail:0 skip:14
fi-bsw-n3050 total:252 pass:213 dwarn:0 dfail:0 fail:0 skip:39
fi-bxt-j4205 total:252 pass:230 dwarn:0 dfail:0 fail:0 skip:22
fi-bxt-t5700 total:83 pass:70 dwarn:0 dfail:0 fail:0 skip:12
fi-byt-j1900 total:252 pass:225 dwarn:0 dfail:0 fail:0 skip:27
fi-byt-n2820 total:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31
fi-hsw-4770 total:252 pass:233 dwarn:0 dfail:0 fail:0 skip:19
fi-hsw-4770r total:252 pass:233 dwarn:0 dfail:0 fail:0 skip:19
fi-ilk-650 total:252 pass:199 dwarn:0 dfail:0 fail:0 skip:53
fi-ivb-3520m total:252 pass:231 dwarn:0 dfail:0 fail:0 skip:21
fi-ivb-3770 total:252 pass:231 dwarn:0 dfail:0 fail:0 skip:21
fi-kbl-7500u total:252 pass:229 dwarn:0 dfail:0 fail:2 skip:21
fi-skl-6260u total:252 pass:239 dwarn:0 dfail:0 fail:0 skip:13
fi-skl-6700hq total:252 pass:232 dwarn:0 dfail:0 fail:0 skip:20
fi-skl-6700k total:252 pass:227 dwarn:4 dfail:0 fail:0 skip:21
fi-skl-6770hq total:252 pass:239 dwarn:0 dfail:0 fail:0 skip:13
fi-snb-2520m total:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31
fi-snb-2600 total:252 pass:220 dwarn:0 dfail:0 fail:0 skip:32
8ca0c2e06c85344b8fee2bd5c48d6f3571fc870a drm-tip: 2017y-02m-09d-13h-35m-52s UTC integration manifest
57a62f6 drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3757/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-02-16 22:51 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-02 15:27 [PATCH] drm/i915/guc: Dynamically alloc GuC descriptor Oscar Mateo
2017-02-02 23:52 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-02-03 7:33 ` [PATCH] " Chris Wilson
2017-02-07 8:55 ` Oscar Mateo
2017-02-07 17:25 ` Chris Wilson
2017-02-07 9:37 ` Oscar Mateo
2017-02-07 20:49 ` Chris Wilson
2017-02-09 10:23 ` [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
2017-02-09 21:01 ` Chris Wilson
2017-02-13 16:05 ` Oscar Mateo
2017-02-15 12:37 ` Joonas Lahtinen
2017-02-16 14:15 ` [PATCH v2] " Oscar Mateo
2017-02-16 14:18 ` Oscar Mateo
2017-02-16 22:50 ` Chris Wilson
2017-02-09 18:52 ` ✓ Fi.CI.BAT: success for drm/i915/guc: Dynamically alloc GuC descriptor (rev2) 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.