Quoting Tvrtko Ursulin (2018-02-01 09:42:02)
>
> On 01/02/2018 08:36, Chris Wilson wrote:
> > Pull the physical status page cleanup into a common
> > cleanup_status_page() for caller simplicity.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_engine_cs.c | 23 +++++++----------------
> > 1 file changed, 7 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 7eebfbb95e89..a3ad6925abaa 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -488,21 +488,15 @@ static void intel_engine_cleanup_scratch(struct intel_engine_cs *engine)
> > i915_vma_unpin_and_release(&engine->scratch);
> > }
> >
> > -static void cleanup_phys_status_page(struct intel_engine_cs *engine)
> > -{
> > - struct drm_i915_private *dev_priv = engine->i915;
> > -
> > - if (!dev_priv->status_page_dmah)
> > - return;
> > -
> > - drm_pci_free(&dev_priv->drm, dev_priv->status_page_dmah);
> > - engine->status_page.page_addr = NULL;
> > -}
> > -
> > static void cleanup_status_page(struct intel_engine_cs *engine)
> > {
> > - struct i915_vma *vma;
> > struct drm_i915_gem_object *obj;
> > + struct drm_dma_handle *dmah;
> > + struct i915_vma *vma;
> > +
> > + dmah = fetch_and_zero(&engine->i915->status_page_dmah);
> > + if (dmah)
> > + drm_pci_free(&engine->i915->drm, dmah);
> >
> > vma = fetch_and_zero(&engine->status_page.vma);
> > if (!vma)
> > @@ -674,10 +668,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> > {
> > intel_engine_cleanup_scratch(engine);
> >
> > - if (HWS_NEEDS_PHYSICAL(engine->i915))
> > - cleanup_phys_status_page(engine);
> > - else
> > - cleanup_status_page(engine);
> > + cleanup_status_page(engine);
> >
> > intel_engine_fini_breadcrumbs(engine);
> > intel_engine_cleanup_cmd_parser(engine);
> >
>
> Immediate question arises why not the same treatment for
> init_status_page paths, but on a deeper look, why not move the phys
> paths out of common and into intel_ringbuffer.c?
My feeling is that it's part of the engine setup, especially as we keep
a dedicated HWS for execlist. Overall I'm not sold on this, so if you
have something to make next year easier, or that can trim down
yesteryears, go for it.
I just happened to be looking at seeing if I could easily move the
init_workarounds around to skip the "rc0 setup X workarounds" spam.
Then decided that's better solved by poking Oscar into reviving his
workaround cleanup.
> Although I spotted on the same page when looking at it,
> intel_engine_init_common "Initializes @engine@ structure members shared
> between legacy and execlists..", and in it, "if
> (HAS_LOGICAL_RING_PREEMPTION(..".
>
> So I'd say in general we deviated, again, from the spirit of the earlier
> cleanups. :(
Yeah, the biggest mess at the moment is the multilayered engine cleanup.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
