Hi Daniele - thanks for reviewing this - i will fix all of code in accordance to the review
comments you provided with some exceptions / alternatives:
On Fri, 2023-03-03 at 17:07 -0800, Ceraolo Spurio, Daniele wrote:
>
> On 2/27/2023 6:21 PM, Alan Previn wrote:
> > Add GSC engine based method for sending PXP firmware packets
> > to the GSC firmware for MTL (and future) products.
> >
> > Use the newly added helpers to populate the GSC-CS memory
> > header and send the message packet to the FW by dispatching
> > the GSC_HECI_CMD_PKT instruction on the GSC engine.
> > +gsccs_send_message(struct intel_pxp *pxp,
> > + void *msg_in, size_t msg_in_size,
> > + void *msg_out, size_t msg_out_size_max,
> > + size_t *msg_out_len,
> > + u64 *gsc_msg_handle_retry)
> > +{
> > + struct intel_gt *gt = pxp->ctrl_gt;
> > + struct drm_i915_private *i915 = gt->i915;
> > + struct gsccs_session_resources *exec = &pxp->gsccs_res;
>
> in the alloc/free functions you called this object *strm_res; IMO better
> to use a consistent naming so it is clear they're the same object
>
alan: agred - actually i think i will go with "exec_res" across the board instead.
alan:snip
> > + max_msg_size = PXP43_MAX_HECI_IN_SIZE - sizeof(*header);
>
> Using the same max_msg_size for both in and out only works if
> PXP43_MAX_HECI_IN_SIZE == PXP43_MAX_HECI_OUT_SIZE. This is true now, but
> I'd add a:
> BUILD_BUG_ON(PXP43_MAX_HECI_IN_SIZE != PXP43_MAX_HECI_OUT_SIZE);
> just to be safe. Potentially also a:
> GEM_BUG_ON(exec->pkt_vma->size < (PXP43_MAX_HECI_IN_SIZE +
> PXP43_MAX_HECI_OUT_SIZE));
> After checking that exec->pkt_vma exists.
>
alan: actually an even simpler alternative would be to just use #define
PXP43_MAX_HECI_INOUT_SIZE - a single definition that will make both
the code and logic easier - it is after reflecting the HW spec too where
the total sizes would be 2xPXP43_MAX_HECI_INOUT_SIZE
alan:snip
>
>
> nit: can't you just use if (!msg_in && !msg_out) instead of a local var?
> not a blocker.
alan:sure
alan:snip
> > + /* Response validity marker, status and busyness */
> > + if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) {
>
> AFAICS you're not clearing the reply header when you re-send the same
> packets after the pending bit, so this marker might be stale data. Same
> for the other fields below.
Good catch - I can clear it before i do the submission - and i assume
you agree that clearing the validity marker alone (in the reply offset)
is sufficient here to strenghten this check.
alan:snip
> >
> > + if (header->flags & GSC_HECI_FLAG_MSG_PENDING) {
> > + drm_dbg(&i915->drm, "gsc PXP reply is busy\n");
> > + /*
> > + * When the GSC firmware replies with pending bit, it means that the requested
> > + * operation has begun but the completion is pending and the caller needs
> > + * to re-request with the gsc_message_handle that was returned by the firmware.
> > + * until the pending bit is turned off.
> > + */
> > + *gsc_msg_handle_retry = header->gsc_message_handle;
>
> Non blocking question: would it be worth it to copy the value to the
> header_in directly, instead of returning the value to the caller and
> copying it on resubmit? Just a thought, I see pro and cons with both
> approaches.
>
alan: Hmm - good idea - okay - let me think about this one... although i
prefer the control be on the caller's side.
alan:snip
> > + } else if (reply_size != msg_out_size_max) {
> > + drm_dbg(&i915->drm, "caller unexpected PXP reply size %u (%ld)\n",
> > + reply_size, msg_out_size_max);
> Are we expecting all caller to always pass the exact size? Not a
> complain, but if that's the case then maybe rename msg_out_size_max to
> msg_out_expected_size, so it's clearer. size_max sounds like any size
> smaller than it is allowed. My personal preference would be to leave
> this as a size_max and have the caller decide if the actual returned
> size matches the expectations (via msg_out_len)
>
No, i am allowing the user to provide buffers bigger than what the reply
In which case you are right- i can let the caller do the size checking.
and remove that dbg msg.
alan:snip
> > + /* all PXP sessions commands are treated as non-priveleged */
> typo priveleged
will fix.
> nit: maybe move gsccs_create_buffer after the cleanup/destruction
> functions? so we can group all the creation functions close together.
>
i was also think of moving functions around to group them but i want to
keep the init/fini right at the bottomg.
> > +static void
> > +gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp)
> > +{
> > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> > + int ret;
> > +
> > + ret = gsccs_send_message_retry_complete(pxp, NULL, 0, NULL, 0, NULL);
> > + if (ret)
> > + drm_dbg(&i915->drm, "Failed to send gsccs msg host-session-cleanup: ret=[%d]\n",
> > + ret);
> > +}
> > +
> > static void
> > gsccs_destroy_execution_resource(struct intel_pxp *pxp)
> > {
> > struct gsccs_session_resources *strm_res = &pxp->gsccs_res;
> >
> > + if (strm_res->host_session_handle)
> > + gsccs_cleanup_fw_host_session_handle(pxp);
> > if (strm_res->ce)
> > intel_context_put(strm_res->ce);
> > + if (strm_res->bb_vma)
> > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP);
> > + if (strm_res->pkt_vma)
> > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP);
> >
> > memset(strm_res, 0, sizeof(*strm_res));
> > }
> > @@ -30,6 +237,7 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
> > struct gsccs_session_resources *strm_res = &pxp->gsccs_res;
> > struct intel_engine_cs *engine = gt->engine[GSC0];
> > struct intel_context *ce;
> > + int err = 0;
> >
> > /*
> > * First, ensure the GSC engine is present.
> > @@ -38,16 +246,43 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
> > if (!engine)
> > return -ENODEV;
> >
> > + /*
> > + * Now, allocate, pin and map two objects, one for the heci message packet
> > + * and another for the batch buffer we submit into GSC engine (that includes the packet).
> > + * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem.
> > + */
> > + err = gsccs_create_buffer(pxp->ctrl_gt, "Heci Packet",
> > + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE,
> > + &strm_res->pkt_vma, &strm_res->pkt_vaddr);
> > + if (err)
> > + return err;
> > +
> > + err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE,
> > + &strm_res->bb_vma, &strm_res->bb_vaddr);
> > + if (err)
> > + goto free_pkt;
> > +
> > /* Finally, create an intel_context to be used during the submission */
> > ce = intel_context_create(engine);
> > if (IS_ERR(ce)) {
> > drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n");
> > - return PTR_ERR(ce);
> > + err = PTR_ERR(ce);
> > + goto free_batch;
> > }
> > -
> > strm_res->ce = ce;
> >
> > + /* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */
> > + get_random_bytes(&strm_res->host_session_handle, sizeof(strm_res->host_session_handle));
>
> This does not guarantee that each host session handle is unique
> (although getting the same u64 twice is going to be extremely extremely
> unlikely). Not sure if it is a problem.
>
yes, you are correct.. i am hoping this is sufficioent.
> > +
> > return 0;
> > +
> > +free_pkt:
> > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP);
> > +free_batch:
> > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP);
>
> those gotos are the wrong way around, the pkt is allocated first and
> therefore it should be freed second
alan: yeah - my mistake - will fix it - thanks.
