Re: [PATCH 3/3] drm/i915/huc: improve documentation

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 10/9/19 2:44 PM, Daniele Ceraolo Spurio wrote:
> On 10/9/19 7:41 AM, Martin Peres wrote:
> > On 28/09/2019 00:42, Daniele Ceraolo Spurio wrote:
> > > Better explain the usage of the microcontroller and what i915 is
> > > responsible of. While at it, fix the documentation for the auth
> > > function, which doesn't do any pinning anymore.
> > >
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > ---
> > >   Documentation/gpu/i915.rst                | 10 ++++++++--
> > >   drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 19 +++++++++++++++----
> > >   drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 15 ---------------
> > >   3 files changed, 23 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > > index 357e9dfa7de1..bfb64337db66 100644
> > > --- a/Documentation/gpu/i915.rst
> > > +++ b/Documentation/gpu/i915.rst
> > > @@ -471,8 +471,14 @@ GuC-based command submission
> > >   HuC
> > >   ---
> > > -.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > > -   :doc: HuC Firmware
> > > +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > > +   :doc: HuC
> > > +.. kernel-doc:: drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > > +   :functions: intel_huc_auth
> > > +
> > > +HuC Firmware Layout
> > > +~~~~~~~~~~~~~~~~~~~
> > > +The HuC FW layout is the same as the GuC one, see `GuC Firmware 
> > > Layout`_
> > >   DMC
> > >   ---
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
> > > b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > > index d4625c97b4f9..6e10fe898c90 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > > @@ -9,6 +9,18 @@
> > >   #include "intel_huc.h"
> > >   #include "i915_drv.h"
> > > +/**
> > > + * DOC: HuC
> > > + *
> > > + * The HuC is a dedicated microcontroller for usage in media HEVC (High
> > > + * Efficiency Video Coding) operations. Userspace can directly use 
> > > the firmware
> > > + * capabilities by adding HuC specific commands to batch buffers.
> > > + * The kernel driver is only responsible for loading the HuC 
> > > firmware and
> > > + * triggering its security authentication, which is performed by the 
> > > GuC. For
> > > + * The GuC to correctly perform the authentication, the HuC binary 
> > > must be
> > > + * loaded before the GuC one.
> > > + */
> > > +
> > >   void intel_huc_init_early(struct intel_huc *huc)
> > >   {
> > >       struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
> > > @@ -118,10 +130,9 @@ void intel_huc_fini(struct intel_huc *huc)
> > >    *
> > >    * Called after HuC and GuC firmware loading during 
> > > intel_uc_init_hw().
> > >    *
> > > - * This function pins HuC firmware image object into GGTT.
> > > - * Then it invokes GuC action to authenticate passing the offset to RSA
> > > - * signature through intel_guc_auth_huc(). It then waits for 50ms for
> > > - * firmware verification ACK and unpins the object.
> > > + * This function invokes the GuC action to authenticate the HuC 
> > > firmware,
> > > + * passing the offset of the RSA signature to intel_guc_auth_huc(). 
> > > It then
> > > + * waits for up to 50ms for firmware verification ACK.
> > >    */
> > >   int intel_huc_auth(struct intel_huc *huc)
> > >   {
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c 
> > > b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > > index 74602487ed67..d654340d4d03 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > > @@ -7,21 +7,6 @@
> > >   #include "intel_huc_fw.h"
> > >   #include "i915_drv.h"
> > > -/**
> > > - * DOC: HuC Firmware
> > > - *
> > > - * Motivation:
> > > - * GEN9 introduces a new dedicated firmware for usage in media HEVC 
> > > (High
> > > - * Efficiency Video Coding) operations. Userspace can use the firmware
> > > - * capabilities by adding HuC specific commands to batch buffers.
> >
> > Having a link to the media driver here that would explain what the HuC
> > enables would be beneficial (we don't want to maintain that).
> >
> > > - *
> > > - * Implementation:
> > > - * The same firmware loader is used as the GuC. However, the actual
> > > - * loading to HW is deferred until GEM initialization is done.
> > > - *
> > > - * Note that HuC firmware loading must be done before GuC loading.
> > > - */
> >
> > Could we add a section explaining the access the HuC has to memory, and
>
> I'll have to follow up with the HuC team to understand how the memory 
> access works because I'm not too familiar with HuC. Are you ok if I 
> address all your other comments for GuC and HuC and add this as a follow 
> up later once I get the info?

Never mind, I found the info  I needed (Bspec 48058), so I can do all 
the changes in one go.

Daniele

> Daniele
>
> > one stating that the firmware is optional?
> >
> > Anyways, thanks! The series could be landed as-is already, but a few
> > more additions would be welcomed :)
> >
> > Martin
> >
> > > -
> > >   /**
> > >    * intel_huc_fw_init_early() - initializes HuC firmware struct
> > >    * @huc: intel_huc struct
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx