Re: [PATCH 1/2] drm: print top commit sha after loading the driver

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 08 May 2024, Farah Kassabri <fkassabri@habana.ai> wrote:
> On 4/29/2024 18:32, Lucas De Marchi wrote:
>> On Mon, Apr 29, 2024 at 02:02:28PM GMT, Jani Nikula wrote:
>>> On Wed, 24 Apr 2024, Farah Kassabri <fkassabri@habana.ai> wrote:
>>>> Add the last driver sha to the existing log message
>>>> which prints the drm device info.
>>>
>>> The commit message fails to answer the most important question: why?
>>>
>>> Especially, what makes drm devices such special snowflakes that they'd
>>> need to be the only ones in the kernel to print git commit sha1's?
>> 
>> 
>> the closest to what was added here would be srcversion:
>> 
>>          $ modinfo build64/drivers/gpu/drm/xe/xe.ko  | grep srcversion
>>          srcversion:     0EA08A43AC399A0D942740
>> 
>> which makes more sense and doesn't depend on the git tree checkout and
>> other possible problems with dirty checkouts.  If you want to print it
>> on module load to be able to tell from a log, you could probably just
>> access mod->srcversion.
>> 
>> but I'm not sure what we are trying to accomplish here and the commit
>> message certainly missed that. Please Cc dri-devel on changes outside xe
>> and provide the motivation in the commit message.
>
> The main reason why we need this sha, is because our driver will run in 
> multiple environments and setups (whether it's for clients, our labs or 
> data center), each setup could work with different driver build and it's 
> very convenient to see what is the last sha in the running driver
> in order to see if some fix is inside or not for example.

While I sympathize with this view, it's not really an upstream problem,
is it? When you're running upstream kernels, you don't mix and match,
you run a known kernel version and the drivers that come with it.

And again, why should xe be a special snowflake in this respect? If it
were fine for xe to add this, why wouldn't it be fine for absolutely all
drivers?

> The srcversion is not good enough as it's doesn't server the purpose and 
> it's not incrementing at the same rate as the commits sha.
> Note that this apply not to all drm devices only xe, and it's optional 
> for other devices to set it or not, and in case it's not set this patch 
> will not affect the existing drm log.
> you can see we're already doing that for habanalabs driver.

Where?

Not that it justifies adding more, but I can't find it.


BR,
Jani.


> I'll update the commit message, and the dri-level alreay in CC in case 
> they see any issue with this change.
>
> BR,
> Farah
>
>> 
>> thanks
>> Lucas De Marchi
>> 
>> 
>>>
>>> BR,
>>> Jani.
>>>
>>>>
>>>> Signed-off-by: Farah Kassabri <fkassabri@habana.ai>
>>>> ---
>>>>  drivers/gpu/drm/drm_drv.c | 6 +++---
>>>>  include/drm/drm_drv.h     | 2 ++
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 535b624d4c9d..e0f7af1b6ec3 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -947,10 +947,10 @@ int drm_dev_register(struct drm_device *dev, 
>>>> unsigned long flags)
>>>>      }
>>>>      drm_panic_register(dev);
>>>>
>>>> -    DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>>>> +    DRM_INFO("Initialized %s %d.%d.%d%s %s for %s on minor %d\n",
>>>>           driver->name, driver->major, driver->minor,
>>>> -         driver->patchlevel, driver->date,
>>>> -         dev->dev ? dev_name(dev->dev) : "virtual device",
>>>> +         driver->patchlevel, driver->git_sha ? driver->git_sha : "",
>>>> +         driver->date, dev->dev ? dev_name(dev->dev) : "virtual 
>>>> device",
>>>>           dev->primary ? dev->primary->index : dev->accel->index);
>>>>
>>>>      goto out_unlock;
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index 8878260d7529..7578a1f4ce74 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -407,6 +407,8 @@ struct drm_driver {
>>>>      int minor;
>>>>      /** @patchlevel: driver patch level */
>>>>      int patchlevel;
>>>> +    /** @git_sha: driver last commit sha */
>>>> +    char *git_sha;
>>>>      /** @name: driver name */
>>>>      char *name;
>>>>      /** @desc: driver description */
>>>
>>> -- 
>>> Jani Nikula, Intel
>

-- 
Jani Nikula, Intel