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@xxxxxxxxx> > >>>> > >>>>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@xxxxxxxxx> > >>>>Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > >>>>Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx> > >>>>--- > >>>> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
