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?
> 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
>
--
Jani Nikula, Intel
