Re: [PATCH v3 1/6] drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync

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

On Mon, Sep 30, 2019 at 11:37:41AM -0700, Lucas De Marchi wrote:
> On Sun, Sep 22, 2019 at 10:08:02AM -0700, Manasi Navare wrote:
> >In case of tiled displays when the two tiles are sent across two CRTCs
> >over two separate DP SST connectors, we need a mechanism to synchronize
> >the two CRTCs and their corresponding transcoders.
> >So use the master-slave mode where there is one master corresponding
> >to last horizontal and vertical tile that needs to be genlocked with
> >all other slave tiles.
> >This patch identifies saves the master transcoder in all the slave
> >CRTC states. This is needed to select the master CRTC/transcoder
> >while configuring transcoder port sync for the corresponding slaves.
> >
> >v4:
> >* Rebase
> >v3:
> >* Use master_tramscoder instead of master_crtc for valid
> >HW state readouts (Ville)
> >v2:
> >* Move this to intel_mode_set_pipe_config(Jani N, Ville)
> >* Use slave_bitmask to save associated slaves in master crtc state (Ville)
> >
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >Cc: Matt Roper <matthew.d.roper@intel.com>
> >Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_display.c  | 123 ++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_display.h  |   3 +
> > .../drm/i915/display/intel_display_types.h    |   6 +
> > 3 files changed, 132 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >index c05ba6af6226..4ff375d5852d 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >@@ -521,6 +521,24 @@ needs_modeset(const struct intel_crtc_state *state)
> > 	return drm_atomic_crtc_needs_modeset(&state->base);
> > }
> >
> >+bool
> >+is_trans_port_sync_mode(struct drm_i915_private *dev_priv,
> >+			const struct intel_crtc_state *state)
> 
> on TGL we now also need a master transcoder for DP-MST. I'm wondering if
> we couldn't reuse the same mechanism so we would dissociate a little bit
> the port_sync_mode from saving or searching for a master transcoder
> in crtc_state.

I think we want to track them separately to make the state checker etc.
robust. But I do think we should likely use the same logic for picking
all masters, and I think that logic should probably just be
"lowest numbered pipe is the master".

> 
> >@@ -12369,6 +12478,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
> > 			      CRTC_STEREO_DOUBLE);
> >
> >+	/* Set the crtc_state defaults for trans_port_sync */
> >+	pipe_config->master_transcoder = INVALID_TRANSCODER;
> 
> could we get away with the INVALID_TRANSCODER by simply making
> pipe_config->master_transcoder = pipe_config->cpu_transcoder?

That would degrade the state checker I think. Ie. we could
accidentally program the transcoder to be its own master and wouldn't
even notice. We would also need to add extra logic to check
for this when programming things and that would just result in
more wtfs when reading the code.

> 
> then we can always make sure it's assigned to something valid
> and use it in the cases it makes sense (port sync mode and dp-mst).
> 
> Lucas De Marchi
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx