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

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

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@linux.intel.com>
> >
> > 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@linux.intel.com>
> > ---
> >   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.

-- 
Ville Syrjälä
Intel