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

[Zhang, Xiaolin]

On 03/29/2019 11:40 PM, Chris Wilson wrote:
> 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.
sure, I will remove this next version.
>
>>         }
>>  
>>         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.
indeed, I should do. thanks your great point.
> I'd push for this to live in intel_pv_submission.c or
> intel_vgpu_submission.c
this will create new file and make a change  in Makefile. if this kind
of change is OK,
I will create one to use intel_pv_submission.c name.
>> @@ -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
>
Yep, will move it to other location.