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

Message

Jani Nikula June 18, 2024, 2:22 p.m. UTC

Long story short, we'll need to identify platforms in display code using
some other way than i915 core IS_<PLATFORM>() macros if we ever want to
make a strict separation between the display and non-display parts.

I tossed some ideas around (see the bottom of this mail), Lucas liked
something similar to what I have here. Essentially with this, you can
replace platform checks like IS_TIGERLAKE(i915) with
display->is.TIGERLAKE and subplatform checks like IS_RAPTORLAKE_S(i915)
with display->is.ALDERLAKE_S_RAPTORLAKE_S.

It would be possible to drop the ALDERLAKE_S or "parent" platform part
there, but I think it's useful in many cases to be explicit it's a
subplatform we're talking about.

It's also possible to convert all of this to lowercase if desired,
i.e. display->is.tigerlake and display->is.alderlake_s_raptorlake_s.

There's one more wrinkle I didn't address; currently IS_HASWELL_ULT()
and IS_BROADWELL_ULT() also match the ULX variants. This is not the case
here yet.

Thoughts?

BR,
Jani.


void foo(void)
{
	/*
	 * Examples with a platform check (Tigerlake) and a subplatform check
	 * (Alderlake S subplatform Raptorlake S).
	 */

	/*
	 * is_<platform>(display). Same as i915 core, but lowercase.
	 *
	 * Pros:
	 * - Easy to convert
	 * - Short
	 *
	 * Cons:
	 * - Need to keep defining new macros for new platfoms and subplatforms
	 */
	if (is_tigerlake(display) || is_alderlake_s_raptorlake_s(display)) {
	}

	/*
	 * is_platform(display, <platform>) check.
	 *
	 * Alternatively is_plat() or is_display() or something else?
	 *
	 * Pros:
	 * - Can be made to handle both platforms and subplatforms by
	 *   renumbering subplatforms enum
	 * - No need to define new macros for new platforms
	 *
	 * Cons:
	 * - A bit long
	 */
	if (is_platform(display, INTEL_DISPLAY_TIGERLAKE) ||
	    is_platform(display, INTEL_DISPLAY_ALDERLAKE_S_RAPTORLAKE_S)) {
	}

	/*
	 * An is_platform() with a macro wrapper to abbreviate param.
	 *
	 * Pros:
	 * - Shorter
	 *
	 * Cons:
	 * - Throws off cscope and gnu global
	 */
	if (is_platform(display, TIGERLAKE) ||
	    is_platform(display, ALDERLAKE_S_RAPTORLAKE_S)) {
	}

	/*
	 * Functions to return platform/subplatforms.
	 *
	 * Pros:
	 * - No need to define new macros for new platforms
	 *
	 * Cons:
	 * - Long
	 * - Need separate checks for platform/subplatform
	 */
	if (intel_platform(display) == INTEL_DISPLAY_TIGERLAKE ||
	    intel_subplatform(display) == INTEL_DISPLAY_ALDERLAKE_S_RAPTORLAKE_S) {
	}

	/*
	 * Initialize bitfields in display according to platform/subplatform
	 *
	 * Pros:
	 * - Really short
	 * - Does not pollute namespace with is_something
	 *
	 * Cons:
	 * - Pollutes top level struct intel_display
	 * - Kind of belongs in display device or runtime info, but that would
	 *   again be too long to be helpful
	 */
	if (display->is_tigerlake || display->alderlake_s_raptorlake_s) {
	}
}


Jani Nikula (6):
  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 macros
  drm/i915/display: add "display is" structure with platform members
  drm/i915/display: add "is" member to struct intel_display
  drm/i915/display: remove the display platform enum as unnecessary

 .../gpu/drm/i915/display/intel_display_core.h |   3 +
 .../drm/i915/display/intel_display_device.c   |  79 ++++++---
 .../drm/i915/display/intel_display_device.h   | 165 +++++++++---------
 3 files changed, 136 insertions(+), 111 deletions(-)