[Intel-gfx] [PATCH 04/19] drm/i915: Move the power domain->well mappings to intel_display_power_map.c

[Imre Deak]

On Mon, Jan 31, 2022 at 02:15:25PM +0200, Jani Nikula wrote:
> On Fri, 28 Jan 2022, Imre Deak <imre.deak at intel.com> wrote:
> > Move the list of platform specific power domain -> power well
> > definitions to intel_display_power_map.c. While at it group the
> > platforms' power domain macros with the corresponding power well lists
> > and keep all the power domain lists in the same order (matching the enum
> > order).
> >
> > No functional changes.
> >
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> 
> The commit message should explain the why.
> 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> > index b30e6133c66d0..a0e68ae691021 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > @@ -197,6 +197,7 @@ struct intel_display_power_domain_set {
> >  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> >  		for_each_if(BIT_ULL(domain) & (mask))
> >  
> > +/* intel_display_power.c */
> >  int intel_power_domains_init(struct drm_i915_private *dev_priv);
> >  void intel_power_domains_cleanup(struct drm_i915_private *dev_priv);
> >  void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume);
> > @@ -316,4 +317,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >  			  enum dpio_channel ch, bool override);
> >  
> > +/* intel_display_power_map.c */
> > +const char *
> > +intel_display_power_domain_str(enum intel_display_power_domain domain);
> > +
> >  #endif /* __INTEL_DISPLAY_POWER_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_internal.h b/drivers/gpu/drm/i915/display/intel_display_power_internal.h
> > new file mode 100644
> > index 0000000000000..3fc7c7d0bc9e9
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power_internal.h
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2022 Intel Corporation
> > + */
> > +
> > +#ifndef __INTEL_DISPLAY_POWER_INTERNAL_H__
> > +#define __INTEL_DISPLAY_POWER_INTERNAL_H__
> > +
> > +#include "i915_reg_defs.h"
> > +
> > +#include "intel_display.h"
> > +#include "intel_display_power.h"
> > +
> > +struct i915_power_well_regs;
> > +
> > +/* Power well structure for haswell */
> > +struct i915_power_well_desc {
> > +	const char *name;
> > +	bool always_on;
> > +	u64 domains;
> > +	/* unique identifier for this power well */
> > +	enum i915_power_well_id id;
> > +	/*
> > +	 * Arbitraty data associated with this power well. Platform and power
> > +	 * well specific.
> > +	 */
> > +	union {
> > +		struct {
> > +			/*
> > +			 * request/status flag index in the PUNIT power well
> > +			 * control/status registers.
> > +			 */
> > +			u8 idx;
> > +		} vlv;
> > +		struct {
> > +			enum dpio_phy phy;
> > +		} bxt;
> > +		struct {
> > +			/*
> > +			 * request/status flag index in the power well
> > +			 * constrol/status registers.
> > +			 */
> > +			u8 idx;
> > +			/* Mask of pipes whose IRQ logic is backed by the pw */
> > +			u8 irq_pipe_mask;
> > +			/*
> > +			 * Instead of waiting for the status bit to ack enables,
> > +			 * just wait a specific amount of time and then consider
> > +			 * the well enabled.
> > +			 */
> > +			u16 fixed_enable_delay;
> > +			/* The pw is backing the VGA functionality */
> > +			bool has_vga:1;
> > +			bool has_fuses:1;
> > +			/*
> > +			 * The pw is for an ICL+ TypeC PHY port in
> > +			 * Thunderbolt mode.
> > +			 */
> > +			bool is_tc_tbt:1;
> > +		} hsw;
> > +	};
> > +	const struct i915_power_well_ops *ops;
> > +};
> > +
> > +struct i915_power_well {
> > +	const struct i915_power_well_desc *desc;
> > +	/* power well enable/disable usage count */
> > +	int count;
> > +	/* cached hw enabled state */
> > +	bool hw_enabled;
> > +};
> > +
> > +/* intel_display_power.c */
> 
> I've put a lot of effort into splitting our (display) codebase towards
> having a 1-to-1 mapping between .c and .h files. This patch adds an odd
> split between two headers and two compilation units, and I don't think
> it's pretty.

This header includes struct definitions used by intel_display_power.c
and intel_display_power_map.c. I don't see why this would be a problem,
there are many other cases where multiple .c files include a header file
for the same reason.

> > +extern const struct i915_power_well_ops i9xx_always_on_power_well_ops;
> > +extern const struct i915_power_well_ops chv_pipe_power_well_ops;
> > +extern const struct i915_power_well_ops chv_dpio_cmn_power_well_ops;
> > +extern const struct i915_power_well_ops i830_pipes_power_well_ops;
> > +extern const struct i915_power_well_ops hsw_power_well_ops;
> > +extern const struct i915_power_well_ops hsw_power_well_ops;
> > +extern const struct i915_power_well_ops gen9_dc_off_power_well_ops;
> > +extern const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops;
> > +extern const struct i915_power_well_ops vlv_display_power_well_ops;
> > +extern const struct i915_power_well_ops vlv_dpio_cmn_power_well_ops;
> > +extern const struct i915_power_well_ops vlv_dpio_power_well_ops;
> > +extern const struct i915_power_well_ops icl_ddi_power_well_ops;
> > +extern const struct i915_power_well_ops icl_aux_power_well_ops;
> > +extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
> 
> Also not happy about this. Data is not an interface.
> 
> We currently have 20 symbols with extern, and this adds 14 more with a
> clear path to add more for new platforms. I'd rather we were heading in
> the other direction.
> 
> I'm just wondering if the split introduced here is sound. All of the
> above would make this turn up when I look for stuff that I think needs
> to be refactored. And the commit message does not even say why...

The reason is to reduce the size of intel_display_power.c, to make it
more readable/manageable. The implementation of the power well
enable/disable etc. functionality and the mapping of these power wells
to power domains are two distinct parts in that file that can be
separated.

The externs above are power wells that are mapped to domains, and
besides the symbol name are opaque to the mapping code.

> BR,
> Jani.
> 
> 
> > +
> > +/* intel_display_power_map.c */
> > +int intel_init_power_wells(struct i915_power_domains *power_domains);
> > +void intel_cleanup_power_wells(struct i915_power_domains *power_domains);
> > +
> > +#endif
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center