Re: [PATCH] drm/panthor: Display FW version information

[Carsten Haitzler <carsten.haitzler@xxxxxxxxxxxx>]

On 9/5/24 10:08 PM, Liviu Dudau wrote:
> On Thu, Sep 05, 2024 at 06:45:28PM +0200, Boris Brezillon wrote:
> > On Thu,  5 Sep 2024 16:51:44 +0100
> > Steven Price <steven.price@xxxxxxx> wrote:
> >
> > > The firmware binary has a git SHA embedded into it which can be used to
> > > identify which firmware binary is being loaded. Output this as a
> > > drm_info() so that it's obvious from a dmesg log which firmware binary
> > > is being used.
> > >
> > > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> >
> > Just one formatting issue mentioned below, looks good otherwise.
> >
> > Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>
> Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
>
> > > ---
> > >   drivers/gpu/drm/panthor/panthor_fw.c | 55 ++++++++++++++++++++++++++++
> > >   1 file changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > > index 857f3f11258a..ef007287575c 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > > @@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type {
> > >   
> > >   	/** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */
> > >   	CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4,
> > > +
> > > +	/**
> > > +	 * @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how
> > > +	 * the FW binary was built.
> > > +	 */
> > > +	CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6
> > >   };
> > >   
> > >   #define CSF_FW_BINARY_ENTRY_TYPE(ehdr)					((ehdr) & 0xff)
> > > @@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr {
> > >   	} data;
> > >   };
> > >   
> > > +struct panthor_fw_build_info_hdr {
> > > +	/** @meta_start: Offset of the build info data in the FW binary */
> > > +	u32 meta_start;
> > > +	/** @meta_size: Size of the build info data in the FW binary */
> > > +	u32 meta_size;
> > > +};
> > > +
> > >   /**
> > >    * struct panthor_fw_binary_iter - Firmware binary iterator
> > >    *
> > > @@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> > >   	return 0;
> > >   }
> > >   
> > > +static int panthor_fw_read_build_info(struct panthor_device *ptdev,
> > > +				      const struct firmware *fw,
> > > +				      struct panthor_fw_binary_iter *iter,
> > > +				      u32 ehdr)
> > > +{
> > > +	struct panthor_fw_build_info_hdr hdr;
> > > +	char header[9];
> > > +	const char git_sha_header[sizeof(header)] = "git_sha: ";
> > > +	int ret;
> > > +
> > > +	ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (hdr.meta_start > fw->size ||
> > > +	    hdr.meta_start + hdr.meta_size > fw->size) {
> > > +		drm_err(&ptdev->base, "Firmware build info corrupt\n");
> > > +		/* We don't need the build info, so continue */
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (memcmp(git_sha_header, fw->data + hdr.meta_start,
> > > +	    sizeof(git_sha_header))) {
> >
> > Indentation seems broken here:
> >
> > 	if (memcmp(git_sha_header, fw->data + hdr.meta_start,
> > 		   sizeof(git_sha_header))) {
> >
> > > +		/* Not the expected header, this isn't metadata we understand */
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Check that the git SHA is NULL terminated as expected */
> > > +	if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') {
> > > +		drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n");
> > > +		/* Don't treat as fatal */
> > > +		return 0;
> > > +	}
> > > +
> > > +	drm_info(&ptdev->base, "Firmware git sha: %s\n",
> > > +		 fw->data + hdr.meta_start + sizeof(git_sha_header));
> >
> > Maybe we should also change the "FW vX.Y.Z" message into "FW interface
> > vX.Y.Z" to clarify things.
>
> Or something like "FW using interface vX.Y.Z"

+1. I like the idea. It's a bit confusing otherwise.

> Best regards,
> Liviu
>
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static void
> > >   panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload)
> > >   {
> > > @@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device *ptdev,
> > >   	switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) {
> > >   	case CSF_FW_BINARY_ENTRY_TYPE_IFACE:
> > >   		return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr);
> > > +	case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA:
> > > +		return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr);
> > >   
> > >   	/* FIXME: handle those entry types? */
> > >   	case CSF_FW_BINARY_ENTRY_TYPE_CONFIG: