Re: [PATCH 15/15] drm/i915/display: add mobile platform group

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Nov 07, 2024 at 12:32:56PM +0200, Jani Nikula wrote:
> On Wed, 06 Nov 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Mon, Nov 04, 2024 at 07:19:29PM +0200, Jani Nikula wrote:
> >> Identify mobile platforms separately in display, using the platform
> >> group mechanism. This enables dropping the dependency on i915_drv.h
> >> IS_MOBILE() from display code.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  .../drm/i915/display/intel_display_device.c   | 104 ++++++++++++------
> >>  .../drm/i915/display/intel_display_device.h   |   1 +
> >>  2 files changed, 70 insertions(+), 35 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> >> index 8ba1b4652ca9..5689c5e5db0e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> >> @@ -253,6 +253,7 @@ static const struct intel_display_device_info no_display = {};
> >>  
> >>  static const struct platform_desc i830_desc = {
> >>  	PLATFORM(i830),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		I830_DISPLAY,
> >>  
> >> @@ -271,6 +272,7 @@ static const struct platform_desc i845_desc = {
> >>  
> >>  static const struct platform_desc i85x_desc = {
> >>  	PLATFORM(i85x),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		I830_DISPLAY,
> >>  
> >> @@ -313,6 +315,7 @@ static const struct platform_desc i915g_desc = {
> >>  
> >>  static const struct platform_desc i915gm_desc = {
> >>  	PLATFORM(i915gm),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		GEN3_DISPLAY,
> >>  		I9XX_COLORS,
> >> @@ -337,6 +340,7 @@ static const struct platform_desc i945g_desc = {
> >>  
> >>  static const struct platform_desc i945gm_desc = {
> >>  	PLATFORM(i915gm),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		GEN3_DISPLAY,
> >>  		I9XX_COLORS,
> >> @@ -358,13 +362,21 @@ static const struct platform_desc g33_desc = {
> >>  	},
> >>  };
> >>  
> >> -static const struct platform_desc pnv_desc = {
> >> +static const struct intel_display_device_info pnv_display = {
> >> +	GEN3_DISPLAY,
> >> +	I9XX_COLORS,
> >> +	.has_hotplug = 1,
> >> +};
> >> +
> >> +static const struct platform_desc pnv_g_desc = {
> >>  	PLATFORM(pineview),
> >> -	.info = &(const struct intel_display_device_info) {
> >> -		GEN3_DISPLAY,
> >> -		I9XX_COLORS,
> >> -		.has_hotplug = 1,
> >> -	},
> >> +	.info = &pnv_display,
> >> +};
> >> +
> >> +static const struct platform_desc pnv_m_desc = {
> >> +	PLATFORM(pineview),
> >> +	PLATFORM_GROUP(mobile),
> >> +	.info = &pnv_display,
> >>  };
> >>  
> >>  #define GEN4_DISPLAY \
> >> @@ -391,6 +403,7 @@ static const struct platform_desc i965g_desc = {
> >>  
> >>  static const struct platform_desc i965gm_desc = {
> >>  	PLATFORM(i965gm),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		GEN4_DISPLAY,
> >>  		.has_overlay = 1,
> >> @@ -414,6 +427,7 @@ static const struct platform_desc g45_desc = {
> >>  static const struct platform_desc gm45_desc = {
> >>  	PLATFORM(gm45),
> >>  	PLATFORM_GROUP(g4x),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		GEN4_DISPLAY,
> >>  		.supports_tv = 1,
> >> @@ -444,6 +458,7 @@ static const struct platform_desc ilk_d_desc = {
> >>  
> >>  static const struct platform_desc ilk_m_desc = {
> >>  	PLATFORM(ironlake),
> >> +	PLATFORM_GROUP(mobile),
> >>  	.info = &(const struct intel_display_device_info) {
> >>  		ILK_DISPLAY,
> >>  
> >> @@ -451,38 +466,54 @@ static const struct platform_desc ilk_m_desc = {
> >>  	},
> >>  };
> >>  
> >> -static const struct platform_desc snb_desc = {
> >> +const struct intel_display_device_info snb_display = {
> >
> > get static back here as test robot already pointed out...
> >
> >> +	.has_hotplug = 1,
> >> +	I9XX_PIPE_OFFSETS,
> >> +	I9XX_CURSOR_OFFSETS,
> >> +	ILK_COLORS,
> >> +
> >> +	.__runtime_defaults.ip.ver = 6,
> >> +	.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B),
> >> +	.__runtime_defaults.cpu_transcoder_mask =
> >> +	BIT(TRANSCODER_A) | BIT(TRANSCODER_B),
> >> +	.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D), /* DP A, SDVO/HDMI/DP B, HDMI/DP C/D */
> >> +	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
> >> +};
> >> +
> >> +static const struct platform_desc snb_d_desc = {
> >>  	PLATFORM(sandybridge),
> >> -	.info = &(const struct intel_display_device_info) {
> >> -		.has_hotplug = 1,
> >> -		I9XX_PIPE_OFFSETS,
> >> -		I9XX_CURSOR_OFFSETS,
> >> -		ILK_COLORS,
> >> +	.info = &snb_display,
> >> +};
> >>  
> >> -		.__runtime_defaults.ip.ver = 6,
> >> -		.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B),
> >> -		.__runtime_defaults.cpu_transcoder_mask =
> >> -		BIT(TRANSCODER_A) | BIT(TRANSCODER_B),
> >> -		.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D), /* DP A, SDVO/HDMI/DP B, HDMI/DP C/D */
> >> -		.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
> >> -	},
> >> +static const struct platform_desc snb_m_desc = {
> >> +	PLATFORM(sandybridge),
> >> +	PLATFORM_GROUP(mobile),
> >> +	.info = &snb_display,
> >> +};
> >> +
> >> +static const struct intel_display_device_info ivb_display = {
> >> +	.has_hotplug = 1,
> >> +	IVB_PIPE_OFFSETS,
> >> +	IVB_CURSOR_OFFSETS,
> >> +	IVB_COLORS,
> >> +
> >> +	.__runtime_defaults.ip.ver = 7,
> >> +	.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> >> +	.__runtime_defaults.cpu_transcoder_mask =
> >> +	BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | BIT(TRANSCODER_C),
> >> +	.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D), /* DP A, SDVO/HDMI/DP B, HDMI/DP C/D */
> >> +	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
> >>  };
> >>  
> >> -static const struct platform_desc ivb_desc = {
> >> +static const struct platform_desc ivb_d_desc = {
> >>  	PLATFORM(ivybridge),
> >> -	.info = &(const struct intel_display_device_info) {
> >> -		.has_hotplug = 1,
> >> -		IVB_PIPE_OFFSETS,
> >> -		IVB_CURSOR_OFFSETS,
> >> -		IVB_COLORS,
> >> +	.info = &ivb_display,
> >
> > now I'm wondering 2 things:
> >
> > 1. is info really the good name now?
> 
> Why not?
> 
> Desc is the platform description, unrelated to display, and info is the
> display info. In some cases display info was (and still is) inline, but
> when it's shared between multiple platforms it needs to be separate.
> 
> > 2. does it deserves a separate patch refactor other then this one focused
> > on the mobile group?
> 
> Un-inlining the dispay info init could reasonably be a separate
> patch. Question is, do we then want to do it for all platforms, or just
> the ones that need the split here?

okay, let's just go as is

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> BR,
> Jani.
> 
> 
> >
> >> +};
> >>  
> >> -		.__runtime_defaults.ip.ver = 7,
> >> -		.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> >> -		.__runtime_defaults.cpu_transcoder_mask =
> >> -		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | BIT(TRANSCODER_C),
> >> -		.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D), /* DP A, SDVO/HDMI/DP B, HDMI/DP C/D */
> >> -		.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
> >> -	},
> >> +static const struct platform_desc ivb_m_desc = {
> >> +	PLATFORM(ivybridge),
> >> +	PLATFORM_GROUP(mobile),
> >> +	.info = &ivb_display,
> >>  };
> >>  
> >>  static const struct platform_desc vlv_desc = {
> >> @@ -1385,11 +1416,14 @@ static const struct {
> >>  	INTEL_I965GM_IDS(INTEL_DISPLAY_DEVICE, &i965gm_desc),
> >>  	INTEL_GM45_IDS(INTEL_DISPLAY_DEVICE, &gm45_desc),
> >>  	INTEL_G45_IDS(INTEL_DISPLAY_DEVICE, &g45_desc),
> >> -	INTEL_PNV_IDS(INTEL_DISPLAY_DEVICE, &pnv_desc),
> >> +	INTEL_PNV_G_IDS(INTEL_DISPLAY_DEVICE, &pnv_g_desc),
> >> +	INTEL_PNV_M_IDS(INTEL_DISPLAY_DEVICE, &pnv_m_desc),
> >>  	INTEL_ILK_D_IDS(INTEL_DISPLAY_DEVICE, &ilk_d_desc),
> >>  	INTEL_ILK_M_IDS(INTEL_DISPLAY_DEVICE, &ilk_m_desc),
> >> -	INTEL_SNB_IDS(INTEL_DISPLAY_DEVICE, &snb_desc),
> >> -	INTEL_IVB_IDS(INTEL_DISPLAY_DEVICE, &ivb_desc),
> >> +	INTEL_SNB_D_IDS(INTEL_DISPLAY_DEVICE, &snb_d_desc),
> >> +	INTEL_SNB_M_IDS(INTEL_DISPLAY_DEVICE, &snb_m_desc),
> >> +	INTEL_IVB_D_IDS(INTEL_DISPLAY_DEVICE, &ivb_d_desc),
> >> +	INTEL_IVB_M_IDS(INTEL_DISPLAY_DEVICE, &ivb_m_desc),
> >>  	INTEL_HSW_IDS(INTEL_DISPLAY_DEVICE, &hsw_desc),
> >>  	INTEL_VLV_IDS(INTEL_DISPLAY_DEVICE, &vlv_desc),
> >>  	INTEL_BDW_IDS(INTEL_DISPLAY_DEVICE, &bdw_desc),
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> >> index e1398689cda5..84378c787923 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> >> @@ -24,6 +24,7 @@ struct pci_dev;
> >>  #define INTEL_DISPLAY_PLATFORMS(func) \
> >>  	/* Platform group aliases */ \
> >>  	func(g4x) /* g45 and gm45 */ \
> >> +	func(mobile) /* mobile platforms */ \
> >>  	func(dgfx) /* discrete graphics */ \
> >>  	/* Display ver 2 */ \
> >>  	func(i830) \
> >> -- 
> >> 2.39.5
> >> 
> 
> -- 
> Jani Nikula, Intel