From: Jani Nikula <jani.nikula@linux.intel.com>
To: Farah Kassabri <fkassabri@habana.ai>,
"De Marchi, Lucas" <lucas.demarchi@intel.com>
Cc: "intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>,
Ohad Sharabi <osharabi@habana.ai>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm: print top commit sha after loading the driver
Date: Wed, 08 May 2024 14:34:08 +0300 [thread overview]
Message-ID: <87seysk05r.fsf@intel.com> (raw)
In-Reply-To: <6a605dbe-53c9-416b-9bdc-728c3e155256@habana.ai>
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
next prev parent reply other threads:[~2024-05-08 11:34 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240424100706.269523-1-fkassabri@habana.ai>
[not found] ` <87bk5s4ekb.fsf@intel.com>
2024-04-29 15:32 ` [PATCH 1/2] drm: print top commit sha after loading the driver Lucas De Marchi
2024-05-08 11:01 ` Farah Kassabri
2024-05-08 11:34 ` Jani Nikula [this message]
2024-05-16 14:17 ` Farah Kassabri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87seysk05r.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=fkassabri@habana.ai \
--cc=intel-xe@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=osharabi@habana.ai \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).