[PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Rodrigo Vivi]

We do not break userspace.

This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.

Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper
function")
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Julia Filipchuk <julia.filipchuk@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_devcoredump.c | 87 -----------------------------
 drivers/gpu/drm/xe/xe_devcoredump.h |  6 --
 2 files changed, 93 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
index 71636e80b71d..025b70b0eb0f 100644
--- a/drivers/gpu/drm/xe/xe_devcoredump.c
+++ b/drivers/gpu/drm/xe/xe_devcoredump.c
@@ -6,7 +6,6 @@
 #include "xe_devcoredump.h"
 #include "xe_devcoredump_types.h"
 
-#include <linux/ascii85.h>
 #include <linux/devcoredump.h>
 #include <generated/utsrelease.h>
 
@@ -387,89 +386,3 @@ int xe_devcoredump_init(struct xe_device *xe)
 }
 
 #endif
-
-/**
- * xe_print_blob_ascii85 - print a BLOB to some useful location in ASCII85
- *
- * The output is split to multiple lines because some print targets, e.g. dmesg
- * cannot handle arbitrarily long lines. Note also that printing to dmesg in
- * piece-meal fashion is not possible, each separate call to drm_puts() has a
- * line-feed automatically added! Therefore, the entire output line must be
- * constructed in a local buffer first, then printed in one atomic output call.
- *
- * There is also a scheduler yield call to prevent the 'task has been stuck for
- * 120s' kernel hang check feature from firing when printing to a slow target
- * such as dmesg over a serial port.
- *
- * TODO: Add compression prior to the ASCII85 encoding to shrink huge buffers down.
- *
- * @p: the printer object to output to
- * @prefix: optional prefix to add to output string
- * @blob: the Binary Large OBject to dump out
- * @offset: offset in bytes to skip from the front of the BLOB, must be a multiple of sizeof(u32)
- * @size: the size in bytes of the BLOB, must be a multiple of sizeof(u32)
- */
-void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
-			   const void *blob, size_t offset, size_t size)
-{
-	const u32 *blob32 = (const u32 *)blob;
-	char buff[ASCII85_BUFSZ], *line_buff;
-	size_t line_pos = 0;
-
-#define DMESG_MAX_LINE_LEN	800
-#define MIN_SPACE		(ASCII85_BUFSZ + 2)		/* 85 + "\n\0" */
-
-	if (size & 3)
-		drm_printf(p, "Size not word aligned: %zu", size);
-	if (offset & 3)
-		drm_printf(p, "Offset not word aligned: %zu", size);
-
-	line_buff = kzalloc(DMESG_MAX_LINE_LEN, GFP_KERNEL);
-	if (IS_ERR_OR_NULL(line_buff)) {
-		drm_printf(p, "Failed to allocate line buffer: %pe", line_buff);
-		return;
-	}
-
-	blob32 += offset / sizeof(*blob32);
-	size /= sizeof(*blob32);
-
-	if (prefix) {
-		strscpy(line_buff, prefix, DMESG_MAX_LINE_LEN - MIN_SPACE - 2);
-		line_pos = strlen(line_buff);
-
-		line_buff[line_pos++] = ':';
-		line_buff[line_pos++] = ' ';
-	}
-
-	while (size--) {
-		u32 val = *(blob32++);
-
-		strscpy(line_buff + line_pos, ascii85_encode(val, buff),
-			DMESG_MAX_LINE_LEN - line_pos);
-		line_pos += strlen(line_buff + line_pos);
-
-		if ((line_pos + MIN_SPACE) >= DMESG_MAX_LINE_LEN) {
-			line_buff[line_pos++] = '\n';
-			line_buff[line_pos++] = 0;
-
-			drm_puts(p, line_buff);
-
-			line_pos = 0;
-
-			/* Prevent 'stuck thread' time out errors */
-			cond_resched();
-		}
-	}
-
-	if (line_pos) {
-		line_buff[line_pos++] = '\n';
-		line_buff[line_pos++] = 0;
-
-		drm_puts(p, line_buff);
-	}
-
-	kfree(line_buff);
-
-#undef MIN_SPACE
-#undef DMESG_MAX_LINE_LEN
-}
diff --git a/drivers/gpu/drm/xe/xe_devcoredump.h b/drivers/gpu/drm/xe/xe_devcoredump.h
index 6a17e6d60102..1f7fabfa2eb4 100644
--- a/drivers/gpu/drm/xe/xe_devcoredump.h
+++ b/drivers/gpu/drm/xe/xe_devcoredump.h
@@ -6,9 +6,6 @@
 #ifndef _XE_DEVCOREDUMP_H_
 #define _XE_DEVCOREDUMP_H_
 
-#include <linux/types.h>
-
-struct drm_printer;
 struct xe_device;
 struct xe_exec_queue;
 struct xe_sched_job;
@@ -29,7 +26,4 @@ static inline int xe_devcoredump_init(struct xe_device *xe)
 }
 #endif
 
-void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
-			   const void *blob, size_t offset, size_t size);
-
 #endif
-- 
2.47.1

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Lucas De Marchi]

On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>We do not break userspace.
>
>This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.

But we have users calling this function.... the revert is not so simple.
I think we need to revert the functionality rather than reverting all
the patches, otherwise it will cause a lot of headaches.

I propose we go with:

a) drop the \n that broke mesa and merge that with cc stable.

b) move back the entry to the previous section that broke mesa and cc
    stable.

    José, would it be ok to merge a patch in mesa and port that
    to mesa stable that simply looks at 2 possible sections? Or even
    drop the section checks... ?

c) settle on one of the possible solutions for \n that were presented in
    this discussion.  I think the sanest one is that a space in the end of a
    line printing ascii85 would signify a continuation marker.  But merging
    (a) means we have time to discuss and adjust if needed.

After the dust settles we may think of a better way to evolve the
devcoredump format so it doesn't cause breakages in future updates.

Lucas De Marchi

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > We do not break userspace.

There is other patch that also breaks Mesa parser:

drm/xe/devcoredump: Improve section headings and add tile info

> > 
> > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> 
> But we have users calling this function.... the revert is not so simple.
> I think we need to revert the functionality rather than reverting all
> the patches, otherwise it will cause a lot of headaches.
> 
> I propose we go with:
> 
> a) drop the \n that broke mesa and merge that with cc stable.
> 
> b) move back the entry to the previous section that broke mesa and cc
>     stable.
> 
>     José, would it be ok to merge a patch in mesa and port that
>     to mesa stable that simply looks at 2 possible sections? Or even
>     drop the section checks... ?
> 
> c) settle on one of the possible solutions for \n that were presented in
>     this discussion.  I think the sanest one is that a space in the end of a
>     line printing ascii85 would signify a continuation marker.  But merging
>     (a) means we have time to discuss and adjust if needed.
> 
> After the dust settles we may think of a better way to evolve the
> devcoredump format so it doesn't cause breakages in future updates.
> 
> Lucas De Marchi

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Lucas De Marchi]

