[Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

[Michal Wajdeczko]

On Tue, 27 Mar 2018 22:03:23 +0200, Michel Thierry  
<michel.thierry at intel.com> wrote:

> On 3/27/2018 11:25 AM, Daniele Ceraolo Spurio wrote:
>>   On 26/03/18 12:48, Michal Wajdeczko wrote:
>>> When running on platform with CTB based GuC communication enabled,
>>> GuC to Host event data will be delivered as CT request message.
>>> However, content of the data[1] of this CT message follows format
>>> of the scratch register used in MMIO based communication, so some
>>> code reuse is still possible.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Reviewed-by: Michel Thierry <michel.thierry at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_guc.c    | 5 +++++
>>>   drivers/gpu/drm/i915/intel_guc.h    | 1 +
>>>   drivers/gpu/drm/i915/intel_guc_ct.c | 9 +++++++++
>>>   3 files changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>>> b/drivers/gpu/drm/i915/intel_guc.c
>>> index 118db81..b6d2778 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>>> @@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct  
>>> intel_guc *guc)
>>>       I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
>>>       spin_unlock(&guc->irq_lock);
>>> +    intel_guc_to_host_process_recv_msg(guc, msg);
>>> +}
>>> +
>>> +void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32  
>>> msg)
>>> +{
>>>       if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
>>>              INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
>>>           intel_guc_log_handle_flush_event(&guc->log);
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
>>> b/drivers/gpu/drm/i915/intel_guc.h
>>> index 6dc109a..f1265e1 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>> @@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
>>> const u32 *action, u32 len,
>>>   void intel_guc_to_host_event_handler(struct intel_guc *guc);
>>>   void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
>>>   void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
>>> +void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32  
>>> msg);
>>>   int intel_guc_sample_forcewake(struct intel_guc *guc);
>>>   int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
>>>   int intel_guc_suspend(struct intel_guc *guc);
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c  
>>> b/drivers/gpu/drm/i915/intel_guc_ct.c
>>> index aa810ad..e837084 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
>>> @@ -694,8 +694,17 @@ static int ct_handle_response(struct intel_guc_ct  
>>> *ct, const u32 *msg)
>>>   static void ct_process_request(struct intel_guc_ct *ct,
>>>                      u32 action, u32 len, const u32 *payload)
>>>   {
>>> +    struct intel_guc *guc = ct_to_guc(ct);
>>> +
>>>       switch (action) {
>>> +    case INTEL_GUC_ACTION_DEFAULT:
>>> +        if (unlikely(len < 1))
>>> +            goto fail_unexpected;
>>  Don't we need to remove the non-enabled messages here? i.e. like we do  
>> for the mmio receive:
>>       msg = *payload & guc->msg_enabled_mask;
>>  otherwise I think we'll try to process log interrupts even if the  
>> relay is not enabled.
>
> In Daniele's specific example, I still think guc_log_enable_flush_events  
> should have been called (and don't do anything inside  
> handle_flush_event, the goal was to not serve it).
>
> But it's true that we should filter any unexpected incoming request (&  
> guc->msg_enabled_mask), and any new user should be calling  
> intel_guc_enable_msg during its setup.

hmm, my understanding was that purpose of msg_enabled_mask was to:

	 * Sample the log buffer flush related bits & clear them out now
	 * itself from the message identity register to minimize the
	 * probability of losing a flush interrupt, when there are back
	 * to back flush interrupts.

and is not applicable to messages sent over CTB, as we can't miss msg.

if you still believe that filtering is required, it should not be done
here, as purpose of this function is to verify message completeness and
call actual handler, but in intel_guc_to_host_process_recv_msg(), where
format of received data is known.

/m

>
> Thanks,
>>  Daniele
>>
>>> +        intel_guc_to_host_process_recv_msg(guc, *payload);
>>> +        break;
>>> +
>>>       default:
>>> +fail_unexpected:
>>>           DRM_ERROR("CT: unexpected request %x %*phn\n",
>>>                 action, 4 * len, payload);
>>>           break;