Re: [PATCH 4/9] drm/i915: Introduce a new intel_atomic_check_crtcs()

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

On Thu, Oct 23, 2025 at 03:21:49PM +0300, Luca Coelho wrote:
> On Fri, 2025-10-17 at 19:14 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a new intel_atomic_check_crtcs() that gets called fairly
> > early during intel_atomic_check() and start collecting stuff
> > into it from elsewhere. For now we can suck in the
> > intel_crtc_min_cdclk() stiff.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 89f63c3b1ecd..211761c5b72a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5746,6 +5746,18 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  		new_crtc_state->update_pipe = true;
> >  }
> >  
> > +static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> 
> Minor nitpick: this doesn't seem to be _checking_ crtcs, but assigning
> min_cdclk, so the function name is a bit misleading? Or are you
> planning to add more actual checks here?

The whole foo_check() naming pretty much comes from the fact
that it all gets called from the .atomic_check() hook
(intel_atomic_check() for us).

We'be been more or less moving towards just calling various
intel_foo_atomic_check() functions from there, and having those
do whatever they need to do. So perhaps I should have called
these intel_crtc_atomic_check*() as well, but that name was
already taken by the single crtc variant (what is now
becoming intel_crtc_atomic_check_late()).

Yeah, naming is hard.

> 
> Of course it aligns with the _check_crtcs_late() that you just added,
> so regardless of whether you decide to change this:
> 
> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> 
> --
> Cheers,
> Luca.
> 
> 
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> > +		new_crtc_state->min_cdclk = intel_crtc_min_cdclk(new_crtc_state);
> > +
> > +	return 0;
> > +}
> > +
> >  static int intel_atomic_check_crtcs_late(struct intel_atomic_state *state)
> >  {
> >  	struct intel_display *display = to_intel_display(state);
> > @@ -6449,8 +6461,9 @@ int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> > -		new_crtc_state->min_cdclk = intel_crtc_min_cdclk(new_crtc_state);
> > +	ret = intel_atomic_check_crtcs(state);
> > +	if (ret)
> > +		goto fail;
> >  
> >  	ret = intel_compute_global_watermarks(state);
> >  	if (ret)

-- 
Ville Syrjälä
Intel