On Fri, Dec 13, 2024 at 09:10:05AM -0600, Jose Souza wrote:
>On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>> On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>> > We do not break userspace.
>
>There is other patch that also breaks Mesa parser:
>
>drm/xe/devcoredump: Improve section headings and add tile info

isn't that the item (b) below? At least it what I meant it to be looking
at the regression discussion in
https://lore.kernel.org/all/ca2978d748368f37e0fd29fd65b059c52b1b755f.camel@intel.com/

Lucas De Marchi

>
>> >
>> > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>
>> But we have users calling this function.... the revert is not so simple.
>> I think we need to revert the functionality rather than reverting all
>> the patches, otherwise it will cause a lot of headaches.
>>
>> I propose we go with:
>>
>> a) drop the \n that broke mesa and merge that with cc stable.
>>
>> b) move back the entry to the previous section that broke mesa and cc
>>     stable.
>>
>>     José, would it be ok to merge a patch in mesa and port that
>>     to mesa stable that simply looks at 2 possible sections? Or even
>>     drop the section checks... ?
>>
>> c) settle on one of the possible solutions for \n that were presented in
>>     this discussion.  I think the sanest one is that a space in the end of a
>>     line printing ascii85 would signify a continuation marker.  But merging
>>     (a) means we have time to discuss and adjust if needed.
>>
>> After the dust settles we may think of a better way to evolve the
>> devcoredump format so it doesn't cause breakages in future updates.
>>
>> Lucas De Marchi
>

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
> On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > > We do not break userspace.
> 
> There is other patch that also breaks Mesa parser:
> 
> drm/xe/devcoredump: Improve section headings and add tile info
> 
> > > 
> > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> > 
> > But we have users calling this function.... the revert is not so simple.
> > I think we need to revert the functionality rather than reverting all
> > the patches, otherwise it will cause a lot of headaches.
> > 
> > I propose we go with:
> > 
> > a) drop the \n that broke mesa and merge that with cc stable.
> > 
> > b) move back the entry to the previous section that broke mesa and cc
> >     stable.
> > 
> >     José, would it be ok to merge a patch in mesa and port that
> >     to mesa stable that simply looks at 2 possible sections? Or even
> >     drop the section checks... ?

But if Xe KMD is reverting the patch that changed the hwctx section why would Mesa need to also parse the new(future to be reverted) section?
I could CC the Mesa stable but it would not be picked to all Mesa versions with the devcoredump parser.


> > 
> > c) settle on one of the possible solutions for \n that were presented in
> >     this discussion.  I think the sanest one is that a space in the end of a
> >     line printing ascii85 would signify a continuation marker.  But merging
> >     (a) means we have time to discuss and adjust if needed.
> > 
> > After the dust settles we may think of a better way to evolve the
> > devcoredump format so it doesn't cause breakages in future updates.
> > 
> > Lucas De Marchi
> 

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Lucas De Marchi]

On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>> On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>> > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>> > > We do not break userspace.
>>
>> There is other patch that also breaks Mesa parser:
>>
>> drm/xe/devcoredump: Improve section headings and add tile info
>>
>> > >
>> > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>> >
>> > But we have users calling this function.... the revert is not so simple.
>> > I think we need to revert the functionality rather than reverting all
>> > the patches, otherwise it will cause a lot of headaches.
>> >
>> > I propose we go with:
>> >
>> > a) drop the \n that broke mesa and merge that with cc stable.
>> >
>> > b) move back the entry to the previous section that broke mesa and cc
>> >     stable.
>> >
>> >     José, would it be ok to merge a patch in mesa and port that
>> >     to mesa stable that simply looks at 2 possible sections? Or even
>> >     drop the section checks... ?
>
>But if Xe KMD is reverting the patch that changed the hwctx section why would Mesa need to also parse the new(future to be reverted) section?

first is to undo the damage, with 0 changes in mesa. We do that first and
*then* we agree on what's possible to do to accomodate the 2 parsers we
have.

If we can get something in mesa to work that is backward compatible (i.e. the
changed parser is able to parse both before and after the kernel change),
then it could be considered to a mesa stable and the kernel side
changed.

Lucas De Marchi

>I could CC the Mesa stable but it would not be picked to all Mesa versions with the devcoredump parser.
>
>
>> >
>> > c) settle on one of the possible solutions for \n that were presented in
>> >     this discussion.  I think the sanest one is that a space in the end of a
>> >     line printing ascii85 would signify a continuation marker.  But merging
>> >     (a) means we have time to discuss and adjust if needed.
>> >
>> > After the dust settles we may think of a better way to evolve the
>> > devcoredump format so it doesn't cause breakages in future updates.
>> >
>> > Lucas De Marchi
>>
>

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
> > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
> > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > > > > We do not break userspace.
> > > 
> > > There is other patch that also breaks Mesa parser:
> > > 
> > > drm/xe/devcoredump: Improve section headings and add tile info
> > > 
> > > > > 
> > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> > > > 
> > > > But we have users calling this function.... the revert is not so simple.
> > > > I think we need to revert the functionality rather than reverting all
> > > > the patches, otherwise it will cause a lot of headaches.
> > > > 
> > > > I propose we go with:
> > > > 
> > > > a) drop the \n that broke mesa and merge that with cc stable.
> > > > 
> > > > b) move back the entry to the previous section that broke mesa and cc
> > > >     stable.
> > > > 
> > > >     José, would it be ok to merge a patch in mesa and port that
> > > >     to mesa stable that simply looks at 2 possible sections? Or even
> > > >     drop the section checks... ?
> > 
> > But if Xe KMD is reverting the patch that changed the hwctx section why would Mesa need to also parse the new(future to be reverted) section?
> 
> first is to undo the damage, with 0 changes in mesa. We do that first and
> *then* we agree on what's possible to do to accomodate the 2 parsers we
> have.
> 
> If we can get something in mesa to work that is backward compatible (i.e. the
> changed parser is able to parse both before and after the kernel change),
> then it could be considered to a mesa stable and the kernel side
> changed.

Okay, reasonable plan. But the ascii85 encoder with \n will not be brought back right?
With that I can write the Mesa patch to support both sections, then it is up to other Mesa team members agree with it.

> 
> Lucas De Marchi
> 
> > I could CC the Mesa stable but it would not be picked to all Mesa versions with the devcoredump parser.
> > 
> > 
> > > > 
> > > > c) settle on one of the possible solutions for \n that were presented in
> > > >     this discussion.  I think the sanest one is that a space in the end of a
> > > >     line printing ascii85 would signify a continuation marker.  But merging
> > > >     (a) means we have time to discuss and adjust if needed.
> > > > 
> > > > After the dust settles we may think of a better way to evolve the
> > > > devcoredump format so it doesn't cause breakages in future updates.
> > > > 
> > > > Lucas De Marchi
> > > 
> > 

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Lucas De Marchi]

On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>> > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>> > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>> > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>> > > > > We do not break userspace.
>> > >
>> > > There is other patch that also breaks Mesa parser:
>> > >
>> > > drm/xe/devcoredump: Improve section headings and add tile info
>> > >
>> > > > >
>> > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>> > > >
>> > > > But we have users calling this function.... the revert is not so simple.
>> > > > I think we need to revert the functionality rather than reverting all
>> > > > the patches, otherwise it will cause a lot of headaches.
>> > > >
>> > > > I propose we go with:
>> > > >
>> > > > a) drop the \n that broke mesa and merge that with cc stable.
>> > > >
>> > > > b) move back the entry to the previous section that broke mesa and cc
>> > > >     stable.
>> > > >
>> > > >     José, would it be ok to merge a patch in mesa and port that
>> > > >     to mesa stable that simply looks at 2 possible sections? Or even
>> > > >     drop the section checks... ?
>> >
>> > But if Xe KMD is reverting the patch that changed the hwctx section why would Mesa need to also parse the new(future to be reverted) section?
>>
>> first is to undo the damage, with 0 changes in mesa. We do that first and
>> *then* we agree on what's possible to do to accomodate the 2 parsers we
>> have.
>>
>> If we can get something in mesa to work that is backward compatible (i.e. the
>> changed parser is able to parse both before and after the kernel change),
>> then it could be considered to a mesa stable and the kernel side
>> changed.
>
>Okay, reasonable plan. But the ascii85 encoder with \n will not be brought back right?

maybe let's agree on how to possibly bring it back? I suggested using a
space as continuation line char. This way you can just check the last char
returned by getline() you are calling and see if you can go ahead and
proceed or if you still need to get more data. Neither space nor newline
are part of the ascii85 character set, so it's safe and you can handle
continuation in one place in your loop.

if you are just ignoring any ascii85, then I believe it's even simpler:
you check sections and keys with a space since both keys and section
titles contain space, which is not part of the ascii85 char set.

Lucas De Marchi

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[John Harrison]

On 12/13/2024 08:48, Lucas De Marchi wrote:
> On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>> On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>>> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>>> > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>>> > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>>> > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>>> > > > > We do not break userspace.
>>> > >
>>> > > There is other patch that also breaks Mesa parser:
>>> > >
>>> > > drm/xe/devcoredump: Improve section headings and add tile info
>>> > >
>>> > > > >
>>> > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>> > > >
>>> > > > But we have users calling this function.... the revert is not 
>>> so simple.
>>> > > > I think we need to revert the functionality rather than 
>>> reverting all
>>> > > > the patches, otherwise it will cause a lot of headaches.
>>> > > >
>>> > > > I propose we go with:
>>> > > >
>>> > > > a) drop the \n that broke mesa and merge that with cc stable.
>>> > > >
>>> > > > b) move back the entry to the previous section that broke mesa 
>>> and cc
>>> > > >     stable.
>>> > > >
>>> > > >     José, would it be ok to merge a patch in mesa and port that
>>> > > >     to mesa stable that simply looks at 2 possible sections? 
>>> Or even
>>> > > >     drop the section checks... ?
>>> >
>>> > But if Xe KMD is reverting the patch that changed the hwctx 
>>> section why would Mesa need to also parse the new(future to be 
>>> reverted) section?
>>>
>>> first is to undo the damage, with 0 changes in mesa. We do that 
>>> first and
>>> *then* we agree on what's possible to do to accomodate the 2 parsers we
>>> have.
>>>
>>> If we can get something in mesa to work that is backward compatible 
>>> (i.e. the
>>> changed parser is able to parse both before and after the kernel 
>>> change),
>>> then it could be considered to a mesa stable and the kernel side
>>> changed.
>>
>> Okay, reasonable plan. But the ascii85 encoder with \n will not be 
>> brought back right?
>
> maybe let's agree on how to possibly bring it back? I suggested using a
> space as continuation line char. This way you can just check the last 
> char
> returned by getline() you are calling and see if you can go ahead and
> proceed or if you still need to get more data. Neither space nor newline
> are part of the ascii85 character set, so it's safe and you can handle
> continuation in one place in your loop.
>
> if you are just ignoring any ascii85, then I believe it's even simpler:
> you check sections and keys with a space since both keys and section
> titles contain space, which is not part of the ascii85 char set.
>
> Lucas De Marchi
Yes, I would strongly prefer to use line wrapped ASCII85 data for all 
blobs in the devcoredump. Including things like batch buffers and other 
VM entries that the mesa tool is presumably wanting to decode.

If adding a <space> character to the end of each line is an acceptable 
fix then I have no problems with that. But not line wrapping at all 
means having to carry that change as a non-upstream patch in either the 
internal tree or in individual developer's local trees. Either that or 
we just cannot debug a lot of hard to repro problems.

John.

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
> On 12/13/2024 08:48, Lucas De Marchi wrote:
> > On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
> > > On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
> > > > On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
> > > > > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
> > > > > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> > > > > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > > > > > > > We do not break userspace.
> > > > > > 
> > > > > > There is other patch that also breaks Mesa parser:
> > > > > > 
> > > > > > drm/xe/devcoredump: Improve section headings and add tile info
> > > > > > 
> > > > > > > > 
> > > > > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> > > > > > > 
> > > > > > > But we have users calling this function.... the revert is not 
> > > > so simple.
> > > > > > > I think we need to revert the functionality rather than 
> > > > reverting all
> > > > > > > the patches, otherwise it will cause a lot of headaches.
> > > > > > > 
> > > > > > > I propose we go with:
> > > > > > > 
> > > > > > > a) drop the \n that broke mesa and merge that with cc stable.
> > > > > > > 
> > > > > > > b) move back the entry to the previous section that broke mesa 
> > > > and cc
> > > > > > >      stable.
> > > > > > > 
> > > > > > >      José, would it be ok to merge a patch in mesa and port that
> > > > > > >      to mesa stable that simply looks at 2 possible sections? 
> > > > Or even
> > > > > > >      drop the section checks... ?
> > > > > 
> > > > > But if Xe KMD is reverting the patch that changed the hwctx 
> > > > section why would Mesa need to also parse the new(future to be 
> > > > reverted) section?
> > > > 
> > > > first is to undo the damage, with 0 changes in mesa. We do that 
> > > > first and
> > > > *then* we agree on what's possible to do to accomodate the 2 parsers we
> > > > have.
> > > > 
> > > > If we can get something in mesa to work that is backward compatible 
> > > > (i.e. the
> > > > changed parser is able to parse both before and after the kernel 
> > > > change),
> > > > then it could be considered to a mesa stable and the kernel side
> > > > changed.
> > > 
> > > Okay, reasonable plan. But the ascii85 encoder with \n will not be 
> > > brought back right?
> > 
> > maybe let's agree on how to possibly bring it back? I suggested using a
> > space as continuation line char. This way you can just check the last 
> > char
> > returned by getline() you are calling and see if you can go ahead and
> > proceed or if you still need to get more data. Neither space nor newline
> > are part of the ascii85 character set, so it's safe and you can handle
> > continuation in one place in your loop.
> > 
> > if you are just ignoring any ascii85, then I believe it's even simpler:
> > you check sections and keys with a space since both keys and section
> > titles contain space, which is not part of the ascii85 char set.
> > 
> > Lucas De Marchi
> Yes, I would strongly prefer to use line wrapped ASCII85 data for all 
> blobs in the devcoredump. Including things like batch buffers and other 
> VM entries that the mesa tool is presumably wanting to decode.
> 
> If adding a <space> character to the end of each line is an acceptable 
> fix then I have no problems with that. But not line wrapping at all 
> means having to carry that change as a non-upstream patch in either the 
> internal tree or in individual developer's local trees. Either that or 
> we just cannot debug a lot of hard to repro problems.

Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?

> 
> John.
> 

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[John Harrison]

On 12/13/2024 09:25, Souza, Jose wrote:
> On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
>> On 12/13/2024 08:48, Lucas De Marchi wrote:
>>> On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>>>> On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>>>>> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>>>>>> On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>>>>>>> On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>>>>>>>> On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>>>>>>>>> We do not break userspace.
>>>>>>> There is other patch that also breaks Mesa parser:
>>>>>>>
>>>>>>> drm/xe/devcoredump: Improve section headings and add tile info
>>>>>>>
>>>>>>>>> This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>>>>>>> But we have users calling this function.... the revert is not
>>>>> so simple.
>>>>>>>> I think we need to revert the functionality rather than
>>>>> reverting all
>>>>>>>> the patches, otherwise it will cause a lot of headaches.
>>>>>>>>
>>>>>>>> I propose we go with:
>>>>>>>>
>>>>>>>> a) drop the \n that broke mesa and merge that with cc stable.
>>>>>>>>
>>>>>>>> b) move back the entry to the previous section that broke mesa
>>>>> and cc
>>>>>>>>       stable.
>>>>>>>>
>>>>>>>>       José, would it be ok to merge a patch in mesa and port that
>>>>>>>>       to mesa stable that simply looks at 2 possible sections?
>>>>> Or even
>>>>>>>>       drop the section checks... ?
>>>>>> But if Xe KMD is reverting the patch that changed the hwctx
>>>>> section why would Mesa need to also parse the new(future to be
>>>>> reverted) section?
>>>>>
>>>>> first is to undo the damage, with 0 changes in mesa. We do that
>>>>> first and
>>>>> *then* we agree on what's possible to do to accomodate the 2 parsers we
>>>>> have.
>>>>>
>>>>> If we can get something in mesa to work that is backward compatible
>>>>> (i.e. the
>>>>> changed parser is able to parse both before and after the kernel
>>>>> change),
>>>>> then it could be considered to a mesa stable and the kernel side
>>>>> changed.
>>>> Okay, reasonable plan. But the ascii85 encoder with \n will not be
>>>> brought back right?
>>> maybe let's agree on how to possibly bring it back? I suggested using a
>>> space as continuation line char. This way you can just check the last
>>> char
>>> returned by getline() you are calling and see if you can go ahead and
>>> proceed or if you still need to get more data. Neither space nor newline
>>> are part of the ascii85 character set, so it's safe and you can handle
>>> continuation in one place in your loop.
>>>
>>> if you are just ignoring any ascii85, then I believe it's even simpler:
>>> you check sections and keys with a space since both keys and section
>>> titles contain space, which is not part of the ascii85 char set.
>>>
>>> Lucas De Marchi
>> Yes, I would strongly prefer to use line wrapped ASCII85 data for all
>> blobs in the devcoredump. Including things like batch buffers and other
>> VM entries that the mesa tool is presumably wanting to decode.
>>
>> If adding a <space> character to the end of each line is an acceptable
>> fix then I have no problems with that. But not line wrapping at all
>> means having to carry that change as a non-upstream patch in either the
>> internal tree or in individual developer's local trees. Either that or
>> we just cannot debug a lot of hard to repro problems.
> Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?
Apparently not. Even when not deliberately line wrapping the output, I 
am still seeing it being wrapped when dumping very large buffers such as 
the GuC log. It looks like something in a lower layer is also forcing 
line wrapping of super long lines. So either we can't add full size GuC 
logs to the devcoredump or we need to support line wrapped data in the 
mesa tool.

John.

>
>> John.
>>

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 09:38 -0800, John Harrison wrote:
> On 12/13/2024 09:25, Souza, Jose wrote:
> > On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
> > > On 12/13/2024 08:48, Lucas De Marchi wrote:
> > > > On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
> > > > > On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
> > > > > > On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
> > > > > > > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
> > > > > > > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> > > > > > > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > > > > > > > > > We do not break userspace.
> > > > > > > > There is other patch that also breaks Mesa parser:
> > > > > > > > 
> > > > > > > > drm/xe/devcoredump: Improve section headings and add tile info
> > > > > > > > 
> > > > > > > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> > > > > > > > > But we have users calling this function.... the revert is not
> > > > > > so simple.
> > > > > > > > > I think we need to revert the functionality rather than
> > > > > > reverting all
> > > > > > > > > the patches, otherwise it will cause a lot of headaches.
> > > > > > > > > 
> > > > > > > > > I propose we go with:
> > > > > > > > > 
> > > > > > > > > a) drop the \n that broke mesa and merge that with cc stable.
> > > > > > > > > 
> > > > > > > > > b) move back the entry to the previous section that broke mesa
> > > > > > and cc
> > > > > > > > >       stable.
> > > > > > > > > 
> > > > > > > > >       José, would it be ok to merge a patch in mesa and port that
> > > > > > > > >       to mesa stable that simply looks at 2 possible sections?
> > > > > > Or even
> > > > > > > > >       drop the section checks... ?
> > > > > > > But if Xe KMD is reverting the patch that changed the hwctx
> > > > > > section why would Mesa need to also parse the new(future to be
> > > > > > reverted) section?
> > > > > > 
> > > > > > first is to undo the damage, with 0 changes in mesa. We do that
> > > > > > first and
> > > > > > *then* we agree on what's possible to do to accomodate the 2 parsers we
> > > > > > have.
> > > > > > 
> > > > > > If we can get something in mesa to work that is backward compatible
> > > > > > (i.e. the
> > > > > > changed parser is able to parse both before and after the kernel
> > > > > > change),
> > > > > > then it could be considered to a mesa stable and the kernel side
> > > > > > changed.
> > > > > Okay, reasonable plan. But the ascii85 encoder with \n will not be
> > > > > brought back right?
> > > > maybe let's agree on how to possibly bring it back? I suggested using a
> > > > space as continuation line char. This way you can just check the last
> > > > char
> > > > returned by getline() you are calling and see if you can go ahead and
> > > > proceed or if you still need to get more data. Neither space nor newline
> > > > are part of the ascii85 character set, so it's safe and you can handle
> > > > continuation in one place in your loop.
> > > > 
> > > > if you are just ignoring any ascii85, then I believe it's even simpler:
> > > > you check sections and keys with a space since both keys and section
> > > > titles contain space, which is not part of the ascii85 char set.
> > > > 
> > > > Lucas De Marchi
> > > Yes, I would strongly prefer to use line wrapped ASCII85 data for all
> > > blobs in the devcoredump. Including things like batch buffers and other
> > > VM entries that the mesa tool is presumably wanting to decode.
> > > 
> > > If adding a <space> character to the end of each line is an acceptable
> > > fix then I have no problems with that. But not line wrapping at all
> > > means having to carry that change as a non-upstream patch in either the
> > > internal tree or in individual developer's local trees. Either that or
> > > we just cannot debug a lot of hard to repro problems.
> > Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?
> Apparently not. Even when not deliberately line wrapping the output, I 
> am still seeing it being wrapped when dumping very large buffers such as 
> the GuC log. It looks like something in a lower layer is also forcing 
> line wrapping of super long lines. So either we can't add full size GuC 
> logs to the devcoredump or we need to support line wrapped data in the 
> mesa tool.

It is probably some implementation detail of __drm_printfn_coredump(). That could be replaced to something like i915 error dump write functions to not
have any limit at least when the target is a file descriptor, for dmesg you are free to add any line wrapping.

> 
> John.
> 
> > 
> > > John.
> > > 
> 

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[John Harrison]

On 12/13/2024 11:43, Souza, Jose wrote:
> On Fri, 2024-12-13 at 09:38 -0800, John Harrison wrote:
>> On 12/13/2024 09:25, Souza, Jose wrote:
>>> On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
>>>> On 12/13/2024 08:48, Lucas De Marchi wrote:
>>>>> On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>>>>>> On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>>>>>>> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>>>>>>>> On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>>>>>>>>> On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>>>>>>>>>> On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>>>>>>>>>>> We do not break userspace.
>>>>>>>>> There is other patch that also breaks Mesa parser:
>>>>>>>>>
>>>>>>>>> drm/xe/devcoredump: Improve section headings and add tile info
>>>>>>>>>
>>>>>>>>>>> This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>>>>>>>>> But we have users calling this function.... the revert is not
>>>>>>> so simple.
>>>>>>>>>> I think we need to revert the functionality rather than
>>>>>>> reverting all
>>>>>>>>>> the patches, otherwise it will cause a lot of headaches.
>>>>>>>>>>
>>>>>>>>>> I propose we go with:
>>>>>>>>>>
>>>>>>>>>> a) drop the \n that broke mesa and merge that with cc stable.
>>>>>>>>>>
>>>>>>>>>> b) move back the entry to the previous section that broke mesa
>>>>>>> and cc
>>>>>>>>>>        stable.
>>>>>>>>>>
>>>>>>>>>>        José, would it be ok to merge a patch in mesa and port that
>>>>>>>>>>        to mesa stable that simply looks at 2 possible sections?
>>>>>>> Or even
>>>>>>>>>>        drop the section checks... ?
>>>>>>>> But if Xe KMD is reverting the patch that changed the hwctx
>>>>>>> section why would Mesa need to also parse the new(future to be
>>>>>>> reverted) section?
>>>>>>>
>>>>>>> first is to undo the damage, with 0 changes in mesa. We do that
>>>>>>> first and
>>>>>>> *then* we agree on what's possible to do to accomodate the 2 parsers we
>>>>>>> have.
>>>>>>>
>>>>>>> If we can get something in mesa to work that is backward compatible
>>>>>>> (i.e. the
>>>>>>> changed parser is able to parse both before and after the kernel
>>>>>>> change),
>>>>>>> then it could be considered to a mesa stable and the kernel side
>>>>>>> changed.
>>>>>> Okay, reasonable plan. But the ascii85 encoder with \n will not be
>>>>>> brought back right?
>>>>> maybe let's agree on how to possibly bring it back? I suggested using a
>>>>> space as continuation line char. This way you can just check the last
>>>>> char
>>>>> returned by getline() you are calling and see if you can go ahead and
>>>>> proceed or if you still need to get more data. Neither space nor newline
>>>>> are part of the ascii85 character set, so it's safe and you can handle
>>>>> continuation in one place in your loop.
>>>>>
>>>>> if you are just ignoring any ascii85, then I believe it's even simpler:
>>>>> you check sections and keys with a space since both keys and section
>>>>> titles contain space, which is not part of the ascii85 char set.
>>>>>
>>>>> Lucas De Marchi
>>>> Yes, I would strongly prefer to use line wrapped ASCII85 data for all
>>>> blobs in the devcoredump. Including things like batch buffers and other
>>>> VM entries that the mesa tool is presumably wanting to decode.
>>>>
>>>> If adding a <space> character to the end of each line is an acceptable
>>>> fix then I have no problems with that. But not line wrapping at all
>>>> means having to carry that change as a non-upstream patch in either the
>>>> internal tree or in individual developer's local trees. Either that or
>>>> we just cannot debug a lot of hard to repro problems.
>>> Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?
>> Apparently not. Even when not deliberately line wrapping the output, I
>> am still seeing it being wrapped when dumping very large buffers such as
>> the GuC log. It looks like something in a lower layer is also forcing
>> line wrapping of super long lines. So either we can't add full size GuC
>> logs to the devcoredump or we need to support line wrapped data in the
>> mesa tool.
> It is probably some implementation detail of __drm_printfn_coredump(). That could be replaced to something like i915 error dump write functions to not
> have any limit at least when the target is a file descriptor, for dmesg you are free to add any line wrapping.
Rather than re-writing the entire drm printer infrastructure, could we 
not just update the mesa decoder tool?

Note that the existing VM dumps are using the same drm_puts printer 
function. So if a very large user buffer was included in the dump, it 
seems like that would also break the mesa parser. And that is not 
something under the control of the KMD.

John.

>
>> John.
>>
>>>> John.
>>>>

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Souza, Jose]

On Fri, 2024-12-13 at 12:26 -0800, John Harrison wrote:
> On 12/13/2024 11:43, Souza, Jose wrote:
> > On Fri, 2024-12-13 at 09:38 -0800, John Harrison wrote:
> > > On 12/13/2024 09:25, Souza, Jose wrote:
> > > > On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
> > > > > On 12/13/2024 08:48, Lucas De Marchi wrote:
> > > > > > On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
> > > > > > > On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
> > > > > > > > On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
> > > > > > > > > On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
> > > > > > > > > > On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
> > > > > > > > > > > On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
> > > > > > > > > > > > We do not break userspace.
> > > > > > > > > > There is other patch that also breaks Mesa parser:
> > > > > > > > > > 
> > > > > > > > > > drm/xe/devcoredump: Improve section headings and add tile info
> > > > > > > > > > 
> > > > > > > > > > > > This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
> > > > > > > > > > > But we have users calling this function.... the revert is not
> > > > > > > > so simple.
> > > > > > > > > > > I think we need to revert the functionality rather than
> > > > > > > > reverting all
> > > > > > > > > > > the patches, otherwise it will cause a lot of headaches.
> > > > > > > > > > > 
> > > > > > > > > > > I propose we go with:
> > > > > > > > > > > 
> > > > > > > > > > > a) drop the \n that broke mesa and merge that with cc stable.
> > > > > > > > > > > 
> > > > > > > > > > > b) move back the entry to the previous section that broke mesa
> > > > > > > > and cc
> > > > > > > > > > >        stable.
> > > > > > > > > > > 
> > > > > > > > > > >        José, would it be ok to merge a patch in mesa and port that
> > > > > > > > > > >        to mesa stable that simply looks at 2 possible sections?
> > > > > > > > Or even
> > > > > > > > > > >        drop the section checks... ?
> > > > > > > > > But if Xe KMD is reverting the patch that changed the hwctx
> > > > > > > > section why would Mesa need to also parse the new(future to be
> > > > > > > > reverted) section?
> > > > > > > > 
> > > > > > > > first is to undo the damage, with 0 changes in mesa. We do that
> > > > > > > > first and
> > > > > > > > *then* we agree on what's possible to do to accomodate the 2 parsers we
> > > > > > > > have.
> > > > > > > > 
> > > > > > > > If we can get something in mesa to work that is backward compatible
> > > > > > > > (i.e. the
> > > > > > > > changed parser is able to parse both before and after the kernel
> > > > > > > > change),
> > > > > > > > then it could be considered to a mesa stable and the kernel side
> > > > > > > > changed.
> > > > > > > Okay, reasonable plan. But the ascii85 encoder with \n will not be
> > > > > > > brought back right?
> > > > > > maybe let's agree on how to possibly bring it back? I suggested using a
> > > > > > space as continuation line char. This way you can just check the last
> > > > > > char
> > > > > > returned by getline() you are calling and see if you can go ahead and
> > > > > > proceed or if you still need to get more data. Neither space nor newline
> > > > > > are part of the ascii85 character set, so it's safe and you can handle
> > > > > > continuation in one place in your loop.
> > > > > > 
> > > > > > if you are just ignoring any ascii85, then I believe it's even simpler:
> > > > > > you check sections and keys with a space since both keys and section
> > > > > > titles contain space, which is not part of the ascii85 char set.
> > > > > > 
> > > > > > Lucas De Marchi
> > > > > Yes, I would strongly prefer to use line wrapped ASCII85 data for all
> > > > > blobs in the devcoredump. Including things like batch buffers and other
> > > > > VM entries that the mesa tool is presumably wanting to decode.
> > > > > 
> > > > > If adding a <space> character to the end of each line is an acceptable
> > > > > fix then I have no problems with that. But not line wrapping at all
> > > > > means having to carry that change as a non-upstream patch in either the
> > > > > internal tree or in individual developer's local trees. Either that or
> > > > > we just cannot debug a lot of hard to repro problems.
> > > > Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?
> > > Apparently not. Even when not deliberately line wrapping the output, I
> > > am still seeing it being wrapped when dumping very large buffers such as
> > > the GuC log. It looks like something in a lower layer is also forcing
> > > line wrapping of super long lines. So either we can't add full size GuC
> > > logs to the devcoredump or we need to support line wrapped data in the
> > > mesa tool.
> > It is probably some implementation detail of __drm_printfn_coredump(). That could be replaced to something like i915 error dump write functions to not
> > have any limit at least when the target is a file descriptor, for dmesg you are free to add any line wrapping.
> Rather than re-writing the entire drm printer infrastructure, could we 
> not just update the mesa decoder tool?
> 
> Note that the existing VM dumps are using the same drm_puts printer 
> function. So if a very large user buffer was included in the dump, it 
> seems like that would also break the mesa parser. And that is not 
> something under the control of the KMD.

Both to me looks like a Xe KMD issue.

A easier option is write a drm_print_ascii85_blob(drm_printer *p, u8 *data, size_t len) that directly writes to drm_print_iterator.data.

> 
> John.
> 
> > 
> > > John.
> > > 
> > > > > John.
> > > > > 
> 

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Lucas De Marchi]

On Fri, Dec 13, 2024 at 12:26:23PM -0800, John Harrison wrote:
>On 12/13/2024 11:43, Souza, Jose wrote:
>>On Fri, 2024-12-13 at 09:38 -0800, John Harrison wrote:
>>>On 12/13/2024 09:25, Souza, Jose wrote:
>>>>On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
>>>>>On 12/13/2024 08:48, Lucas De Marchi wrote:
>>>>>>On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>>>>>>>On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>>>>>>>>On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>>>>>>>>>On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>>>>>>>>>>On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>>>>>>>>>>>On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>>>>>>>>>>>>We do not break userspace.
>>>>>>>>>>There is other patch that also breaks Mesa parser:
>>>>>>>>>>
>>>>>>>>>>drm/xe/devcoredump: Improve section headings and add tile info
>>>>>>>>>>
>>>>>>>>>>>>This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>>>>>>>>>>But we have users calling this function.... the revert is not
>>>>>>>>so simple.
>>>>>>>>>>>I think we need to revert the functionality rather than
>>>>>>>>reverting all
>>>>>>>>>>>the patches, otherwise it will cause a lot of headaches.
>>>>>>>>>>>
>>>>>>>>>>>I propose we go with:
>>>>>>>>>>>
>>>>>>>>>>>a) drop the \n that broke mesa and merge that with cc stable.
>>>>>>>>>>>
>>>>>>>>>>>b) move back the entry to the previous section that broke mesa
>>>>>>>>and cc
>>>>>>>>>>>       stable.
>>>>>>>>>>>
>>>>>>>>>>>       José, would it be ok to merge a patch in mesa and port that
>>>>>>>>>>>       to mesa stable that simply looks at 2 possible sections?
>>>>>>>>Or even
>>>>>>>>>>>       drop the section checks... ?
>>>>>>>>>But if Xe KMD is reverting the patch that changed the hwctx
>>>>>>>>section why would Mesa need to also parse the new(future to be
>>>>>>>>reverted) section?
>>>>>>>>
>>>>>>>>first is to undo the damage, with 0 changes in mesa. We do that
>>>>>>>>first and
>>>>>>>>*then* we agree on what's possible to do to accomodate the 2 parsers we
>>>>>>>>have.
>>>>>>>>
>>>>>>>>If we can get something in mesa to work that is backward compatible
>>>>>>>>(i.e. the
>>>>>>>>changed parser is able to parse both before and after the kernel
>>>>>>>>change),
>>>>>>>>then it could be considered to a mesa stable and the kernel side
>>>>>>>>changed.
>>>>>>>Okay, reasonable plan. But the ascii85 encoder with \n will not be
>>>>>>>brought back right?
>>>>>>maybe let's agree on how to possibly bring it back? I suggested using a
>>>>>>space as continuation line char. This way you can just check the last
>>>>>>char
>>>>>>returned by getline() you are calling and see if you can go ahead and
>>>>>>proceed or if you still need to get more data. Neither space nor newline
>>>>>>are part of the ascii85 character set, so it's safe and you can handle
>>>>>>continuation in one place in your loop.
>>>>>>
>>>>>>if you are just ignoring any ascii85, then I believe it's even simpler:
>>>>>>you check sections and keys with a space since both keys and section
>>>>>>titles contain space, which is not part of the ascii85 char set.
>>>>>>
>>>>>>Lucas De Marchi
>>>>>Yes, I would strongly prefer to use line wrapped ASCII85 data for all
>>>>>blobs in the devcoredump. Including things like batch buffers and other
>>>>>VM entries that the mesa tool is presumably wanting to decode.
>>>>>
>>>>>If adding a <space> character to the end of each line is an acceptable
>>>>>fix then I have no problems with that. But not line wrapping at all
>>>>>means having to carry that change as a non-upstream patch in either the
>>>>>internal tree or in individual developer's local trees. Either that or
>>>>>we just cannot debug a lot of hard to repro problems.
>>>>Can't Xe KMD use the line wrapped version of ASCII85 when printing to dmesg and keep the regular encoder when devcoredump file is read?
>>>Apparently not. Even when not deliberately line wrapping the output, I
>>>am still seeing it being wrapped when dumping very large buffers such as
>>>the GuC log. It looks like something in a lower layer is also forcing
>>>line wrapping of super long lines. So either we can't add full size GuC
>>>logs to the devcoredump or we need to support line wrapped data in the
>>>mesa tool.
>>It is probably some implementation detail of __drm_printfn_coredump(). That could be replaced to something like i915 error dump write functions to not

it doesn't look like it as there's nothing there looping through chunks.
And particularly nothing that would really add a '\n', corrupting
the output.

>>have any limit at least when the target is a file descriptor, for dmesg you are free to add any line wrapping.
>Rather than re-writing the entire drm printer infrastructure, could we 
>not just update the mesa decoder tool?

I think it would be great to add line continuation there and document
so it's more future proof and we don't break it again because of this.

>
>Note that the existing VM dumps are using the same drm_puts printer 

would be good to know where that is coming from. I don't see anything in
drm_puts() that would add newlines.... it looks more like how we are
calling xe_print_blob_ascii85() in a loop, but that's strange since we
are actually passing 2M chunks. What did you use to test?

Lucas De Marchi

>function. So if a very large user buffer was included in the dump, it 
>seems like that would also break the mesa parser. And that is not 
>something under the control of the KMD.
>
>John.
>
>>
>>>John.
>>>
>>>>>John.
>>>>>
>

Re: [PATCH] Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[John Harrison]

On 12/13/2024 13:39, Lucas De Marchi wrote:
> On Fri, Dec 13, 2024 at 12:26:23PM -0800, John Harrison wrote:
>> On 12/13/2024 11:43, Souza, Jose wrote:
>>> On Fri, 2024-12-13 at 09:38 -0800, John Harrison wrote:
>>>> On 12/13/2024 09:25, Souza, Jose wrote:
>>>>> On Fri, 2024-12-13 at 08:56 -0800, John Harrison wrote:
>>>>>> On 12/13/2024 08:48, Lucas De Marchi wrote:
>>>>>>> On Fri, Dec 13, 2024 at 04:28:58PM +0000, Jose Souza wrote:
>>>>>>>> On Fri, 2024-12-13 at 09:50 -0600, Lucas De Marchi wrote:
>>>>>>>>> On Fri, Dec 13, 2024 at 03:24:59PM +0000, Jose Souza wrote:
>>>>>>>>>> On Fri, 2024-12-13 at 07:10 -0800, José Roberto de Souza wrote:
>>>>>>>>>>> On Fri, 2024-12-13 at 08:38 -0600, Lucas De Marchi wrote:
>>>>>>>>>>>> On Fri, Dec 13, 2024 at 09:12:52AM -0500, Rodrigo Vivi wrote:
>>>>>>>>>>>>> We do not break userspace.
>>>>>>>>>>> There is other patch that also breaks Mesa parser:
>>>>>>>>>>>
>>>>>>>>>>> drm/xe/devcoredump: Improve section headings and add tile info
>>>>>>>>>>>
>>>>>>>>>>>>> This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
>>>>>>>>>>>> But we have users calling this function.... the revert is not
>>>>>>>>> so simple.
>>>>>>>>>>>> I think we need to revert the functionality rather than
>>>>>>>>> reverting all
>>>>>>>>>>>> the patches, otherwise it will cause a lot of headaches.
>>>>>>>>>>>>
>>>>>>>>>>>> I propose we go with:
>>>>>>>>>>>>
>>>>>>>>>>>> a) drop the \n that broke mesa and merge that with cc stable.
>>>>>>>>>>>>
>>>>>>>>>>>> b) move back the entry to the previous section that broke mesa
>>>>>>>>> and cc
>>>>>>>>>>>>       stable.
>>>>>>>>>>>>
>>>>>>>>>>>>       José, would it be ok to merge a patch in mesa and 
>>>>>>>>>>>> port that
>>>>>>>>>>>>       to mesa stable that simply looks at 2 possible sections?
>>>>>>>>> Or even
>>>>>>>>>>>>       drop the section checks... ?
>>>>>>>>>> But if Xe KMD is reverting the patch that changed the hwctx
>>>>>>>>> section why would Mesa need to also parse the new(future to be
>>>>>>>>> reverted) section?
>>>>>>>>>
>>>>>>>>> first is to undo the damage, with 0 changes in mesa. We do that
>>>>>>>>> first and
>>>>>>>>> *then* we agree on what's possible to do to accomodate the 2 
>>>>>>>>> parsers we
>>>>>>>>> have.
>>>>>>>>>
>>>>>>>>> If we can get something in mesa to work that is backward 
>>>>>>>>> compatible
>>>>>>>>> (i.e. the
>>>>>>>>> changed parser is able to parse both before and after the kernel
>>>>>>>>> change),
>>>>>>>>> then it could be considered to a mesa stable and the kernel side
>>>>>>>>> changed.
>>>>>>>> Okay, reasonable plan. But the ascii85 encoder with \n will not be
>>>>>>>> brought back right?
>>>>>>> maybe let's agree on how to possibly bring it back? I suggested 
>>>>>>> using a
>>>>>>> space as continuation line char. This way you can just check the 
>>>>>>> last
>>>>>>> char
>>>>>>> returned by getline() you are calling and see if you can go 
>>>>>>> ahead and
>>>>>>> proceed or if you still need to get more data. Neither space nor 
>>>>>>> newline
>>>>>>> are part of the ascii85 character set, so it's safe and you can 
>>>>>>> handle
>>>>>>> continuation in one place in your loop.
>>>>>>>
>>>>>>> if you are just ignoring any ascii85, then I believe it's even 
>>>>>>> simpler:
>>>>>>> you check sections and keys with a space since both keys and 
>>>>>>> section
>>>>>>> titles contain space, which is not part of the ascii85 char set.
>>>>>>>
>>>>>>> Lucas De Marchi
>>>>>> Yes, I would strongly prefer to use line wrapped ASCII85 data for 
>>>>>> all
>>>>>> blobs in the devcoredump. Including things like batch buffers and 
>>>>>> other
>>>>>> VM entries that the mesa tool is presumably wanting to decode.
>>>>>>
>>>>>> If adding a <space> character to the end of each line is an 
>>>>>> acceptable
>>>>>> fix then I have no problems with that. But not line wrapping at all
>>>>>> means having to carry that change as a non-upstream patch in 
>>>>>> either the
>>>>>> internal tree or in individual developer's local trees. Either 
>>>>>> that or
>>>>>> we just cannot debug a lot of hard to repro problems.
>>>>> Can't Xe KMD use the line wrapped version of ASCII85 when printing 
>>>>> to dmesg and keep the regular encoder when devcoredump file is read?
>>>> Apparently not. Even when not deliberately line wrapping the output, I
>>>> am still seeing it being wrapped when dumping very large buffers 
>>>> such as
>>>> the GuC log. It looks like something in a lower layer is also forcing
>>>> line wrapping of super long lines. So either we can't add full size 
>>>> GuC
>>>> logs to the devcoredump or we need to support line wrapped data in the
>>>> mesa tool.
>>> It is probably some implementation detail of 
>>> __drm_printfn_coredump(). That could be replaced to something like 
>>> i915 error dump write functions to not
>
> it doesn't look like it as there's nothing there looping through chunks.
> And particularly nothing that would really add a '\n', corrupting
> the output.
>
>>> have any limit at least when the target is a file descriptor, for 
>>> dmesg you are free to add any line wrapping.
>> Rather than re-writing the entire drm printer infrastructure, could 
>> we not just update the mesa decoder tool?
>
> I think it would be great to add line continuation there and document
> so it's more future proof and we don't break it again because of this.
>
>>
>> Note that the existing VM dumps are using the same drm_puts printer 
>
> would be good to know where that is coming from. I don't see anything in
> drm_puts() that would add newlines.... it looks more like how we are
> calling xe_print_blob_ascii85() in a loop, but that's strange since we
Doh! Yes. The GuC log splits the log data into 2MB chunks to avoid the 
kmalloc size limit (of 8MB IIRC). Which means it would either have to 
dump each chunk as a separately tagged devcoredump field/data pair or 
the final linefeed would have to be added manually by the caller and not 
automatically by the helper. Neither option is particular nice.

