[Intel-gfx] [PATCH v4 4/8] drm/i915: vgpu context submission pv optimization

[Chris Wilson]

Quoting Xiaolin Zhang (2019-03-29 13:32:40)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2f78829..28e8ee0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -37,6 +37,7 @@
>  #include "i915_drv.h"
>  #include "i915_trace.h"
>  #include "intel_drv.h"
> +#include "i915_vgpu.h"
>  
>  /**
>   * DOC: interrupt handling
> @@ -1470,6 +1471,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>         if (iir & GT_RENDER_USER_INTERRUPT) {
>                 intel_engine_breadcrumbs_irq(engine);
>                 tasklet |= USES_GUC_SUBMISSION(engine->i915);
> +               tasklet |= USES_PV_SUBMISSION(engine->i915);

We should move this to an engine->flag.

>         }
>  
>         if (tasklet)
> +static void vgpu_pv_set_default_submission(struct intel_engine_cs *engine)
> +{
> +       /*
> +        * We inherit a bunch of functions from execlists that we'd like
> +        * to keep using:
> +        *
> +        *    engine->submit_request = execlists_submit_request;
> +        *    engine->cancel_requests = execlists_cancel_requests;
> +        *    engine->schedule = execlists_schedule;
> +        *
> +        * But we need to override the actual submission backend in order
> +        * to talk to the GVT with PV notification message.
> +        */
> +       intel_execlists_set_default_submission(engine);
> +
> +       engine->execlists.tasklet.func = vgpu_pv_submission_tasklet;

You need to pin the breadcrumbs irq, or it will not fire on every
request.

I'd push for this to live in intel_pv_submission.c or
intel_vgpu_submission.c

> @@ -401,6 +551,12 @@ void intel_vgpu_config_pv_caps(struct drm_i915_private *dev_priv,
>                 ppgtt->vm.insert_entries = gen8_ppgtt_insert_4lvl_pv;
>                 ppgtt->vm.clear_range = gen8_ppgtt_clear_4lvl_pv;
>         }
> +
> +       if (cap == PV_SUBMISSION) {
> +               engine = (struct intel_engine_cs *)data;
> +               engine->set_default_submission = vgpu_pv_set_default_submission;
> +               engine->set_default_submission(engine);
> +       }
>  }

> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c1b9780..0a66714 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2352,6 +2352,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>                  */
>         }
>         engine->emit_bb_start = gen8_emit_bb_start;
> +
> +       if (intel_vgpu_active(engine->i915))
> +               intel_vgpu_config_pv_caps(engine->i915, PV_SUBMISSION, engine);

That pair is ugly. Should clean up the engine initialisation so that it
doesn't involve placing a chunk of code in a foreign class.
-Chris