Re: [Intel-xe] [PATCH 1/2] drm/xe: Add DRIVER_DATE to coredump error.

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jul 26, 2023 at 06:58:01PM -0300, Lucas De Marchi wrote:
> On Wed, Jul 26, 2023 at 05:35:08PM -0400, Rodrigo Vivi wrote:
> > On Wed, Jul 26, 2023 at 05:38:13PM -0300, Lucas De Marchi wrote:
> > > On Wed, Jul 26, 2023 at 07:37:43PM +0000, Rodrigo Vivi wrote:
> > > > On Wed, 2023-07-26 at 19:29 +0000, Matthew Brost wrote:
> > > > On Wed, Jul 26, 2023 at 03:25:19PM -0400, Rodrigo Vivi wrote:
> > > > This might be an useful debugging information if the driver
> > > > date is properly updated with a certain cadence. With this
> > > > we know from which point in time in our development the
> > > > driver was from.
> > > >
> > > >
> > > > Make sense to have this, wondering what the cadence should be...
> > > >
> > > > before we are merged we should probably change the date along with our rebases.
> > > >
> > > > after we are in tree probably after the latest pull request towards the next version.
> > > 
> > > 
> > > does it really make sense though? I think this exists only because of a
> > > pre-git era. What's the problem with using the git short hash that will
> > > automatically be updated and accurate?
> > 
> > well, we still have the drm->date to fill so we need to maintain it.
> > 
> > maybe we could do like nouveau?!
> > 
> > #ifdef GIT_REVISION
> >        .date = GIT_REVISION,
> > #else
> >        .date = DRIVER_DATE,
> > #endif
> 
> Sounds better. I don't see any value on DRIVER_DATE and having to update
> it. Just setting to some dummy value when GIT_REVISION is not available
> would be ok
> 
> 	$ git grep "\.date\s*=" -- drivers/gpu/drm/ | grep 2023
> 	$ git grep DRIVER_DATE -- drivers/gpu/drm/ | grep 2023
> 
> Tells me nobody really maintains that.
> 

Well I think we should attempt to maintain this, a date is helpful to
give us a very quick idea of when the driver is from without having to
dig through the git logs.

My opinion is both of these patches are valid but let's add a 3rd patch
which also includes the GIT_REVISION in the core dump and perhaps a 4th
that makes the date plus GIT_REVISION easily available in debugfs entry
or something.

Matt

> Lucas De Marchi
> 
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > >
> > > >
> > > > Anyways:
> > > > Reviewed-by: Matthew Brost <matthew.brost@intel.com<mailto:matthew.brost@intel.com>>
> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com<mailto:rodrigo.vivi@intel.com>>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_devcoredump.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > index f53f4b51233a..79b506dc2622 100644
> > > > --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > @@ -10,6 +10,7 @@
> > > > #include <generated/utsrelease.h>
> > > >
> > > > #include "xe_device.h"
> > > > +#include "xe_drv.h"
> > > > #include "xe_engine.h"
> > > > #include "xe_force_wake.h"
> > > > #include "xe_gt.h"
> > > > @@ -83,6 +84,7 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
> > > >        drm_printf(&p, "**** Xe Device Coredump ****\n");
> > > >        drm_printf(&p, "kernel: " UTS_RELEASE "\n");
> > > >        drm_printf(&p, "module: " KBUILD_MODNAME "\n");
> > > > +       drm_printf(&p, "driver date: " DRIVER_DATE "\n");
> > > >
> > > >        ts = ktime_to_timespec64(ss->snapshot_time);
> > > >        drm_printf(&p, "Snapshot time: %lld.%09ld\n", ts.tv_sec, ts.tv_nsec);
> > > > --
> > > > 2.41.0
> > > >
> > > >