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

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

[Rodrigo Vivi]

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.

Signed-off-by: Rodrigo Vivi <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

[Intel-xe] [PATCH 2/2] drm/xe: Update DRIVER_DATE to 20230726

[Rodrigo Vivi]

It is time to update the long and forgotten initial driver date.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_drv.h b/drivers/gpu/drm/xe/xe_drv.h
index d45b71426cc8..dd1a421cd4f2 100644
--- a/drivers/gpu/drm/xe/xe_drv.h
+++ b/drivers/gpu/drm/xe/xe_drv.h
@@ -10,7 +10,7 @@
 
 #define DRIVER_NAME		"xe"
 #define DRIVER_DESC		"Intel Xe Graphics"
-#define DRIVER_DATE		"20201103"
+#define DRIVER_DATE		"20230726"
 
 /* Interface history:
  *
-- 
2.41.0

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

[Matthew Brost]

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...

Anyways:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
 
> Signed-off-by: Rodrigo Vivi <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
> 

Re: [Intel-xe] [PATCH 2/2] drm/xe: Update DRIVER_DATE to 20230726

[Matthew Brost]

On Wed, Jul 26, 2023 at 03:25:20PM -0400, Rodrigo Vivi wrote:
> It is time to update the long and forgotten initial driver date.
> 

That old date is way off, AFAIK this date is long prior to Xe
development starting.

Anyways:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_drv.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_drv.h b/drivers/gpu/drm/xe/xe_drv.h
> index d45b71426cc8..dd1a421cd4f2 100644
> --- a/drivers/gpu/drm/xe/xe_drv.h
> +++ b/drivers/gpu/drm/xe/xe_drv.h
> @@ -10,7 +10,7 @@
>  
>  #define DRIVER_NAME		"xe"
>  #define DRIVER_DESC		"Intel Xe Graphics"
> -#define DRIVER_DATE		"20201103"
> +#define DRIVER_DATE		"20230726"
>  
>  /* Interface history:
>   *
> -- 
> 2.41.0
> 

Re: [Intel-xe] [PATCH 2/2] drm/xe: Update DRIVER_DATE to 20230726

[Vivi, Rodrigo]

On Wed, 2023-07-26 at 19:32 +0000, Matthew Brost wrote:
> On Wed, Jul 26, 2023 at 03:25:20PM -0400, Rodrigo Vivi wrote:
> > It is time to update the long and forgotten initial driver date.
> > 
> 
> That old date is way off, AFAIK this date is long prior to Xe
> development starting.

indeed, that was probably a copy and paste from something older
that remained.

we should probably make this into a fix up or later squash in
the original patch before the first pull request...

> 
> Anyways:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_drv.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_drv.h
> > b/drivers/gpu/drm/xe/xe_drv.h
> > index d45b71426cc8..dd1a421cd4f2 100644
> > --- a/drivers/gpu/drm/xe/xe_drv.h
> > +++ b/drivers/gpu/drm/xe/xe_drv.h
> > @@ -10,7 +10,7 @@
> >  
> >  #define DRIVER_NAME            "xe"
> >  #define DRIVER_DESC            "Intel Xe Graphics"
> > -#define DRIVER_DATE            "20201103"
> > +#define DRIVER_DATE            "20230726"
> >  
> >  /* Interface history:
> >   *
> > -- 
> > 2.41.0
> > 

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

[Vivi, Rodrigo]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

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.


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



[-- Attachment #2: Type: text/html, Size: 3803 bytes --]

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

[Lucas De Marchi]

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?

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
>
>

[Intel-xe] ✗ CI.Patch_applied: failure for series starting with [1/2] drm/xe: Add DRIVER_DATE to coredump error. (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/xe: Add DRIVER_DATE to coredump error. (rev2)
URL   : https://patchwork.freedesktop.org/series/121383/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: b8202a008 drm/xe: Always use xe_vm_queue_rebind_worker helper
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_devcoredump.c:83
error: drivers/gpu/drm/xe/xe_devcoredump.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/xe: Add DRIVER_DATE to coredump error.
Patch failed at 0001 drm/xe: Add DRIVER_DATE to coredump error.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[Rodrigo Vivi]

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

> 
> 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
> > 
> > 

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

[Lucas De Marchi]

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.

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
>> >
>> >

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

[Matthew Brost]

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
> > > >
> > > >

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

[Lucas De Marchi]

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.
>>
>
>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

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@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
>> > > >
>> > > >

Re: [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@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
> > > > > >
> > > > > >