mbox series

[v2,0/9] drm/i915/display: platform identification with display->is.<PLATFORM>

Message ID cover.1724092799.git.jani.nikula@intel.com (mailing list archive)
Headers show
Series drm/i915/display: platform identification with display->is.<PLATFORM> | expand

Message

Jani Nikula Aug. 19, 2024, 6:44 p.m. UTC
v2 of [1]. Please read the cover letter there.

This addresses review comments and adds a few more commits on top, in particular
the last one showcasing the approach.

The main question remains, is this what we want?

BR,
Jani.

[1] https://lore.kernel.org/r/cover.1718719962.git.jani.nikula@intel.com


Jani Nikula (9):
  drm/i915/display: use a macro to initialize subplatforms
  drm/i915/display: use a macro to define platform enumerations
  drm/i915/display: join the platform and subplatform enums
  drm/i915/display: add display platforms structure with platform
    members
  drm/i915/display: add platforms "is" member to struct intel_display
  drm/i915/display: remove the display platform enum as unnecessary
  drm/i915/display: consider HSW/BDW ULX to also be ULT
  drm/i915/display: keep subplatforms next to their platforms
  drm/i915/bios: use display->is.PLATFORM instead of IS_PLATFORM()

 drivers/gpu/drm/i915/display/intel_bios.c     |  36 ++--
 .../gpu/drm/i915/display/intel_display_core.h |   3 +
 .../drm/i915/display/intel_display_device.c   |  89 ++++++---
 .../drm/i915/display/intel_display_device.h   | 180 ++++++++++--------
 4 files changed, 174 insertions(+), 134 deletions(-)

Comments

Rodrigo Vivi Aug. 28, 2024, 8:41 p.m. UTC | #1
On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> v2 of [1]. Please read the cover letter there.
> 
> This addresses review comments and adds a few more commits on top, in particular
> the last one showcasing the approach.
> 
> The main question remains, is this what we want?

I don't know why, but the 'is' thing is still strange.

I know I know... I'm bad with naming myself.

I think about 'platform' but that get too big

if (display->platform.BROADWELL)

I think about 'gen' but then it is overloaded....

then I think about 'ip' is worse...

'version'?

'name'?

if (display->name.HASWELL)...

....

But well, I like the overall simplification here in general.
Without a better name to suggest, I guess let's just move ahead...

> 
> BR,
> Jani.
> 
> [1] https://lore.kernel.org/r/cover.1718719962.git.jani.nikula@intel.com
> 
> 
> Jani Nikula (9):
>   drm/i915/display: use a macro to initialize subplatforms
>   drm/i915/display: use a macro to define platform enumerations
>   drm/i915/display: join the platform and subplatform enums
>   drm/i915/display: add display platforms structure with platform
>     members
>   drm/i915/display: add platforms "is" member to struct intel_display
>   drm/i915/display: remove the display platform enum as unnecessary
>   drm/i915/display: consider HSW/BDW ULX to also be ULT
>   drm/i915/display: keep subplatforms next to their platforms
>   drm/i915/bios: use display->is.PLATFORM instead of IS_PLATFORM()
> 
>  drivers/gpu/drm/i915/display/intel_bios.c     |  36 ++--
>  .../gpu/drm/i915/display/intel_display_core.h |   3 +
>  .../drm/i915/display/intel_display_device.c   |  89 ++++++---
>  .../drm/i915/display/intel_display_device.h   | 180 ++++++++++--------
>  4 files changed, 174 insertions(+), 134 deletions(-)
> 
> -- 
> 2.39.2
>
Ville Syrjala Aug. 29, 2024, 4:03 p.m. UTC | #2
On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> > v2 of [1]. Please read the cover letter there.
> > 
> > This addresses review comments and adds a few more commits on top, in particular
> > the last one showcasing the approach.
> > 
> > The main question remains, is this what we want?
> 
> I don't know why, but the 'is' thing is still strange.
> 
> I know I know... I'm bad with naming myself.
> 
> I think about 'platform' but that get too big
> 
> if (display->platform.BROADWELL)
> 
> I think about 'gen' but then it is overloaded....
> 
> then I think about 'ip' is worse...
> 
> 'version'?
> 
> 'name'?
> 
> if (display->name.HASWELL)...
> 
> ....
> 
> But well, I like the overall simplification here in general.
> Without a better name to suggest, I guess let's just move ahead...

One slight concern with the is.foo is whether it complicates finding
things with eg. cscope. But I suppose for platforms that doesn't matter
all that much. For the has_foo stuff it'd be much more relevant.

Anyways, can't think of anything particularly elegant myself either,
so go ahead I guess.

> 
> > 
> > BR,
> > Jani.
> > 
> > [1] https://lore.kernel.org/r/cover.1718719962.git.jani.nikula@intel.com
> > 
> > 
> > Jani Nikula (9):
> >   drm/i915/display: use a macro to initialize subplatforms
> >   drm/i915/display: use a macro to define platform enumerations
> >   drm/i915/display: join the platform and subplatform enums
> >   drm/i915/display: add display platforms structure with platform
> >     members
> >   drm/i915/display: add platforms "is" member to struct intel_display
> >   drm/i915/display: remove the display platform enum as unnecessary
> >   drm/i915/display: consider HSW/BDW ULX to also be ULT
> >   drm/i915/display: keep subplatforms next to their platforms
> >   drm/i915/bios: use display->is.PLATFORM instead of IS_PLATFORM()
> > 
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  36 ++--
> >  .../gpu/drm/i915/display/intel_display_core.h |   3 +
> >  .../drm/i915/display/intel_display_device.c   |  89 ++++++---
> >  .../drm/i915/display/intel_display_device.h   | 180 ++++++++++--------
> >  4 files changed, 174 insertions(+), 134 deletions(-)
> > 
> > -- 
> > 2.39.2
> >
Jani Nikula Sept. 24, 2024, 9:49 a.m. UTC | #3
On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
>> > v2 of [1]. Please read the cover letter there.
>> > 
>> > This addresses review comments and adds a few more commits on top, in particular
>> > the last one showcasing the approach.
>> > 
>> > The main question remains, is this what we want?
>> 
>> I don't know why, but the 'is' thing is still strange.
>> 
>> I know I know... I'm bad with naming myself.
>> 
>> I think about 'platform' but that get too big
>> 
>> if (display->platform.BROADWELL)
>> 
>> I think about 'gen' but then it is overloaded....
>> 
>> then I think about 'ip' is worse...
>> 
>> 'version'?
>> 
>> 'name'?
>> 
>> if (display->name.HASWELL)...
>> 
>> ....
>> 
>> But well, I like the overall simplification here in general.
>> Without a better name to suggest, I guess let's just move ahead...
>
> One slight concern with the is.foo is whether it complicates finding
> things with eg. cscope. But I suppose for platforms that doesn't matter
> all that much. For the has_foo stuff it'd be much more relevant.

It does make finding things harder with cscope and gnu global, but git
grep for is.FOO is pretty accurate.

> Anyways, can't think of anything particularly elegant myself either,
> so go ahead I guess.

So I haven't yet. I just still have that slightly uneasy feeling about
whether this is a good thing or not. That doesn't usually make me shy
away from things, because you can fix stuff later, but getting this
wrong causes so much churn everywhere.

The fact that it's not a macro makes it less flexible for future
changes. The display->is.FOO is somewhat legible, but could be
better. Would all lowercase make it better? I don't know.

More alternatives? Not elegant for sure, but just alternatives:

- Lowercase names:

	if (display->is.rocketlake)

  Does not help with flexibility or cscope.

- Lowercase macros for display, e.g. is_rocketlake().

	if (is_rocketlake(display))

- Macros based on just the platform name, e.g. ROCKETLAKE().

	if (ROCKETLAKE(display))

  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().

	if (PLATFORM_ROCKETLAKE(display))

  But that can get a bit long in some if ladders etc.

- Go through the trouble of making the existing IS_FOO() macros _Generic
  and accept either i915 or display pointer. This does postpone making
  any further changes, but fairly soon there will need to be two sets of
  macros, separate for i915 and display, even though named the same.

  Also, the _Generic thing would look up the platform definitions from
  different places, which could be error prone.


Yeah, procrastination...


BR,
Jani.
Lucas De Marchi Sept. 24, 2024, 1:14 p.m. UTC | #4
On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
>>> > v2 of [1]. Please read the cover letter there.
>>> >
>>> > This addresses review comments and adds a few more commits on top, in particular
>>> > the last one showcasing the approach.
>>> >
>>> > The main question remains, is this what we want?
>>>
>>> I don't know why, but the 'is' thing is still strange.
>>>
>>> I know I know... I'm bad with naming myself.
>>>
>>> I think about 'platform' but that get too big
>>>
>>> if (display->platform.BROADWELL)
>>>
>>> I think about 'gen' but then it is overloaded....
>>>
>>> then I think about 'ip' is worse...
>>>
>>> 'version'?
>>>
>>> 'name'?
>>>
>>> if (display->name.HASWELL)...
>>>
>>> ....
>>>
>>> But well, I like the overall simplification here in general.
>>> Without a better name to suggest, I guess let's just move ahead...
>>
>> One slight concern with the is.foo is whether it complicates finding
>> things with eg. cscope. But I suppose for platforms that doesn't matter
>> all that much. For the has_foo stuff it'd be much more relevant.
>
>It does make finding things harder with cscope and gnu global, but git
>grep for is.FOO is pretty accurate.
>
>> Anyways, can't think of anything particularly elegant myself either,
>> so go ahead I guess.
>
>So I haven't yet. I just still have that slightly uneasy feeling about
>whether this is a good thing or not. That doesn't usually make me shy
>away from things, because you can fix stuff later, but getting this
>wrong causes so much churn everywhere.
>
>The fact that it's not a macro makes it less flexible for future
>changes. The display->is.FOO is somewhat legible, but could be
>better. Would all lowercase make it better? I don't know.
>
>More alternatives? Not elegant for sure, but just alternatives:
>
>- Lowercase names:
>
>	if (display->is.rocketlake)

what I really dislike is a struct named "is". Going full mesa-way would
be slightly better IMO:

	if (display->is_rockelake)

or

	if (display->platform_rocketlake)

or

	if (display->platform.rocketlake)

>
>  Does not help with flexibility or cscope.
>
>- Lowercase macros for display, e.g. is_rocketlake().
>
>	if (is_rocketlake(display))
>
>- Macros based on just the platform name, e.g. ROCKETLAKE().
>
>	if (ROCKETLAKE(display))
>
>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
>
>	if (PLATFORM_ROCKETLAKE(display))
>
>  But that can get a bit long in some if ladders etc.

