Re: [Intel-gfx] [PATCH v6 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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(&gt->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.