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

[Rodrigo Vivi]

On Thu, Jul 27, 2023 at 09:04:55AM -0300, Lucas De Marchi wrote:
> On Thu, Jul 27, 2023 at 12:11:45AM +0000, Matthew Brost wrote:
> > 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.

Fair point. Maybe this could be used to convince the drm level to get
entirely rid of drm->date?

> > > 
> > 
> > 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.
> 
> I don't see how the date helps with that. Nobody really maintains the
> date as the grep above shows. Even if we decided "we are going to be the
> outlier and really maintain this because we like", then it would be a
> date every .... 3 months?  How is that any useful for whom got the
> coredump?  Once we are merged upstream, having the *kernel* version is
> maybe useful, but the date doesn't really tell much

Well, some bug reports we receive might come from end users without git
and from trees that are ports and not necessarily the mainline, so even
the version wouldn't tell where actually that code came from...

But well, without a good maintenance of the date, that is useless anyway
indeed.

So, okay, let's drop this patch for now... let's move with the other
patch as a fixup at last to not start with an insanely old version?

> 
> Lucas De Marchi
> 
> > 
> > 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 at intel.com<mailto:matthew.brost at intel.com>>
> > > > > >
> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com<mailto:rodrigo.vivi at 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
> > > > > >
> > > > > >