> are actually passing 2M chunks. What did you use to test?
I've been mostly using igt@xe_exec_reset@cat-error with a modified IGT 
to save out each coredump rather than splatting it. We don't actually 
have any devcoredump tests at the moment. Zhanjun has been writing one. 
Apparently Maarten Lankhorst had also started writing one but it never 
got anywhere?

It would be useful to also add the mesa decoder tool itself as test for 
CI too. Otherwise it may still be making assumptions that we do not 
realise and are not testing for in an IGT. E.g. the thing about section 
headers.

John.

>
> Lucas De Marchi
>
>> function. So if a very large user buffer was included in the dump, it 
>> seems like that would also break the mesa parser. And that is not 
>> something under the control of the KMD.
>>
>> John.
>>
>>>
>>>> John.
>>>>
>>>>>> John.
>>>>>>
>>

✓ CI.Patch_applied: success for Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Patchwork]

== Series Details ==

Series: Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"
URL   : https://patchwork.freedesktop.org/series/142569/
State : success

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: fb3d98511a2f drm-tip: 2024y-12m-13d-15h-56m-25s UTC integration manifest
=== git am output follows ===
Applying: Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

✗ CI.checkpatch: warning for Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Patchwork]

== Series Details ==

Series: Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"
URL   : https://patchwork.freedesktop.org/series/142569/
State : warning

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
30ab6715fc09baee6cc14cb3c89ad8858688d474
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit 403dad10f30f593757705583745d54f3806e1fdb
Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
Date:   Fri Dec 13 09:12:52 2024 -0500

    Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"
    
    We do not break userspace.
    
    This reverts commit ec1455ce7e35a31289d2dbc1070b980538698921.
    
    Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper
    function")
    Cc: John Harrison <John.C.Harrison@Intel.com>
    Cc: Julia Filipchuk <julia.filipchuk@intel.com>
    Cc: José Roberto de Souza <jose.souza@intel.com>
    Cc: Lucas De Marchi <lucas.demarchi@intel.com>
    Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
    Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
+ /mt/dim checkpatch fb3d98511a2f6375a5c989ca5520aa31b689b119 drm-intel
403dad10f30f Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"
-:13: WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper function")'
#13: 
Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper

total: 0 errors, 1 warnings, 0 checks, 26 lines checked

✗ CI.KUnit: failure for Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"

[Patchwork]

== Series Details ==

Series: Revert "drm/xe/devcoredump: Add ASCII85 dump helper function"
URL   : https://patchwork.freedesktop.org/series/142569/
State : failure

== Summary ==

+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
[16:59:09] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[16:59:14] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json ARCH=um O=.kunit --jobs=48
ERROR:root:../drivers/gpu/drm/xe/xe_guc_ct.c: In function ‘xe_guc_ct_snapshot_print’:
../drivers/gpu/drm/xe/xe_guc_ct.c:1727:25: error: implicit declaration of function ‘xe_print_blob_ascii85’ [-Werror=implicit-function-declaration]
 1727 |                         xe_print_blob_ascii85(p, "CTB data", snapshot->ctb, 0, snapshot->ctb_size);
      |                         ^~~~~~~~~~~~~~~~~~~~~
../drivers/gpu/drm/xe/xe_guc_log.c: In function ‘xe_guc_log_snapshot_print’:
../drivers/gpu/drm/xe/xe_guc_log.c:215:17: error: implicit declaration of function ‘xe_print_blob_ascii85’ [-Werror=implicit-function-declaration]
  215 |                 xe_print_blob_ascii85(p, i ? NULL : "Log data", snapshot->copy[i], 0, size);
      |                 ^~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[7]: *** [../scripts/Makefile.build:194: drivers/gpu/drm/xe/xe_guc_log.o] Error 1
make[7]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[7]: *** [../scripts/Makefile.build:194: drivers/gpu/drm/xe/xe_guc_ct.o] Error 1
../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
make[6]: *** [../scripts/Makefile.build:440: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:440: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:440: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:440: drivers] Error 2
make[2]: *** [/kernel/Makefile:1989: .] Error 2
make[1]: *** [/kernel/Makefile:251: __sub-make] Error 2
make: *** [Makefile:251: __sub-make] Error 2

+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel