[Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying firmware

[Arkadiusz Hiler]

On Wed, Mar 08, 2017 at 11:23:36AM +0200, Jani Nikula wrote:
> On Wed, 08 Mar 2017, "Srivatsa, Anusha" <anusha.srivatsa at intel.com> wrote:
> >>-----Original Message-----
> >>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> >>Arkadiusz Hiler
> >>Sent: Tuesday, March 7, 2017 7:25 AM
> >>To: intel-gfx at lists.freedesktop.org
> >>Subject: [Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying
> >>firmware
> >>
> >>`guc_firmware_path` and `huc_firmware_path` module parameters are added.
> >>
> >>Using the parameter disables version checks and loads desired firmware instead
> >>of the default one.
> >
> > I see that the effort of this patch makes us test with different
> > firmware versions and not just the default one. But is it worth
> > introducing two new params ? We already have 3 parameters that are guc
> > and huc related.
> 
> Obviously I'd prefer there were fewer module parameters, but looks like
> they multiply like rabbits...
> 
> Back when we decided that we only accept one firmware version, there
> were complaints about it becoming hard to test various firmware versions
> or to bisect the kernel while keeping the firmware constant. This
> addresses those issues. If you decide those are non-issues and the patch
> is not needed, I'll point whoever complains about the issues to this
> discussion.
> 
> >>-	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> >>-	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> >>+	if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
> >>+		DRM_NOTE("Skipping uC firmware version check\n");
> 
> Log the version found in the firmware? Or does that happens somewhere
> else already?

It's logging the version couple lines down using DRM_DEBUG_DRIVER(). The
log in the "else if" is there because we are reporting error and
then returning.

This function requires general cleanup, especially when it comes to the
log messages. I'll do it after this series lands (it's starting to go
out of hand already), along with other fixes, using targeted and
independent patches.

-- 
Cheers,
Arek