[Intel-gfx] [PATCH] drm/i915: Inline engine->init_context into its caller
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jul 30 10:11:27 UTC 2019
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 at chris-wilson.co.uk>
>>> ---
>>> 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 at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list