[Intel-gfx] [RFC] drm/i915/dp: Preliminary support for 2 pipe 1 port mode for 5K at 120

[Manasi Navare]

Hi Ville/Maarten/Matt,

Thanks for all your comments, please see my comments/concerns below

On Wed, Jan 23, 2019 at 06:58:22PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote:
> > On Gen 11 platform, to enable resolutions like 5K at 120 where
> > the pixel clock is greater than pipe pixel rate, we need to split it across
> > 2 pipes and enable it using DSC and big joiner. In order to support this
> > dual pipe single port mode, we need to link two crtcs involved in this
> > ganged mode.
> > 
> > This patch is a RFC patch that links two crtcs using linked_crtc pointer
> > in intel_crtc_state and slave to indicate if the crtc is a master or slave.
> > Here the HW necessitates the first CRTC to be the master CRTC through which
> > the final output will be driven and the next consecutive CRTC should be
> > slave crtc.
> > 
> > This is currently not tested, but I wanted to get some inputs on this approach.
> > The idea is to follow the same approach used in Ganged plane mode for NV12
> > planes.
> 
> This doesn't actually do much so not quite sure what I'm supposed to
> say.
> 
> The more I've thought about the two pipe one port case the more I'm
> thinking we'll need to do something like:
> 
> struct drm_foo_uapi_state {
> 	/* all the uapi visible state for the obj */
> };
> 
> struct drm_foo_state {
> 	struct drm_foo_uapi_state uapi;
> 
> 	/*
> 	 * derived state, including duplicates of the pieces
> 	 * of the uapi state we need to massage when stealing
> 	 * planes/crtcs for internal uses.
> 	 */
> };

> 
> And then change all the uapi facing code to play with the uapi stuff
> only, and find out where we need to do the uapi->derived copy for
> planes/crtcs actually enabled by userspace.

It makes sense to have a subset of fields in a separate struct that is uapi
facing and that would the values set by and exposed to userspace for Eg the mode
while have the rest in a derived state that would be essentially map to half
of the mode being sent on each crtc, half of the plane etc.

But one thing that I am not clear on is why do we need this separation at the drm level?
Initial design discussion we had we concluded that drm_stomic_state and drm_crtc_state would
not change while the intel_crtc_state will be state where we can have intel_crtc_uapi_state struct
with all the uapi visible fields. And the remaining fields outside of that will be the ones
derived for half the mode, half the planes etc for each of the 2 crtcs involved in the modeset.

The uapi visible struct in intel_crtc_state will remain same across both these crtcs and the slave
crtc configurations can be done using the master intel_crtc_uapi_state.

What do you think of this approach or I am missing something here because of which you feel that
having these two states at drm level would be better?

Manasi

> 
> Embedding it like that means all the state iterators etc. will keep
> working, and we don't have to change all the driver code playing
> around with the state. Ie. changes would be limited to mostly the
> uapi facing code.
>
 
> We didn't have to do this for the nv12 thing because we got lucky
> and the difference between the two planes turned out to be minimal.
> But in the two pipe case we have massage much more state.
> 
> > 
> > Suggested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>,
> > Matt Roper <matthew.d.roper at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 +++
> >  2 files changed, 69 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2fa9f4aec08e..9910dad7371b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10900,6 +10900,63 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
> >  	return true;
> >  }
> >  
> > +static bool icl_dual_pipe_mode(struct drm_i915_private *dev_priv,
> > +			       struct drm_crtc_state *crtc_state)
> > +{
> > +	if (crtc_state->mode.clock <= 2 * dev_priv->max_cdclk_freq)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static int icl_set_linked_crtcs(struct drm_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct intel_crtc_state *linked_state = NULL;
> > +	struct intel_crtc *slave_crtc = NULL;
> > +	int i;
> > +
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(crtc_state);
> > +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +		if (crtc_state->active)
> > +			continue;
> > +
> > +		if (!icl_dual_pipe_mode(dev_priv, crtc_state))
> > +			continue;
> > +
> > +		if (!pipe_config->linked_crtc) {
> > +			slave_crtc = intel_get_crtc_for_pipe(dev_priv,
> > +							     intel_crtc->pipe + 1);
> > +			if (!slave_crtc)
> > +				return PTR_ERR(slave_crtc);
> > +
> > +			linked_state = intel_atomic_get_crtc_state(state, slave_crtc);
> > +			if (IS_ERR(linked_state))
> > +				return PTR_ERR(linked_state);
> > +
> > +			pipe_config->linked_crtc = slave_crtc;
> > +			pipe_config->slave = false;
> > +			linked_state->linked_crtc = intel_crtc;
> > +			linked_state->slave = true;
> > +			// Update the intel_state->active_crtcs if needed
> > +
> > +			DRM_DEBUG_KMS("Using [CRTC:%d:%s] as master and [CRTC:%d:%s] as slave\n",
> > +				      intel_crtc->base.base.id, intel_crtc->base.name,
> > +				      slave_crtc->base.base.id, slave_crtc->base.name);
> > +
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +}
> > +
> >  static int icl_add_linked_planes(struct intel_atomic_state *state)
> >  {
> >  	struct intel_plane *plane, *linked;
> > @@ -12702,6 +12759,12 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 11) {
> > +		ret = icl_set_linked_crtcs(state);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
> >  		struct intel_crtc_state *pipe_config =
> >  			to_intel_crtc_state(crtc_state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 33b733d37706..f8bbed525ec3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -959,6 +959,12 @@ struct intel_crtc_state {
> >  
> >  	/* Forward Error correction State */
> >  	bool fec_enable;
> > +
> > +	/* Pointer to linked crtc in dual pipe mode */
> > +	struct intel_crtc *linked_crtc;
> > +
> > +	/* Flag to indicate whether this crtc is master or slave */
> > +	bool slave;
> >  };
> >  
> >  struct intel_crtc {
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel