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

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Thomas,

On Mon, 9 Sep 2024 16:14:32 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 06.09.24 um 11:40 schrieb Steven Price:
> > The version number output when loading the firmware is actually the
> > interface version not the version of the firmware itself. Update the
> > message to make this clearer.
> >
> > However, the firmware binary has a git SHA embedded into it which can be
> > used to identify which firmware binary is being loaded. So output this
> > as a drm_info() so that it's obvious from a dmesg log which firmware
> > binary is being used.
> >
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> > v2:
> >   * Fix indentation
> >   * Also update the FW interface message to include "using interface" to
> >     make it clear it's not the FW version
> >   * Add Reviewed-bys
> >
> >   drivers/gpu/drm/panthor/panthor_fw.c | 57 +++++++++++++++++++++++++++-
> >   1 file changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index 857f3f11258a..aea5dd9a4969 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))) {
> > +		/* 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));  
> 
> Please consider making this debugging-only information. Printing takes 
> time and the information is not essential unless for debugging.

Sounds like someone working on boot time optimization :-). More
seriously, I don't mind downgrading those to debug messages, as long as
we have the same information exposed through sysfs or DEV_QUERY, but
I'd prefer doing that in a follow-up patch that takes care of all
drm_info()s we have in panthor rather than addressing the two messages
we're modifying in this patch.

Regards,

Boris