Re: [PATCH 15/19] drm/i915: Simplify cdclk_disable_noatomic()

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

On Tue, Mar 04, 2025 at 03:04:07PM +0000, Govindapillai, Vinod wrote:
> On Tue, 2025-02-18 at 23:19 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Instead of hand rolling the cdclk state disabling for a
> > pipe in noatomic() let's just recompute the whole thing
> > from scratch. Less code we have to remember to keep in sync.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 62caee4a8b64..2a8749a0213e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -3364,13 +3364,8 @@ void intel_cdclk_update_hw_state(struct intel_display *display)
> >  void intel_cdclk_crtc_disable_noatomic(struct intel_crtc *crtc)
> >  {
> >  	struct intel_display *display = to_intel_display(crtc);
> > -	struct intel_cdclk_state *cdclk_state =
> > -		to_intel_cdclk_state(display->cdclk.obj.state);
> > -	enum pipe pipe = crtc->pipe;
> >  
> > -	cdclk_state->min_cdclk[pipe] = 0;
> > -	cdclk_state->min_voltage_level[pipe] = 0;
> > -	cdclk_state->active_pipes &= ~BIT(pipe);
> > +	intel_cdclk_update_hw_state(display);
> >  }
> >  
> 
> Okay! Now I see that passing active_pipes to intel_cdclk_update_hw_state() as I commented in one of
> the earlier patch wont work!
> 
> But isnt this bit efficient, as we will be calling,  intel_cdclk_crtc_disable_noatomic() (and
> intel_cdclk_update_hw_state()) for_each_intel_crtc_in_pipe_mask(), we end up executing
> intel_cdclk_update_hw_state() redundantly?
> 
> Instead should we extract intel_cdclk_update_crtc_hw_state() from  intel_cdclk_update_hw_state() 
> and use that here?

We'll just do these once during driver for the typical case, and
only if have to sanitize off some pipes then we'll potentially
do them extra times. Performance doesn't matter at that point,
so the simpler it all is the better.

-- 
Ville Syrjälä
Intel