Re: [PATCH] drm/xe: Sort again the info flags

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

On Tue, 19 Nov 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Nov 19, 2024 at 10:51:58AM +0000, Matthew Auld wrote:
>>On 18/11/2024 22:33, Lucas De Marchi wrote:
>>>Those flags are supposed to be kept sorted alphabetically. Unfortunately
>>>it's a constant battle as new flags are added to the end or at random
>>>places. Sort it again.
>>>
>>>v2: Include the other non-has_* 1-bit flags in the sort
>>>
>>>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> # v1
>>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>>---
>>>  drivers/gpu/drm/xe/xe_device_types.h | 32 ++++++++++++++++------------
>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>>index 8592f1b02db11..8b2b12daa49dd 100644
>>>--- a/drivers/gpu/drm/xe/xe_device_types.h
>>>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>>>@@ -294,14 +294,21 @@ struct xe_device {
>>>  		/** @info.va_bits: Maximum bits of a virtual address */
>>>  		u8 va_bits;
>>
>>Should we not document somewhere here that the below should be kept sorted?
>
> yes, probably. Also... we have the CI Hooks we should make better use of
> too. First check if the patch series changed drivers/gpu/drm/xe/xe_device_types.h
> at all and if it did, check that these flags are sorted. We should be
> able to add some quick and dirty checks so we don't have to keep
> changing it. Just need to make sure the output on what was
> the error is helpful, otherwise people will just ignore it.
>
> Ryszard / Jani  any thought on that idea?

Some random thoughts:

We need more transparency on all of CI. I still have no clue of all the
things that get run, where it's all hosted, how I could contribute
changes to it. I'm guessing maybe it's all out there somewhere, but I
find it difficult to figure out. A check like this would need to be
discoverable.

Something like this should be locally runnable. I know it's been hard to
get people to run even checkpatch or sparse, but it's just dumb noise to
get the first indication something's wrong in a CI result. And if you
don't block further runs based on this, it just burns resources.

On that last point, I think we should aim for reliable pass/fail, with
fast fail halting further resource intensive stuff. E.g. checkpatch is
not like this, we can't block on checkpatch failures, and due to this
the results tend to be ignored and we merge stuff with checkpatch
failures. Ditto with detailed checks like this, if it doesn't block
merging, it's bound to get ignored at times. (Side note, maybe we should
look into a limited checkpatch config that we could fail fast on?)

BR,
Jani.


-- 
Jani Nikula, Intel