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

[Farah Kassabri]

On 5/8/2024 14:34, Jani Nikula wrote:
> On Wed, 08 May 2024, Farah Kassabri <fkassabri at 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 at 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?

I think you're right, we'll keep this change internally, no need to 
upstream it.

Thanks,
Farah

> 
>> 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 at 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
>>
>