| Message ID | 20180326194829.58836-11-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Reviewed-by: Michel Thierry <michel.thierry@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. 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; >
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@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Reviewed-by: Michel Thierry <michel.thierry@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. 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; >>
On Tue, 27 Mar 2018 22:03:23 +0200, Michel Thierry <michel.thierry@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@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> Reviewed-by: Michel Thierry <michel.thierry@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;
On 27/03/18 14:05, Michal Wajdeczko wrote: > On Tue, 27 Mar 2018 22:03:23 +0200, Michel Thierry > <michel.thierry@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@intel.com> >>>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>>> Reviewed-by: Michel Thierry <michel.thierry@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;
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; + 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;