Re: [PATCH 14/15] drm/i915/pfit: Account for pfit downscaling in pixel rate on GMCH platforms

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 12 Nov 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 12, 2026 at 10:47:47AM +0100, Michał Grzelak wrote:
>> On Mon, Oct 19, 2026 at 04:53:08PM GMT, Ville Syrjala wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > 
>> > Now that we have crtc_state->pch.dst populated on GMCH platforms
>> > we can use the existing ILK+ code to calculate the downscale
>> > adjusted pixel rate correctly on GMCH platforms.
>> > 
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_display.c | 23 +++++++-------------
>> >  1 file changed, 8 insertions(+), 15 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > index 200426bf0c59..6b813efff955 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -2278,7 +2278,8 @@ static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
>> >  		(crtc->pipe == PIPE_A || display->platform.i915g);
>> >  }
>> >  
>> > -static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
>> > +static unsigned int
>> 
>> Is there any consensus on when to use u32 and when unsigned int? I see
>> that here it is because of pixel_rate_cdclk being defined as unsigned
>> int, but asking for general guidance.
>
> My general rules are roughly this:
> 1. Just plain old 'int' for most things
> 2. Use 'unsigned int' for some address/offset calculations and whatnot
>    where negative values are never a thing
> 3. u32 for hardware registers and so forth

Agreed.

I'd further emphasize to use the explicitly sized types when a specific
size constraint exists. Like you say, hardware, binary formats.

If you're thinking "the value range always fits in n bits", it's almost
invariably the wrong reason to use a sized type. It's usually the wrong
optimization, it's the wrong way to limit the result. There are some
cases where you do want to reduce the size of a type just because of the
memory footprint (think big arrays, widely used structs), but they need
to be justified.

Similarly, using an unsigned type to gain an extra bit is also almost
invariably the wrong thing to do. Use a bigger type instead. But there
are exceptions, you need to know what you're doing, and you need to
justify it.

Outside of the above guidelines, I think choosing when to use unsigned
without the explicit size is the hardest one.

> The pixel rate stuff should perhaps just all be 'int', but that
> would probably need to double check a few things before doing that.
>
> The one slight annoyance with signed ints is that stupid static
> analyzers may start to nag you about unchecked negative values
> even if they're not actually possible.
>
> But a signed int is almost always the safer option in case you end
> up using it in some arithmetic expression where negative results are
> a possibility. Even a single 'unsigned int' there will probably 
> convert the whole expression to unsigned due to C rules on integer
> promotion/conversion, and then you negative result will become a
> really large positive result.

Yeah, I think the whole "can't be negative" argument is usually trading
it for "can't be really large positive either". Usually you're just
better off checking the negatives. Especially when you don't really have
an upper bound, and you have to figure out an arbitrary upper bound just
to check for things that could've been obvious checks for negative
values.

HTH,
Jani.


>
>> 
>> Since that's a side question:
>> 
>> Reviewed-by: Michał Grzelak <michal.grzelak@linux.intel.com>
>> 
>> BR,
>> Michał
>> 
>> > +intel_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
>> >  {
>> >  	u32 pixel_rate = crtc_state->hw.pipe_mode.crtc_clock;
>> >  	struct drm_rect src;
>> > @@ -2299,7 +2300,8 @@ static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
>> >  				   pixel_rate);
>> >  }
>> >  
>> > -static u32 ilk_pipe_pixel_rate_cdclk(const struct intel_crtc_state *crtc_state)
>> > +static unsigned int
>> > +intel_pipe_pixel_rate_cdclk(const struct intel_crtc_state *crtc_state)
>> >  {
>> >  	struct intel_display *display = to_intel_display(crtc_state);
>> >  	u32 pixel_rate = crtc_state->hw.pipe_mode.crtc_clock;
>> > @@ -2345,19 +2347,10 @@ static void intel_mode_from_crtc_timings(struct drm_display_mode *mode,
>> >  
>> >  static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
>> >  {
>> > -	struct intel_display *display = to_intel_display(crtc_state);
>> > -
>> > -	if (HAS_GMCH(display)) {
>> > -		/* FIXME calculate proper pipe pixel rate for GMCH pfit */
>> > -		crtc_state->pixel_rate =
>> > -			crtc_state->hw.pipe_mode.crtc_clock;
>> > -		crtc_state->pixel_rate_cdclk = crtc_state->pixel_rate;
>> > -	} else {
>> > -		crtc_state->pixel_rate =
>> > -			ilk_pipe_pixel_rate(crtc_state);
>> > -		crtc_state->pixel_rate_cdclk =
>> > -			ilk_pipe_pixel_rate_cdclk(crtc_state);
>> > -	}
>> > +	crtc_state->pixel_rate =
>> > +		intel_pipe_pixel_rate(crtc_state);
>> > +	crtc_state->pixel_rate_cdclk =
>> > +		intel_pipe_pixel_rate_cdclk(crtc_state);
>> >  }
>> >  
>> >  static void intel_joiner_adjust_timings(const struct intel_crtc_state *crtc_state,
>> > -- 
>> > 2.55.0
>> > 
>> > 

-- 
Jani Nikula, Intel