| Message ID | 20220610121205.29645-2-juhapekka.heikkila@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/display: Add smem fallback allocation for dpt | expand |
On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote: > > From: CQ Tang <cq.tang@intel.com> > > Display might allocate a smem object and call > i915_vma_pin_iomap(), the existing code will fail. > > This fix was suggested by Chris P Wilson, that we pin > the smem with i915_gem_object_pin_map_unlocked(). > > v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to > i915_gem_object_pin_map > > Signed-off-by: CQ Tang <cq.tang@intel.com> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Cc: Chris Wilson <chris.p.wilson@intel.com> > Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
On Fri, 10 Jun 2022 at 15:53, Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila > <juhapekka.heikkila@gmail.com> wrote: > > > > From: CQ Tang <cq.tang@intel.com> > > > > Display might allocate a smem object and call > > i915_vma_pin_iomap(), the existing code will fail. > > > > This fix was suggested by Chris P Wilson, that we pin > > the smem with i915_gem_object_pin_map_unlocked(). > > > > v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to > > i915_gem_object_pin_map > > > > Signed-off-by: CQ Tang <cq.tang@intel.com> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > Cc: Chris Wilson <chris.p.wilson@intel.com> > > Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> > Reviewed-by: Matthew Auld <matthew.auld@intel.com> Although maybe consider putting this as patch 1, and then reword the commit title/message to be more like "drm/i915: extend i915_vma_iomap()" or so, which then becomes a prep patch for supporting the dpt fallback to smem. Otherwise it looks like this patch is basically just fixing the first patch to not trigger the WARN_ON(), which seems iffy IMO. Each patch by itself should ideally be functional.
On 10.6.2022 20.43, Matthew Auld wrote: > On Fri, 10 Jun 2022 at 15:53, Matthew Auld > <matthew.william.auld@gmail.com> wrote: >> >> On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila >> <juhapekka.heikkila@gmail.com> wrote: >>> >>> From: CQ Tang <cq.tang@intel.com> >>> >>> Display might allocate a smem object and call >>> i915_vma_pin_iomap(), the existing code will fail. >>> >>> This fix was suggested by Chris P Wilson, that we pin >>> the smem with i915_gem_object_pin_map_unlocked(). >>> >>> v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to >>> i915_gem_object_pin_map >>> >>> Signed-off-by: CQ Tang <cq.tang@intel.com> >>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >>> Cc: Chris Wilson <chris.p.wilson@intel.com> >>> Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> >> Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > Although maybe consider putting this as patch 1, and then reword the > commit title/message to be more like "drm/i915: extend > i915_vma_iomap()" or so, which then becomes a prep patch for > supporting the dpt fallback to smem. Otherwise it looks like this > patch is basically just fixing the first patch to not trigger the > WARN_ON(), which seems iffy IMO. Each patch by itself should ideally > be functional. Probably easiest is to put patch 1 in as last, it's the final customer for these two other patches. This way if someone will end up doing bisecting there would be nothing interesting to see with these. I did finally get ci to look all green after last weeks outages. I'd guess these patches are ready to be pushed but I don't have commit rights to drm-tip. Are you able to help with these or I'll go bother Imre or Jani to get them into tip? /Juha-Pekka
On Mon, 20 Jun 2022 at 10:38, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote: > > On 10.6.2022 20.43, Matthew Auld wrote: > > On Fri, 10 Jun 2022 at 15:53, Matthew Auld > > <matthew.william.auld@gmail.com> wrote: > >> > >> On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila > >> <juhapekka.heikkila@gmail.com> wrote: > >>> > >>> From: CQ Tang <cq.tang@intel.com> > >>> > >>> Display might allocate a smem object and call > >>> i915_vma_pin_iomap(), the existing code will fail. > >>> > >>> This fix was suggested by Chris P Wilson, that we pin > >>> the smem with i915_gem_object_pin_map_unlocked(). > >>> > >>> v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to > >>> i915_gem_object_pin_map > >>> > >>> Signed-off-by: CQ Tang <cq.tang@intel.com> > >>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > >>> Cc: Chris Wilson <chris.p.wilson@intel.com> > >>> Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> > >> Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > > > Although maybe consider putting this as patch 1, and then reword the > > commit title/message to be more like "drm/i915: extend > > i915_vma_iomap()" or so, which then becomes a prep patch for > > supporting the dpt fallback to smem. Otherwise it looks like this > > patch is basically just fixing the first patch to not trigger the > > WARN_ON(), which seems iffy IMO. Each patch by itself should ideally > > be functional. > > Probably easiest is to put patch 1 in as last, it's the final customer > for these two other patches. This way if someone will end up doing > bisecting there would be nothing interesting to see with these. > > I did finally get ci to look all green after last weeks outages. I'd > guess these patches are ready to be pushed but I don't have commit > rights to drm-tip. Are you able to help with these or I'll go bother > Imre or Jani to get them into tip? Ok, if no objections I will go ahead and push this to drm-intel-gt-next, with the tweaked patch ordering. > > /Juha-Pekka
On 21.6.2022 13.53, Matthew Auld wrote: > On Mon, 20 Jun 2022 at 10:38, Juha-Pekka Heikkila > <juhapekka.heikkila@gmail.com> wrote: >> >> On 10.6.2022 20.43, Matthew Auld wrote: >>> On Fri, 10 Jun 2022 at 15:53, Matthew Auld >>> <matthew.william.auld@gmail.com> wrote: >>>> >>>> On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila >>>> <juhapekka.heikkila@gmail.com> wrote: >>>>> >>>>> From: CQ Tang <cq.tang@intel.com> >>>>> >>>>> Display might allocate a smem object and call >>>>> i915_vma_pin_iomap(), the existing code will fail. >>>>> >>>>> This fix was suggested by Chris P Wilson, that we pin >>>>> the smem with i915_gem_object_pin_map_unlocked(). >>>>> >>>>> v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to >>>>> i915_gem_object_pin_map >>>>> >>>>> Signed-off-by: CQ Tang <cq.tang@intel.com> >>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >>>>> Cc: Chris Wilson <chris.p.wilson@intel.com> >>>>> Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> >>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com> >>> >>> Although maybe consider putting this as patch 1, and then reword the >>> commit title/message to be more like "drm/i915: extend >>> i915_vma_iomap()" or so, which then becomes a prep patch for >>> supporting the dpt fallback to smem. Otherwise it looks like this >>> patch is basically just fixing the first patch to not trigger the >>> WARN_ON(), which seems iffy IMO. Each patch by itself should ideally >>> be functional. >> >> Probably easiest is to put patch 1 in as last, it's the final customer >> for these two other patches. This way if someone will end up doing >> bisecting there would be nothing interesting to see with these. >> >> I did finally get ci to look all green after last weeks outages. I'd >> guess these patches are ready to be pushed but I don't have commit >> rights to drm-tip. Are you able to help with these or I'll go bother >> Imre or Jani to get them into tip? > > Ok, if no objections I will go ahead and push this to > drm-intel-gt-next, with the tweaked patch ordering. No objections. I had this set yet on test run on Imre's wish on try-bot with forcing adlp (on bat) to use smem and results were all clean. /Juha-Pekka
On Tue, 21 Jun 2022 at 19:38, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote: > > On 21.6.2022 13.53, Matthew Auld wrote: > > On Mon, 20 Jun 2022 at 10:38, Juha-Pekka Heikkila > > <juhapekka.heikkila@gmail.com> wrote: > >> > >> On 10.6.2022 20.43, Matthew Auld wrote: > >>> On Fri, 10 Jun 2022 at 15:53, Matthew Auld > >>> <matthew.william.auld@gmail.com> wrote: > >>>> > >>>> On Fri, 10 Jun 2022 at 13:12, Juha-Pekka Heikkila > >>>> <juhapekka.heikkila@gmail.com> wrote: > >>>>> > >>>>> From: CQ Tang <cq.tang@intel.com> > >>>>> > >>>>> Display might allocate a smem object and call > >>>>> i915_vma_pin_iomap(), the existing code will fail. > >>>>> > >>>>> This fix was suggested by Chris P Wilson, that we pin > >>>>> the smem with i915_gem_object_pin_map_unlocked(). > >>>>> > >>>>> v2 (jheikkil): Change i915_gem_object_pin_map_unlocked to > >>>>> i915_gem_object_pin_map > >>>>> > >>>>> Signed-off-by: CQ Tang <cq.tang@intel.com> > >>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > >>>>> Cc: Chris Wilson <chris.p.wilson@intel.com> > >>>>> Cc: Jari Tahvanainen <jari.tahvanainen@intel.com> > >>>> Reviewed-by: Matthew Auld <matthew.auld@intel.com> > >>> > >>> Although maybe consider putting this as patch 1, and then reword the > >>> commit title/message to be more like "drm/i915: extend > >>> i915_vma_iomap()" or so, which then becomes a prep patch for > >>> supporting the dpt fallback to smem. Otherwise it looks like this > >>> patch is basically just fixing the first patch to not trigger the > >>> WARN_ON(), which seems iffy IMO. Each patch by itself should ideally > >>> be functional. > >> > >> Probably easiest is to put patch 1 in as last, it's the final customer > >> for these two other patches. This way if someone will end up doing > >> bisecting there would be nothing interesting to see with these. > >> > >> I did finally get ci to look all green after last weeks outages. I'd > >> guess these patches are ready to be pushed but I don't have commit > >> rights to drm-tip. Are you able to help with these or I'll go bother > >> Imre or Jani to get them into tip? > > > > Ok, if no objections I will go ahead and push this to > > drm-intel-gt-next, with the tweaked patch ordering. > > No objections. I had this set yet on test run on Imre's wish on try-bot > with forcing adlp (on bat) to use smem and results were all clean. And pushed. Thanks for the patches. > > /Juha-Pekka
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 0bffb70b3c5f..7753337ff9fb 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -551,13 +551,6 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) if (WARN_ON_ONCE(vma->obj->flags & I915_BO_ALLOC_GPU_ONLY)) return IOMEM_ERR_PTR(-EINVAL); - if (!i915_gem_object_is_lmem(vma->obj)) { - if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) { - err = -ENODEV; - goto err; - } - } - GEM_BUG_ON(!i915_vma_is_ggtt(vma)); GEM_BUG_ON(!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)); GEM_BUG_ON(i915_vma_verify_bind_complete(vma)); @@ -573,17 +566,30 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) if (i915_gem_object_is_lmem(vma->obj)) ptr = i915_gem_object_lmem_io_map(vma->obj, 0, vma->obj->base.size); - else + else if (i915_vma_is_map_and_fenceable(vma)) ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap, vma->node.start, vma->node.size); + else { + ptr = (void __iomem *) + i915_gem_object_pin_map(vma->obj, I915_MAP_WC); + if (IS_ERR(ptr)) { + err = PTR_ERR(ptr); + goto err; + } + ptr = page_pack_bits(ptr, 1); + } + if (ptr == NULL) { err = -ENOMEM; goto err; } if (unlikely(cmpxchg(&vma->iomap, NULL, ptr))) { - io_mapping_unmap(ptr); + if (page_unmask_bits(ptr)) + __i915_gem_object_release_map(vma->obj); + else + io_mapping_unmap(ptr); ptr = vma->iomap; } } @@ -597,7 +603,7 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) i915_vma_set_ggtt_write(vma); /* NB Access through the GTT requires the device to be awake. */ - return ptr; + return page_mask_bits(ptr); err_unpin: __i915_vma_unpin(vma); @@ -615,6 +621,8 @@ void i915_vma_unpin_iomap(struct i915_vma *vma) { GEM_BUG_ON(vma->iomap == NULL); + /* XXX We keep the mapping until __i915_vma_unbind()/evict() */ + i915_vma_flush_writes(vma); i915_vma_unpin_fence(vma); @@ -1763,7 +1771,10 @@ static void __i915_vma_iounmap(struct i915_vma *vma) if (vma->iomap == NULL) return; - io_mapping_unmap(vma->iomap); + if (page_unmask_bits(vma->iomap)) + __i915_gem_object_release_map(vma->obj); + else + io_mapping_unmap(vma->iomap); vma->iomap = NULL; }