Does it matter much? I think those would be the exception, particularly
because platform checks are kind of rare these days.

grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
because wherever we can we check by graphics/display version rather than
platform.

Then simply using something similar to what we already have in xe, would
be great IMO:

	if (display->platform == PLATFORM_LUNARLAKE)

it may be verbose, but shouldn't be much used to matter in the end.

Lucas De Marchi

>
>- Go through the trouble of making the existing IS_FOO() macros _Generic
>  and accept either i915 or display pointer. This does postpone making
>  any further changes, but fairly soon there will need to be two sets of
>  macros, separate for i915 and display, even though named the same.
>
>  Also, the _Generic thing would look up the platform definitions from
>  different places, which could be error prone.
>
>
>Yeah, procrastination...
>
>
>BR,
>Jani.
>
>
>
>
>-- 
>Jani Nikula, Intel
Jani Nikula Sept. 24, 2024, 1:37 p.m. UTC | #5
On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
>>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
>>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
>>>> > v2 of [1]. Please read the cover letter there.
>>>> >
>>>> > This addresses review comments and adds a few more commits on top, in particular
>>>> > the last one showcasing the approach.
>>>> >
>>>> > The main question remains, is this what we want?
>>>>
>>>> I don't know why, but the 'is' thing is still strange.
>>>>
>>>> I know I know... I'm bad with naming myself.
>>>>
>>>> I think about 'platform' but that get too big
>>>>
>>>> if (display->platform.BROADWELL)
>>>>
>>>> I think about 'gen' but then it is overloaded....
>>>>
>>>> then I think about 'ip' is worse...
>>>>
>>>> 'version'?
>>>>
>>>> 'name'?
>>>>
>>>> if (display->name.HASWELL)...
>>>>
>>>> ....
>>>>
>>>> But well, I like the overall simplification here in general.
>>>> Without a better name to suggest, I guess let's just move ahead...
>>>
>>> One slight concern with the is.foo is whether it complicates finding
>>> things with eg. cscope. But I suppose for platforms that doesn't matter
>>> all that much. For the has_foo stuff it'd be much more relevant.
>>
>>It does make finding things harder with cscope and gnu global, but git
>>grep for is.FOO is pretty accurate.
>>
>>> Anyways, can't think of anything particularly elegant myself either,
>>> so go ahead I guess.
>>
>>So I haven't yet. I just still have that slightly uneasy feeling about
>>whether this is a good thing or not. That doesn't usually make me shy
>>away from things, because you can fix stuff later, but getting this
>>wrong causes so much churn everywhere.
>>
>>The fact that it's not a macro makes it less flexible for future
>>changes. The display->is.FOO is somewhat legible, but could be
>>better. Would all lowercase make it better? I don't know.
>>
>>More alternatives? Not elegant for sure, but just alternatives:
>>
>>- Lowercase names:
>>
>>	if (display->is.rocketlake)
>
> what I really dislike is a struct named "is". Going full mesa-way would
> be slightly better IMO:
>
> 	if (display->is_rockelake)
>
> or
>
> 	if (display->platform_rocketlake)
>
> or
>
> 	if (display->platform.rocketlake)

Fair enough.

From implementation POV having a sub-struct is easier than not.

>>
>>  Does not help with flexibility or cscope.
>>
>>- Lowercase macros for display, e.g. is_rocketlake().
>>
>>	if (is_rocketlake(display))
>>
>>- Macros based on just the platform name, e.g. ROCKETLAKE().
>>
>>	if (ROCKETLAKE(display))
>>
>>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
>>
>>	if (PLATFORM_ROCKETLAKE(display))
>>
>>  But that can get a bit long in some if ladders etc.
>
> Does it matter much? I think those would be the exception, particularly
> because platform checks are kind of rare these days.

Well, they're maybe the exception for new platforms, but i915 display
does have to deal with a lot of legacy with a lot of platform checks.

> grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
> because wherever we can we check by graphics/display version rather than
> platform.

i915 display has only one use of IS_LUNARLAKE(), but there are 1k+ other
uses of IS_<PLATFORM>.

Incidentally, this is the reason I'm procrastinating about the change at
all.

> Then simply using something similar to what we already have in xe, would
> be great IMO:
>
> 	if (display->platform == PLATFORM_LUNARLAKE)
>
> it may be verbose, but shouldn't be much used to matter in the end.

The downside with that is that you can't deal with subplatforms as
easily. It becomes

	if (display->platform == PLATFORM_LUNARLAKE ||
	    (display->platform == PLATFORM_ALDERLAKE_P &&
	     display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N))

or similar. Definitely not a fan.


BR,
Jani.


>
> Lucas De Marchi
>
>>
>>- Go through the trouble of making the existing IS_FOO() macros _Generic
>>  and accept either i915 or display pointer. This does postpone making
>>  any further changes, but fairly soon there will need to be two sets of
>>  macros, separate for i915 and display, even though named the same.
>>
>>  Also, the _Generic thing would look up the platform definitions from
>>  different places, which could be error prone.
>>
>>
>>Yeah, procrastination...
>>
>>
>>BR,
>>Jani.
>>
>>
>>
>>
>>-- 
>>Jani Nikula, Intel
Ville Syrjala Sept. 24, 2024, 1:43 p.m. UTC | #6
On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
> >>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> >>>> > v2 of [1]. Please read the cover letter there.
> >>>> >
> >>>> > This addresses review comments and adds a few more commits on top, in particular
> >>>> > the last one showcasing the approach.
> >>>> >
> >>>> > The main question remains, is this what we want?
> >>>>
> >>>> I don't know why, but the 'is' thing is still strange.
> >>>>
> >>>> I know I know... I'm bad with naming myself.
> >>>>
> >>>> I think about 'platform' but that get too big
> >>>>
> >>>> if (display->platform.BROADWELL)
> >>>>
> >>>> I think about 'gen' but then it is overloaded....
> >>>>
> >>>> then I think about 'ip' is worse...
> >>>>
> >>>> 'version'?
> >>>>
> >>>> 'name'?
> >>>>
> >>>> if (display->name.HASWELL)...
> >>>>
> >>>> ....
> >>>>
> >>>> But well, I like the overall simplification here in general.
> >>>> Without a better name to suggest, I guess let's just move ahead...
> >>>
> >>> One slight concern with the is.foo is whether it complicates finding
> >>> things with eg. cscope. But I suppose for platforms that doesn't matter
> >>> all that much. For the has_foo stuff it'd be much more relevant.
> >>
> >>It does make finding things harder with cscope and gnu global, but git
> >>grep for is.FOO is pretty accurate.
> >>
> >>> Anyways, can't think of anything particularly elegant myself either,
> >>> so go ahead I guess.
> >>
> >>So I haven't yet. I just still have that slightly uneasy feeling about
> >>whether this is a good thing or not. That doesn't usually make me shy
> >>away from things, because you can fix stuff later, but getting this
> >>wrong causes so much churn everywhere.
> >>
> >>The fact that it's not a macro makes it less flexible for future
> >>changes. The display->is.FOO is somewhat legible, but could be
> >>better. Would all lowercase make it better? I don't know.
> >>
> >>More alternatives? Not elegant for sure, but just alternatives:
> >>
> >>- Lowercase names:
> >>
> >>	if (display->is.rocketlake)
> >
> > what I really dislike is a struct named "is". Going full mesa-way would
> > be slightly better IMO:
> >
> > 	if (display->is_rockelake)
> >
> > or
> >
> > 	if (display->platform_rocketlake)
> >
> > or
> >
> > 	if (display->platform.rocketlake)
> 
> Fair enough.
> 
> >From implementation POV having a sub-struct is easier than not.
> 
> >>
> >>  Does not help with flexibility or cscope.
> >>
> >>- Lowercase macros for display, e.g. is_rocketlake().
> >>
> >>	if (is_rocketlake(display))
> >>
> >>- Macros based on just the platform name, e.g. ROCKETLAKE().
> >>
> >>	if (ROCKETLAKE(display))
> >>
> >>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
> >>
> >>	if (PLATFORM_ROCKETLAKE(display))
> >>
> >>  But that can get a bit long in some if ladders etc.
> >
> > Does it matter much? I think those would be the exception, particularly
> > because platform checks are kind of rare these days.
> 
> Well, they're maybe the exception for new platforms, but i915 display
> does have to deal with a lot of legacy with a lot of platform checks.

There are a lot of annoying exceptions for new platforms
as well. Feels like most features are doing some kind
of weird leap frogging between integrated vs. discrete
these days. So most new features seem to require an
exception one way or the other :(
Rodrigo Vivi Sept. 26, 2024, 2:58 p.m. UTC | #7
On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
> >>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> >>>> > v2 of [1]. Please read the cover letter there.
> >>>> >
> >>>> > This addresses review comments and adds a few more commits on top, in particular
> >>>> > the last one showcasing the approach.
> >>>> >
> >>>> > The main question remains, is this what we want?
> >>>>
> >>>> I don't know why, but the 'is' thing is still strange.
> >>>>
> >>>> I know I know... I'm bad with naming myself.
> >>>>
> >>>> I think about 'platform' but that get too big
> >>>>
> >>>> if (display->platform.BROADWELL)
> >>>>
> >>>> I think about 'gen' but then it is overloaded....
> >>>>
> >>>> then I think about 'ip' is worse...
> >>>>
> >>>> 'version'?
> >>>>
> >>>> 'name'?
> >>>>
> >>>> if (display->name.HASWELL)...
> >>>>
> >>>> ....
> >>>>
> >>>> But well, I like the overall simplification here in general.
> >>>> Without a better name to suggest, I guess let's just move ahead...
> >>>
> >>> One slight concern with the is.foo is whether it complicates finding
> >>> things with eg. cscope. But I suppose for platforms that doesn't matter
> >>> all that much. For the has_foo stuff it'd be much more relevant.
> >>
> >>It does make finding things harder with cscope and gnu global, but git
> >>grep for is.FOO is pretty accurate.
> >>
> >>> Anyways, can't think of anything particularly elegant myself either,
> >>> so go ahead I guess.
> >>
> >>So I haven't yet. I just still have that slightly uneasy feeling about
> >>whether this is a good thing or not. That doesn't usually make me shy
> >>away from things, because you can fix stuff later, but getting this
> >>wrong causes so much churn everywhere.
> >>
> >>The fact that it's not a macro makes it less flexible for future
> >>changes. The display->is.FOO is somewhat legible, but could be
> >>better. Would all lowercase make it better? I don't know.
> >>
> >>More alternatives? Not elegant for sure, but just alternatives:
> >>
> >>- Lowercase names:
> >>
> >>	if (display->is.rocketlake)
> >
> > what I really dislike is a struct named "is". Going full mesa-way would
> > be slightly better IMO:
> >
> > 	if (display->is_rockelake)
> >
> > or
> >
> > 	if (display->platform_rocketlake)
> >
> > or
> >
> > 	if (display->platform.rocketlake)
> 
> Fair enough.
> 
> >From implementation POV having a sub-struct is easier than not.

how the subplatform would appear in this case?

> 
> >>
> >>  Does not help with flexibility or cscope.
> >>
> >>- Lowercase macros for display, e.g. is_rocketlake().
> >>
> >>	if (is_rocketlake(display))
> >>
> >>- Macros based on just the platform name, e.g. ROCKETLAKE().
> >>
> >>	if (ROCKETLAKE(display))
> >>
> >>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
> >>
> >>	if (PLATFORM_ROCKETLAKE(display))
> >>
> >>  But that can get a bit long in some if ladders etc.
> >
> > Does it matter much? I think those would be the exception, particularly
> > because platform checks are kind of rare these days.
> 
> Well, they're maybe the exception for new platforms, but i915 display
> does have to deal with a lot of legacy with a lot of platform checks.
> 
> > grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
> > because wherever we can we check by graphics/display version rather than
> > platform.
> 
> i915 display has only one use of IS_LUNARLAKE(), but there are 1k+ other
> uses of IS_<PLATFORM>.
> 
> Incidentally, this is the reason I'm procrastinating about the change at
> all.
> 
> > Then simply using something similar to what we already have in xe, would
> > be great IMO:
> >
> > 	if (display->platform == PLATFORM_LUNARLAKE)
> >
> > it may be verbose, but shouldn't be much used to matter in the end.
> 
> The downside with that is that you can't deal with subplatforms as
> easily. It becomes
> 
> 	if (display->platform == PLATFORM_LUNARLAKE ||
> 	    (display->platform == PLATFORM_ALDERLAKE_P &&
> 	     display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N))
> 
> or similar. Definitely not a fan.

unless the subplatform already includes the platform?

But well, I also don't have a good suggestion here.
The '.is' struct is strange indeed, but at least covers all the past
and future strange cases.

But I also wouldn't mind if we decide to get the verbose path,
but try to at least making the subplatform already infering the
platform in a way that this case could only be:

       if (display->platform == PLATFORM_LUNARLAKE ||
            display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)


or perhaps do in a way that we don't even need the subplatform struct?

       if (display->platform == PLATFORM_LUNARLAKE ||
            display->platform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)

> 
> 
> BR,
> Jani.
> 
> 
> >
> > Lucas De Marchi
> >
> >>
> >>- Go through the trouble of making the existing IS_FOO() macros _Generic
> >>  and accept either i915 or display pointer. This does postpone making
> >>  any further changes, but fairly soon there will need to be two sets of
> >>  macros, separate for i915 and display, even though named the same.
> >>
> >>  Also, the _Generic thing would look up the platform definitions from
> >>  different places, which could be error prone.
> >>
> >>
> >>Yeah, procrastination...
> >>
> >>
> >>BR,
> >>Jani.
> >>
> >>
> >>
> >>
> >>-- 
> >>Jani Nikula, Intel
> 
> -- 
> Jani Nikula, Intel
Jani Nikula Sept. 27, 2024, 12:16 p.m. UTC | #8
On Thu, 26 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
>> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
>> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
>> >>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
>> >>>> > v2 of [1]. Please read the cover letter there.
>> >>>> >
>> >>>> > This addresses review comments and adds a few more commits on top, in particular
>> >>>> > the last one showcasing the approach.
>> >>>> >
>> >>>> > The main question remains, is this what we want?
>> >>>>
>> >>>> I don't know why, but the 'is' thing is still strange.
>> >>>>
>> >>>> I know I know... I'm bad with naming myself.
>> >>>>
>> >>>> I think about 'platform' but that get too big
>> >>>>
>> >>>> if (display->platform.BROADWELL)
>> >>>>
>> >>>> I think about 'gen' but then it is overloaded....
>> >>>>
>> >>>> then I think about 'ip' is worse...
>> >>>>
>> >>>> 'version'?
>> >>>>
>> >>>> 'name'?
>> >>>>
>> >>>> if (display->name.HASWELL)...
>> >>>>
>> >>>> ....
>> >>>>
>> >>>> But well, I like the overall simplification here in general.
>> >>>> Without a better name to suggest, I guess let's just move ahead...
>> >>>
>> >>> One slight concern with the is.foo is whether it complicates finding
>> >>> things with eg. cscope. But I suppose for platforms that doesn't matter
>> >>> all that much. For the has_foo stuff it'd be much more relevant.
>> >>
>> >>It does make finding things harder with cscope and gnu global, but git
>> >>grep for is.FOO is pretty accurate.
>> >>
>> >>> Anyways, can't think of anything particularly elegant myself either,
>> >>> so go ahead I guess.
>> >>
>> >>So I haven't yet. I just still have that slightly uneasy feeling about
>> >>whether this is a good thing or not. That doesn't usually make me shy
>> >>away from things, because you can fix stuff later, but getting this
>> >>wrong causes so much churn everywhere.
>> >>
>> >>The fact that it's not a macro makes it less flexible for future
>> >>changes. The display->is.FOO is somewhat legible, but could be
>> >>better. Would all lowercase make it better? I don't know.
>> >>
>> >>More alternatives? Not elegant for sure, but just alternatives:
>> >>
>> >>- Lowercase names:
>> >>
>> >>	if (display->is.rocketlake)
>> >
>> > what I really dislike is a struct named "is". Going full mesa-way would
>> > be slightly better IMO:
>> >
>> > 	if (display->is_rockelake)
>> >
>> > or
>> >
>> > 	if (display->platform_rocketlake)
>> >
>> > or
>> >
>> > 	if (display->platform.rocketlake)
>> 
>> Fair enough.
>> 
>> >From implementation POV having a sub-struct is easier than not.
>
> how the subplatform would appear in this case?

For example, RPL-S:


	if (display->platform.alderlake_s_raptorlake_s)

But the main platform also matches its subplatforms:

	if (display->platform.alderlake_s)

This is the same as with the patches at hand. Except for the
uppercase/lowercase difference, and s/is/platform/.

>> >>  Does not help with flexibility or cscope.
>> >>
>> >>- Lowercase macros for display, e.g. is_rocketlake().
>> >>
>> >>	if (is_rocketlake(display))
>> >>
>> >>- Macros based on just the platform name, e.g. ROCKETLAKE().
>> >>
>> >>	if (ROCKETLAKE(display))
>> >>
>> >>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
>> >>
>> >>	if (PLATFORM_ROCKETLAKE(display))
>> >>
>> >>  But that can get a bit long in some if ladders etc.
>> >
>> > Does it matter much? I think those would be the exception, particularly
>> > because platform checks are kind of rare these days.
>> 
>> Well, they're maybe the exception for new platforms, but i915 display
>> does have to deal with a lot of legacy with a lot of platform checks.
>> 
>> > grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
>> > because wherever we can we check by graphics/display version rather than
>> > platform.
>> 
>> i915 display has only one use of IS_LUNARLAKE(), but there are 1k+ other
>> uses of IS_<PLATFORM>.
>> 
>> Incidentally, this is the reason I'm procrastinating about the change at
>> all.
>> 
>> > Then simply using something similar to what we already have in xe, would
>> > be great IMO:
>> >
>> > 	if (display->platform == PLATFORM_LUNARLAKE)
>> >
>> > it may be verbose, but shouldn't be much used to matter in the end.
>> 
>> The downside with that is that you can't deal with subplatforms as
>> easily. It becomes
>> 
>> 	if (display->platform == PLATFORM_LUNARLAKE ||
>> 	    (display->platform == PLATFORM_ALDERLAKE_P &&
>> 	     display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N))
>> 
>> or similar. Definitely not a fan.
>
> unless the subplatform already includes the platform?

Oh, yeah, it would.

> But well, I also don't have a good suggestion here.
> The '.is' struct is strange indeed, but at least covers all the past
> and future strange cases.
>
> But I also wouldn't mind if we decide to get the verbose path,
> but try to at least making the subplatform already infering the
> platform in a way that this case could only be:
>
>        if (display->platform == PLATFORM_LUNARLAKE ||
>             display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)
>
>
> or perhaps do in a way that we don't even need the subplatform struct?
>
>        if (display->platform == PLATFORM_LUNARLAKE ||
>             display->platform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)

How would that even be possible? display->platform can't be multiple
things at the same time, unless it's a bitmask. If it's a bitmask, you
need a way to nicely check for it instead of having it everywhere.

The alternatives are using a macro, or using bitfields - which is
exactly what the patch at hand does. We've come full circle.


BR,
Jani.


>
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Lucas De Marchi
>> >
>> >>
>> >>- Go through the trouble of making the existing IS_FOO() macros _Generic
>> >>  and accept either i915 or display pointer. This does postpone making
>> >>  any further changes, but fairly soon there will need to be two sets of
>> >>  macros, separate for i915 and display, even though named the same.
>> >>
>> >>  Also, the _Generic thing would look up the platform definitions from
>> >>  different places, which could be error prone.
>> >>
>> >>
>> >>Yeah, procrastination...
>> >>
>> >>
>> >>BR,
>> >>Jani.
>> >>
>> >>
>> >>
>> >>
>> >>-- 
>> >>Jani Nikula, Intel
>> 
>> -- 
>> Jani Nikula, Intel
Rodrigo Vivi Sept. 27, 2024, 2:49 p.m. UTC | #9
On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote:
> On Thu, 26 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Tue, Sep 24, 2024 at 04:37:04PM +0300, Jani Nikula wrote:
> >> On Tue, 24 Sep 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> > On Tue, Sep 24, 2024 at 12:49:25PM GMT, Jani Nikula wrote:
> >> >>On Thu, 29 Aug 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> >>> On Wed, Aug 28, 2024 at 04:41:24PM -0400, Rodrigo Vivi wrote:
> >> >>>> On Mon, Aug 19, 2024 at 09:44:27PM +0300, Jani Nikula wrote:
> >> >>>> > v2 of [1]. Please read the cover letter there.
> >> >>>> >
> >> >>>> > This addresses review comments and adds a few more commits on top, in particular
> >> >>>> > the last one showcasing the approach.
> >> >>>> >
> >> >>>> > The main question remains, is this what we want?
> >> >>>>
> >> >>>> I don't know why, but the 'is' thing is still strange.
> >> >>>>
> >> >>>> I know I know... I'm bad with naming myself.
> >> >>>>
> >> >>>> I think about 'platform' but that get too big
> >> >>>>
> >> >>>> if (display->platform.BROADWELL)
> >> >>>>
> >> >>>> I think about 'gen' but then it is overloaded....
> >> >>>>
> >> >>>> then I think about 'ip' is worse...
> >> >>>>
> >> >>>> 'version'?
> >> >>>>
> >> >>>> 'name'?
> >> >>>>
> >> >>>> if (display->name.HASWELL)...
> >> >>>>
> >> >>>> ....
> >> >>>>
> >> >>>> But well, I like the overall simplification here in general.
> >> >>>> Without a better name to suggest, I guess let's just move ahead...
> >> >>>
> >> >>> One slight concern with the is.foo is whether it complicates finding
> >> >>> things with eg. cscope. But I suppose for platforms that doesn't matter
> >> >>> all that much. For the has_foo stuff it'd be much more relevant.
> >> >>
> >> >>It does make finding things harder with cscope and gnu global, but git
> >> >>grep for is.FOO is pretty accurate.
> >> >>
> >> >>> Anyways, can't think of anything particularly elegant myself either,
> >> >>> so go ahead I guess.
> >> >>
> >> >>So I haven't yet. I just still have that slightly uneasy feeling about
> >> >>whether this is a good thing or not. That doesn't usually make me shy
> >> >>away from things, because you can fix stuff later, but getting this
> >> >>wrong causes so much churn everywhere.
> >> >>
> >> >>The fact that it's not a macro makes it less flexible for future
> >> >>changes. The display->is.FOO is somewhat legible, but could be
> >> >>better. Would all lowercase make it better? I don't know.
> >> >>
> >> >>More alternatives? Not elegant for sure, but just alternatives:
> >> >>
> >> >>- Lowercase names:
> >> >>
> >> >>	if (display->is.rocketlake)
> >> >
> >> > what I really dislike is a struct named "is". Going full mesa-way would
> >> > be slightly better IMO:
> >> >
> >> > 	if (display->is_rockelake)
> >> >
> >> > or
> >> >
> >> > 	if (display->platform_rocketlake)
> >> >
> >> > or
> >> >
> >> > 	if (display->platform.rocketlake)
> >> 
> >> Fair enough.
> >> 
> >> >From implementation POV having a sub-struct is easier than not.
> >
> > how the subplatform would appear in this case?
> 
> For example, RPL-S:
> 
> 
> 	if (display->platform.alderlake_s_raptorlake_s)
> 
> But the main platform also matches its subplatforms:
> 
> 	if (display->platform.alderlake_s)
> 
> This is the same as with the patches at hand. Except for the
> uppercase/lowercase difference, and s/is/platform/.

Yeap, let's go with that then!

> 
> >> >>  Does not help with flexibility or cscope.
> >> >>
> >> >>- Lowercase macros for display, e.g. is_rocketlake().
> >> >>
> >> >>	if (is_rocketlake(display))
> >> >>
> >> >>- Macros based on just the platform name, e.g. ROCKETLAKE().
> >> >>
> >> >>	if (ROCKETLAKE(display))
> >> >>
> >> >>  or change IS_ to something else e.g. PLATFORM_ROCKETLAKE().
> >> >>
> >> >>	if (PLATFORM_ROCKETLAKE(display))
> >> >>
> >> >>  But that can get a bit long in some if ladders etc.
> >> >
> >> > Does it matter much? I think those would be the exception, particularly
> >> > because platform checks are kind of rare these days.
> >> 
> >> Well, they're maybe the exception for new platforms, but i915 display
> >> does have to deal with a lot of legacy with a lot of platform checks.
> >> 
> >> > grepping for LUNARLAKE in xe reveals only 2 users (+ few workarounds),
> >> > because wherever we can we check by graphics/display version rather than
> >> > platform.
> >> 
> >> i915 display has only one use of IS_LUNARLAKE(), but there are 1k+ other
> >> uses of IS_<PLATFORM>.
> >> 
> >> Incidentally, this is the reason I'm procrastinating about the change at
> >> all.
> >> 
> >> > Then simply using something similar to what we already have in xe, would
> >> > be great IMO:
> >> >
> >> > 	if (display->platform == PLATFORM_LUNARLAKE)
> >> >
> >> > it may be verbose, but shouldn't be much used to matter in the end.
> >> 
> >> The downside with that is that you can't deal with subplatforms as
> >> easily. It becomes
> >> 
> >> 	if (display->platform == PLATFORM_LUNARLAKE ||
> >> 	    (display->platform == PLATFORM_ALDERLAKE_P &&
> >> 	     display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N))
> >> 
> >> or similar. Definitely not a fan.
> >
> > unless the subplatform already includes the platform?
> 
> Oh, yeah, it would.
> 
> > But well, I also don't have a good suggestion here.
> > The '.is' struct is strange indeed, but at least covers all the past
> > and future strange cases.
> >
> > But I also wouldn't mind if we decide to get the verbose path,
> > but try to at least making the subplatform already infering the
> > platform in a way that this case could only be:
> >
> >        if (display->platform == PLATFORM_LUNARLAKE ||
> >             display->subplatform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)
> >
> >
> > or perhaps do in a way that we don't even need the subplatform struct?
> >
> >        if (display->platform == PLATFORM_LUNARLAKE ||
> >             display->platform == SUBPLATFORM_ALDERLAKE_P_ALDERLAKE_N)
> 
> How would that even be possible? display->platform can't be multiple
> things at the same time, unless it's a bitmask. If it's a bitmask, you
> need a way to nicely check for it instead of having it everywhere.
> 
> The alternatives are using a macro, or using bitfields - which is
> exactly what the patch at hand does. We've come full circle.

yeap, forget about this...

> 
> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> > Lucas De Marchi
> >> >
> >> >>
> >> >>- Go through the trouble of making the existing IS_FOO() macros _Generic
> >> >>  and accept either i915 or display pointer. This does postpone making
> >> >>  any further changes, but fairly soon there will need to be two sets of
> >> >>  macros, separate for i915 and display, even though named the same.
> >> >>
> >> >>  Also, the _Generic thing would look up the platform definitions from
> >> >>  different places, which could be error prone.
> >> >>
> >> >>
> >> >>Yeah, procrastination...
> >> >>
> >> >>
> >> >>BR,
> >> >>Jani.
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>-- 
> >> >>Jani Nikula, Intel
> >> 
> >> -- 
> >> Jani Nikula, Intel
> 
> -- 
> Jani Nikula, Intel
Jani Nikula Sept. 27, 2024, 3:01 p.m. UTC | #10
On Fri, 27 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote:
>> For example, RPL-S:
>> 
>> 
>> 	if (display->platform.alderlake_s_raptorlake_s)
>> 
>> But the main platform also matches its subplatforms:
>> 
>> 	if (display->platform.alderlake_s)
>> 
>> This is the same as with the patches at hand. Except for the
>> uppercase/lowercase difference, and s/is/platform/.
>
> Yeap, let's go with that then!

Double-checking: do you want both s/is/platform *and* lowercase?

	if (display->platform.alderlake_s_raptorlake_s)

Mulling over this I've ended up preferring lowercase. (Maybe need to do
something to pretty print them to logs.)

And just the platform without "is_" prefix, i.e. *not*:

	if (display->platform.is_alderlake_s_raptorlake_s)


BR,
Jani.
Rodrigo Vivi Sept. 27, 2024, 4:13 p.m. UTC | #11
On Fri, Sep 27, 2024 at 06:01:58PM +0300, Jani Nikula wrote:
> On Fri, 27 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote:
> >> For example, RPL-S:
> >> 
> >> 
> >> 	if (display->platform.alderlake_s_raptorlake_s)
> >> 
> >> But the main platform also matches its subplatforms:
> >> 
> >> 	if (display->platform.alderlake_s)
> >> 
> >> This is the same as with the patches at hand. Except for the
> >> uppercase/lowercase difference, and s/is/platform/.
> >
> > Yeap, let's go with that then!
> 
> Double-checking: do you want both s/is/platform *and* lowercase?
> 
> 	if (display->platform.alderlake_s_raptorlake_s)
> 
> Mulling over this I've ended up preferring lowercase. (Maybe need to do
> something to pretty print them to logs.)

I believe that I like the lowercase more too.
We could have a map for the debug prints, but I would delay this, because
that could end up in discussions with marketing brand strings... :/

> 
> And just the platform without "is_" prefix, i.e. *not*:
> 
> 	if (display->platform.is_alderlake_s_raptorlake_s)

I like the version without 'is' better...

> 
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel
Lucas De Marchi Sept. 27, 2024, 6:12 p.m. UTC | #12
On Fri, Sep 27, 2024 at 12:13:20PM GMT, Rodrigo Vivi wrote:
>On Fri, Sep 27, 2024 at 06:01:58PM +0300, Jani Nikula wrote:
>> On Fri, 27 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > On Fri, Sep 27, 2024 at 03:16:23PM +0300, Jani Nikula wrote:
>> >> For example, RPL-S:
>> >>
>> >>
>> >> 	if (display->platform.alderlake_s_raptorlake_s)
>> >>
>> >> But the main platform also matches its subplatforms:
>> >>
>> >> 	if (display->platform.alderlake_s)
>> >>
>> >> This is the same as with the patches at hand. Except for the
>> >> uppercase/lowercase difference, and s/is/platform/.
>> >
>> > Yeap, let's go with that then!
>>
>> Double-checking: do you want both s/is/platform *and* lowercase?
>>
>> 	if (display->platform.alderlake_s_raptorlake_s)
>>
>> Mulling over this I've ended up preferring lowercase. (Maybe need to do
>> something to pretty print them to logs.)
>
>I believe that I like the lowercase more too.

ack

>We could have a map for the debug prints, but I would delay this, because
>that could end up in discussions with marketing brand strings... :/
>
>>
>> And just the platform without "is_" prefix, i.e. *not*:
>>
>> 	if (display->platform.is_alderlake_s_raptorlake_s)
>
>I like the version without 'is' better...

ack

Lucas De Marchi

>
>>
>>
>> BR,
>> Jani.
>>
>>
>> --
>> Jani Nikula, Intel