[Intel-gfx] [PATCH v9 5/6] drm/i915/mtl: Add function to send command to GSC CS

[Kandpal, Suraj]

> From: Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>
> Sent: Wednesday, February 1, 2023 2:18 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Shankar, Uma <uma.shankar at intel.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal at intel.com>; Ceraolo Spurio, Daniele
> <daniele.ceraolospurio at intel.com>; Gupta, Anshuman
> <anshuman.gupta at intel.com>
> Subject: Re: [PATCH v9 5/6] drm/i915/mtl: Add function to send command to
> GSC CS
> 
> Conditional Rb with below fix in intel_hdcp_gsc_free_message:
> 
> Reviewed-by: Alan Previn <alan.previn.teres.alexis at intel.com>
> 
> On Tue, 2023-01-31 at 12:03 +0530, Kandpal, Suraj wrote:
> > Add function that takes care of sending command to gsc cs. We start of
> > with allocation of memory for our command intel_hdcp_gsc_message that
> > contains gsc cs memory header as directed in specs followed by the
> > actual payload hdcp message that we want to send.
> > Spec states that we need to poll pending bit of response header around
> > 20 times each try being 50ms apart hence adding that to current
> > gsc_msg_send function Also we use the same function to take care of
> > both sending and receiving hence no separate function to get the
> > response.
> >
> >
> alan:snip..
> 
> > +/*This function helps allocate memory for the command that we will
> > +send to gsc cs */ int intel_hdcp_gsc_hdcp2_init(struct
> > +drm_i915_private *i915) {
> > +
> alan: okay i see this is now being called from intel_hdcp_gsc_init
> 
> > +void intel_hdcp_gsc_free_message(struct drm_i915_private *i915) {
> > +       struct intel_hdcp_gsc_message *hdcp_message =
> > +
> > +i915->display.hdcp.hdcp_message;
> > +       if (hdcp_message->vma)
> > +               i915_vma_unpin(fetch_and_zero(&hdcp_message->vma));
> alan: i believe you don't need this extra i915_vma_unpin. Also, take note you
> have a bug above ...
> first code does a "fetch_and_zero" and right after zero-ing out its used to call
> i915_vma_unpin_and_release on the line below. So.. "if (hdcp_message-
> >vma) -> i915_vma_unpin_and_release"

I see we can just skip this part and directly do an i915_vma_unpin_and_release

Regards,
Suraj Kandpal
> > +
> > +       i915_vma_unpin_and_release(&hdcp_message->vma,
> > +I915_VMA_RELEASE_MAP);
> > +       kfree(hdcp_message);
> > +}
> > +
> >
> 
> alan:snip..
> > --
> > 2.25.1
> >