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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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.

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

-- 
Ville Syrjälä
Intel