Re: [PATCH 10/12] drm/i915: Eliminate bigjoiner boolean

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

On 2/16/2022 4:34 PM, Ville Syrjälä wrote:
> On Wed, Feb 16, 2022 at 04:27:49PM +0530, Nautiyal, Ankit K wrote:
> > On 2/16/2022 12:02 AM, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Since we now have the bigjoiner_pipes bitmask the boolean
> > > is redundant. Get rid of it.
> > >
> > > Also, populating bigjoiner_pipes already during
> > > encoder->compute_config() allows us to use it much earlier
> > > during the state calculation as well. The initial aim is
> > > to use it in intel_crtc_compute_config().
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
> > >    drivers/gpu/drm/i915/display/intel_display.c  | 50 ++++++++-----------
> > >    .../drm/i915/display/intel_display_debugfs.c  |  2 +-
> > >    .../drm/i915/display/intel_display_types.h    |  3 --
> > >    drivers/gpu/drm/i915/display/intel_dp.c       | 13 ++---
> > >    drivers/gpu/drm/i915/display/intel_vdsc.c     |  8 +--
> > >    .../drm/i915/display/skl_universal_plane.c    |  2 +-
> > >    7 files changed, 36 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > index 1f448f4e9aaf..da6cf0515164 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > @@ -640,7 +640,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> > >    	 * FIXME bigjoiner fastpath would be good
> > >    	 */
> > >    	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
> > > -	    crtc_state->update_pipe || crtc_state->bigjoiner)
> > > +	    crtc_state->update_pipe || crtc_state->bigjoiner_pipes)
> > >    		goto slow;
> > >    
> > >    	/*
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 47b5d8cc16fd..192474163edb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1926,7 +1926,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >    	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > >    		return;
> > >    
> > > -	if (!new_crtc_state->bigjoiner) {
> > > +	if (!new_crtc_state->bigjoiner_pipes) {
> > >    		intel_encoders_pre_pll_enable(state, crtc);
> > >    
> > >    		if (new_crtc_state->shared_dpll)
> > > @@ -2727,7 +2727,7 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> > >    static void intel_bigjoiner_adjust_timings(const struct intel_crtc_state *crtc_state,
> > >    					   struct drm_display_mode *mode)
> > >    {
> > > -	if (!crtc_state->bigjoiner)
> > > +	if (!crtc_state->bigjoiner_pipes)
> > >    		return;
> > >    
> > >    	mode->crtc_clock /= 2;
> > > @@ -2811,7 +2811,7 @@ static void intel_bigjoiner_compute_pipe_src(struct intel_crtc_state *crtc_state
> > >    {
> > >    	int width, height;
> > >    
> > > -	if (!crtc_state->bigjoiner)
> > > +	if (!crtc_state->bigjoiner_pipes)
> > >    		return;
> > >    
> > >    	width = drm_rect_width(&crtc_state->pipe_src);
> > > @@ -4218,7 +4218,6 @@ static void intel_bigjoiner_get_config(struct intel_crtc_state *crtc_state)
> > >    	if (((master_pipes | slave_pipes) & BIT(pipe)) == 0)
> > >    		return;
> > >    
> > > -	crtc_state->bigjoiner = true;
> > >    	crtc_state->bigjoiner_pipes =
> > >    		BIT(get_bigjoiner_master_pipe(pipe, master_pipes, slave_pipes)) |
> > >    		get_bigjoiner_slave_pipes(pipe, master_pipes, slave_pipes);
> > Although not part of this patch, do we need to check if
> > get_bigjoiner_master_pipe() does not give PIPE_INVALID?
> >
> > Perhaps in a case where master_pipe is read as 0 but some garbage for
> > slave_pipes during readout?
> >
> > Should there be a check for INVALID_PIPE, before feeding into BIT() macro?
> I think if we want to do more thourough validation against totally bogus
> hardware programming then we should just do it once at the start.
> enabled_bigjoiner_pipes() does have something, but it's only good for
> the two joined pipes cases. Also it just warns and doesn't do anything
> more than that atm. The simple option might be to make it just zero out
> the masks entirely if they look totally bogus. The readout would then
> be skipped for all slave pipes.

Yes you are right, enabled_bigjoiner_pipes does have a check in the end 
and that will prevent bogus value to

a certain extent. Given case would not occur, atleast for two joined 
pipes case.

Anyways, the patch seems to be straight forward and looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>