Re: [PATCH] drm/i915: Inline engine->init_context into its caller

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 30/07/2019 10:34, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-29 15:49:29)
> > On 29/07/2019 12:37, Chris Wilson wrote:
> > > We only use the init_context vfunc once while recording the default
> > > context state, and we use the same sequence in each backend (eliding
> > > steps that do not apply). Remove the vfunc for simplicity and
> > > de-duplication.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/gt/intel_engine_types.h |  1 -
> > >    drivers/gpu/drm/i915/gt/intel_lrc.c          | 21 --------------------
> > >    drivers/gpu/drm/i915/gt/intel_mocs.c         |  5 ++++-
> > >    drivers/gpu/drm/i915/gt/intel_mocs.h         |  3 ++-
> > >    drivers/gpu/drm/i915/gt/intel_renderstate.c  |  2 +-
> > >    drivers/gpu/drm/i915/gt/intel_ringbuffer.c   | 18 -----------------
> > >    drivers/gpu/drm/i915/i915_gem.c              | 21 +++++++++++++++++---
> > >    7 files changed, 25 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > index 8be63019d707..da61dd329210 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -383,7 +383,6 @@ struct intel_engine_cs {
> > >        const struct intel_context_ops *cops;
> > >    
> > >        int             (*request_alloc)(struct i915_request *rq);
> > > -     int             (*init_context)(struct i915_request *rq);
> > >    
> > >        int             (*emit_flush)(struct i915_request *request, u32 mode);
> > >    #define EMIT_INVALIDATE     BIT(0)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > index 884dfc1cb033..4d7c4d0dbf75 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > @@ -141,7 +141,6 @@
> > >    #include "intel_gt.h"
> > >    #include "intel_lrc_reg.h"
> > >    #include "intel_mocs.h"
> > > -#include "intel_renderstate.h"
> > >    #include "intel_reset.h"
> > >    #include "intel_workarounds.h"
> > >    
> > > @@ -2727,25 +2726,6 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
> > >        return gen8_emit_wa_tail(request, cs);
> > >    }
> > >    
> > > -static int gen8_init_rcs_context(struct i915_request *rq)
> > > -{
> > > -     int ret;
> > > -
> > > -     ret = intel_engine_emit_ctx_wa(rq);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     ret = intel_rcs_context_init_mocs(rq);
> > > -     /*
> > > -      * Failing to program the MOCS is non-fatal.The system will not
> > > -      * run at peak performance. So generate an error and carry on.
> > > -      */
> > > -     if (ret)
> > > -             DRM_ERROR("MOCS failed to program: expect performance issues.\n");
> > > -
> > > -     return intel_renderstate_emit(rq);
> > > -}
> > > -
> > >    static void execlists_park(struct intel_engine_cs *engine)
> > >    {
> > >        del_timer_sync(&engine->execlists.timer);
> > > @@ -2853,7 +2833,6 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
> > >        logical_ring_default_irqs(engine);
> > >    
> > >        if (engine->class == RENDER_CLASS) {
> > > -             engine->init_context = gen8_init_rcs_context;
> > >                engine->emit_flush = gen8_emit_flush_render;
> > >                engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb_rcs;
> > >        }
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > index 290a5e9b90b9..e082b25d2db1 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > > @@ -568,11 +568,14 @@ void intel_mocs_init_l3cc_table(struct intel_gt *gt)
> > >     *
> > >     * Return: 0 on success, otherwise the error status.
> > >     */
> > > -int intel_rcs_context_init_mocs(struct i915_request *rq)
> > > +int intel_mocs_emit(struct i915_request *rq)
> > >    {
> > >        struct drm_i915_mocs_table t;
> > >        int ret;
> > >    
> > > +     if (rq->engine->class != RENDER_CLASS)
> > > +             return 0;
> > > +
> > >        if (get_mocs_settings(rq->engine->gt, &t)) {
> > >                /* Program the RCS control registers */
> > >                ret = emit_mocs_control_table(rq, &t);
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.h b/drivers/gpu/drm/i915/gt/intel_mocs.h
> > > index 8b9813e6f9ac..a334db2d6d6b 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_mocs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.h
> > > @@ -54,8 +54,9 @@ struct i915_request;
> > >    struct intel_engine_cs;
> > >    struct intel_gt;
> > >    
> > > -int intel_rcs_context_init_mocs(struct i915_request *rq);
> > >    void intel_mocs_init_l3cc_table(struct intel_gt *gt);
> > >    void intel_mocs_init_engine(struct intel_engine_cs *engine);
> > >    
> > > +int intel_mocs_emit(struct i915_request *rq);
> > > +
> > >    #endif
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_renderstate.c b/drivers/gpu/drm/i915/gt/intel_renderstate.c
> > > index 06a8dc40b19f..be37d4501c67 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_renderstate.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_renderstate.c
> > > @@ -41,7 +41,7 @@ struct intel_renderstate {
> > >    static const struct intel_renderstate_rodata *
> > >    render_state_get_rodata(const struct intel_engine_cs *engine)
> > >    {
> > > -     if (engine->id != RCS0)
> > > +     if (engine->class != RENDER_CLASS)
> > >                return NULL;
> > >    
> > >        switch (INTEL_GEN(engine->i915)) {
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > > index 1de19dac4a14..5c7f2fdc5ec3 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > > @@ -37,7 +37,6 @@
> > >    #include "i915_trace.h"
> > >    #include "intel_context.h"
> > >    #include "intel_gt.h"
> > > -#include "intel_renderstate.h"
> > >    #include "intel_reset.h"
> > >    #include "intel_workarounds.h"
> > >    
> > > @@ -849,21 +848,6 @@ static void reset_finish(struct intel_engine_cs *engine)
> > >    {
> > >    }
> > >    
> > > -static int intel_rcs_ctx_init(struct i915_request *rq)
> > > -{
> > > -     int ret;
> > > -
> > > -     ret = intel_engine_emit_ctx_wa(rq);
> > > -     if (ret != 0)
> > > -             return ret;
> > > -
> > > -     ret = intel_renderstate_emit(rq);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >    static int rcs_resume(struct intel_engine_cs *engine)
> > >    {
> > >        struct drm_i915_private *dev_priv = engine->i915;
> > > @@ -2227,11 +2211,9 @@ static void setup_rcs(struct intel_engine_cs *engine)
> > >        engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> > >    
> > >        if (INTEL_GEN(i915) >= 7) {
> > > -             engine->init_context = intel_rcs_ctx_init;
> > >                engine->emit_flush = gen7_render_ring_flush;
> > >                engine->emit_fini_breadcrumb = gen7_rcs_emit_breadcrumb;
> > >        } else if (IS_GEN(i915, 6)) {
> > > -             engine->init_context = intel_rcs_ctx_init;
> > >                engine->emit_flush = gen6_render_ring_flush;
> > >                engine->emit_fini_breadcrumb = gen6_rcs_emit_breadcrumb;
> > >        } else if (IS_GEN(i915, 5)) {
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 01dd0d1d9bf6..65863e955f40 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -50,6 +50,7 @@
> > >    #include "gt/intel_gt_pm.h"
> > >    #include "gt/intel_mocs.h"
> > >    #include "gt/intel_reset.h"
> > > +#include "gt/intel_renderstate.h"
> > >    #include "gt/intel_workarounds.h"
> > >    
> > >    #include "i915_drv.h"
> > > @@ -1294,10 +1295,24 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > >                        goto err_active;
> > >                }
> > >    
> > > -             err = 0;
> > > -             if (rq->engine->init_context)
> > > -                     err = rq->engine->init_context(rq);
> > > +             err = intel_engine_emit_ctx_wa(rq);
> > > +             if (err)
> > > +                     goto err_rq;
> > > +
> > > +             /*
> > > +              * Failing to program the MOCS is non-fatal.The system will not
> > > +              * run at peak performance. So warn the user and carry on.
> > > +              */
> > > +             err = intel_mocs_emit(rq);
> >
> > I think platforms without MOCS will not be happy unless you also change
> > the MOCS code.
>
> Older platforms fail gracefully (silently with err=0 as get_mocs_settings() returns false).
> New platforms without MOCS descriptions issue a warning (as currently).

True, I've missed the fact WARN_ON on the final else branch is 
conditional on gen >= 9.

>   
> > > +             if (err)
> > > +                     dev_notice(i915->drm.dev,
> > > +                                "Failed to program MOCS registers; expect performance issues.\n");
> > > +
> > > +             err = intel_renderstate_emit(rq);
> > > +             if (err)
> > > +                     goto err_rq;
> >
> > Would it be prettier to move all the tree into
> > intel_engine_cs.c/intel_engine_context_init() or something? Here it
> > seems a bit too much into details.
>
> Indeed, it's not staying here. But that is tied up in other in-flight
> patches.

Okay.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx