Re: [PATCH] drm/i915: Skip logging impossible slices

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Jani Nikula (2018-03-21 11:47:06)
>> 
>> On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > Quoting Chris Wilson (2018-03-21 10:41:37)
>> >> Quoting Tvrtko Ursulin (2018-03-21 10:32:28)
>> >> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >> > 
>> >> > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid
>> >> > printing impossible and empty slices for a platform.
>> >> > 
>> >> > Also compact slice total and slice mask into one log line.
>> >> > 
>> >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/intel_device_info.c | 8 ++++----
>> >> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >> > 
>> >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>> >> > index 4babfc6ee45b..68aa9746d0e1 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_device_info.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> >> > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p)
>> >> >  {
>> >> >         int s;
>> >> >  
>> >> > -       drm_printf(p, "slice mask: %04x\n", sseu->slice_mask);
>> >> > -       drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask));
>> >> > +       drm_printf(p, "slice total: %u, mask=%04x\n",
>> >> > +                  hweight8(sseu->slice_mask), sseu->slice_mask);
>> >> >         drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu));
>> >> > -       for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) {
>> >> > -               drm_printf(p, "slice%d %u subslices mask=%04x\n",
>> >> > +       for (s = 0; s < sseu->max_slices; s++) {
>> >> > +               drm_printf(p, "slice%d: %u subslices, mask=%04x\n",
>> >> >                            s, hweight8(sseu->subslice_mask[s]),
>> >> >                            sseu->subslice_mask[s]);
>> >> 
>> >> Just idly testing the waters...
>> >> 
>> >> In yaml, this would be
>> >>   "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n"
>> >
>> > Or if we keep the node name the same for easier parsing:
>> >
>> >       "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n"
>> 
>> I'm not against doing this, especially for gpu dumps.
>> 
>> Wouldn't json be easier to generate and parse? Or do you prefer the
>> slightly better human readability of yaml?
>
> I think for any of the debug output preferring to keep it as readable as
> possible is essential. libyaml isn't that hard to use, even for a
> beginner like myself.

Fair enough; I'm only familiar with json, and that's my natural reason
to lean towards it.

>> I think it would be pretty straighforward to write drm printer helpers
>> for printing valid json without having to actually manually print the
>> colons and braces etc. And the struct drm_printer could even have checks
>> for ensuring you don't burp verbatim stuff to a printer that's supposed
>> to be json.
>
> About the biggest challenge is tracking indent; which drm_printer
> already does iirc. Still, I think we want to move this into lib/

I agree having this in lib is preferrable. But apparently that requires
moving the whole abstracted drm_printer style printing there first, and
then hoping to get yaml/json added on top. There's bound to be some
friction there.

>> Any considerations for the transition? Massive wholesale patch bomb
>> conversion? Yikes.
>
> I think it's only worth converting bits and pieces that we are trying to
> parse. So quite a few debugfs are candidates, and the error-state being
> a prime example as we want to make it more amenable and flexible for
> future post-mortem capture depending on what userspace needs. (I might
> even go as far as all future debugfs should come with a parser for igt.)

Oh, I did mean mass conversion on a per-debugfs-file basis, i.e. error
state in one go. But I guess there's no way around that, anything else
is just pain spread over a longer period.

I'd definitely go as far as saying any debugfs file that's supposed to
be in a serialized format should have a corresponding parser in igt, and
written *before* merging the kernel change.

There's also some discipline required wrt backward/forward compatibility
to actually reap the benefits of the format. Can't just change the
contents nilly willy with that either. It's not a magic bullet.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx