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

["Souza, Jose" <jose.souza@intel.com>]

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