[PATCH v2] drm/xe/hwmon: expose fan speed

[Rodrigo Vivi]

On Fri, Mar 07, 2025 at 08:07:44PM +0200, Raag Jadav wrote:
> On Fri, Mar 07, 2025 at 07:58:26PM +0200, Raag Jadav wrote:
> > On Fri, Mar 07, 2025 at 09:33:05AM -0500, Rodrigo Vivi wrote:
> > > On Fri, Mar 07, 2025 at 06:21:12PM +0530, Raag Jadav wrote:
> > 
> > ...
> > 
> > > > +	case REG_FAN_SPEED:
> > > > +		if (xe->info.platform == XE_BATTLEMAGE || xe->info.platform == XE_DG2) {
> > > 
> > > we should probably have a has_fan_control flag in the platform definition struct so you
> > > don't need to do platform checks here and it gets easier when we need to add new platforms.
> > 
> > My experience with struct level flags has mostly been with high stakes
> > features which require maintaining a state (usually atomic), and I'm not
> > sure if fan control is worth having one of them.
> 
> I know it's always taken on a lighter note in drm but we might never
> know the cost of it overtime.
> 
> https://lore.kernel.org/netdev/20231129072756.3684495-1-lixiaoyan@google.com/

We are talking about a 1-bit flag here, that is not passed over network.

Please check xe_pci_types.h

My experience with enabling new platforms is that you will end up forgetting
to cover all the if else of the previous ones and end up invalid memory
access on the new platform and realize that too late.

We try to limit the platform check for a very specific case where that
case is on that platform only and wont be extended to future platforms.

Otherwise we tend to use the graphics_ver >= X.X if we know the future
will consider this a legacy feature.

But in cases like this where the intermidiate version doesn't have the
feature and in the future we might have platforms with and without this
feature, then has_flag:1 it is.

> 
> Raag