Re: [PATCH v2 15/17] drm/gma500: Stop using mode->private_flags

From: Sam Ravnborg <sam@ravnborg.org>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	Emil Velikov <emil.l.velikov@gmail.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 15/17] drm/gma500: Stop using mode->private_flags
Date: Thu, 9 Apr 2020 22:47:43 +0200	[thread overview]
Message-ID: <20200409204743.GA29379@ravnborg.org> (raw)
In-Reply-To: <20200409194952.GZ6112@intel.com>

Hi Ville.

> > > > > index 264d7ad004b4..9e88a37f55e9 100644
> > > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> > > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo {
> > > > >  	/* DDC bus used by this SDVO encoder */
> > > > >  	uint8_t ddc_bus;
> > > > >  
> > > > > +	u8 pixel_multiplier;
> > > > > +
> > > > 
> > > > There is really no good reason to use an u8 here.
> > > 
> > > Wastes less space.
> > 
> > When there is a good reason - use the size limited variants.
> > But in this use case there is no reason to space optimize it.
> 
> IMO when it's stuffed into a structure there's no reason not to
> optimize it. At some point it all starts to add up.
> 
> At least i915 suffers a lot from bloated structures (dev_priv
> and atomic state structs being the prime examples) where we
> could probably shave dozens if not hundreds of bytes if
> everything just used the smallest type possible. In fact
> this series does shave dozens of bytes from the crtc state
> alone.

There is a difference between a structure used many times -
And a structure used once or only a few times.
If everyone started to optimize the types used, then we
would end up with code that is hard to maintain.

The point here is that we have a structure allocated maybe
once and a field assinged from a int - which using integer promotion
is then stuffed into an u8. If we one day start to be clever and
use values above 255 we need to find all the places where a
u8 was used to optimize size of some random struct.

If this was a struct instantiated many times and used all over
the story was another - but thats not the case here.
Here the principle of least suprises hold - do not change the type.

I try to explain the rationale behind the argument to use int.
Feel free to disagree.

> 
> > 
> > When in the slightly pedantic mode, using u8 is not consistent.
> > ddc_bus defined above usese uint8_t.
> 
> u8 & co. are preferred in kernel code. Checkpatch even complains when
> you use the stdint types. The uint8_t here is some old leftovers.

Mixing coding practice makes code less readable, no matter
the output of checkpatch.
The right fix would be to update gma500 to migrate away from the
stdint types. But that would be a sepearte patch for another day.

My orginal feedback on the patch has not changed.
Feel free to move forward with the patch without my r-b.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel