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