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