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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 04/19] drm/i915: Move the power domain->well mappings to intel_display_power_map.c
Date: Tue, 01 Feb 2022 12:53:13 +0200	[thread overview]
Message-ID: <87iltyvox2.fsf@intel.com> (raw)
In-Reply-To: <20220131160022.GA2434344@ideak-desk.fi.intel.com>

On Mon, 31 Jan 2022, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Jan 31, 2022 at 02:15:25PM +0200, Jani Nikula wrote:
>> On Fri, 28 Jan 2022, Imre Deak <imre.deak@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@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.

Declaring types is one thing, but I'd like to have the symbols defined
in intel_foo.c be declared in intel_foo.h, and named intel_foo_*. And by
symbols I mostly mean functions, preferrably nothing else.

IOW, if you have stuff in intel_display_power_map.c, add
intel_display_power_map.h that describes the interface to that file.

>
>> > +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.

I understand the mapping is a complicated and kind of separate part of
it all. But if I put that aside for a bit, I think I'd consider putting
the abstraction boundary at struct i915_power_well_desc and everything
within.

What would the code look like if struct i915_power_well_desc and
subsequently struct i915_power_well_ops were opaque pointers and split
out of current intel_display_power.c to a separate file? Add functions
to access everything in desc and to call the ops.

Just splitting out the mapping still leaves high and low level code in
the same file, and I think intel_display_power.c would be clarified a
great deal more if the split was between them instead.

Also, intel_display_power_* functions in the file are a kind of separate
set of functionality from the intel_power_domains_* functions. I think
that's a clear candidate for a split too. There's also the dbuf stuff
that probably belongs somewhere else (lots of it in intel_pm.c but
that's another rabbit hole).

Long story short, I think there are other, IMHO cleaner, splits to be
made in intel_display_power.c that should have priority. And they don't
block us from splitting the mapping as follow-up later, but I think that
should not be the first thing.

BR,
Jani.

>
>> 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

-- 
Jani Nikula, Intel Open Source Graphics Center