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

[Daniele Ceraolo Spurio]

On 27/03/18 14:05, Michal Wajdeczko wrote:
> 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
> 

The problem is that now we keep the interrupts enabled even if the log 
relay is not enabled. If we handle the interrupt anyway and queue the 
flush_work (from intel_guc_log_handle_flush_event), we end up hitting:

	if (WARN_ON(!intel_guc_log_relay_enabled(log)))
		goto out_unlock;

inside guc_read_update_log_buffer.

I don't think that the comment you added above referred to the specific 
usage of msg_enabled_mask but more to the fact that we clean the bits 
related to the messages we're going to handle.

Daniele

>>
>> 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;