| Message ID | 20190122211207.20055-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915/dp: Preliminary support for 2 pipe 1 port mode for 5K@120 | expand |
Op 22-01-2019 om 22:12 schreef Manasi Navare: > On Gen 11 platform, to enable resolutions like 5K@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. > > Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, > Matt Roper <matthew.d.roper@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@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 { Hey, Looks good for a first iteration, and I think with some tweaks it will work well. The link should be broken in the first for_each_oldnew_crtc_in_state loop in intel_atomic_check, after needs_modeset() is evaluated to true, but before the early return on !enable. After that we should call icl_set_linked_crtcs before the if (any_ms) branch, making sure we try to re-establish to the same if needs_modeset() == false, but note that the linked pipe might not be available, in which case we need to unset crtc_state->update_pipe and set crtc_state->mode_changed again to establish the new mapping and undo the fastset. It sounds complicated, but it will be similar to the nv12 planes mapping, except only on modeset, with special code for undoing fastset. Your code may also not work if we try to set a 5k120 mode on the last pipe, it will try to get a nonexistent pipe or dereference random memory, it might be better to create a separate get_dual_pipe_crtc() function which returns the correct crtc or NULL. ~Maarten
On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote: > On Gen 11 platform, to enable resolutions like 5K@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. 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@linux.intel.com>, > Matt Roper <matthew.d.roper@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@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
On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote: > On Gen 11 platform, to enable resolutions like 5K@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. > > Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, > Matt Roper <matthew.d.roper@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Looks like the right general approach of using slave/master. I agree with Ville and Maarten's feedback as well. The other thing we're going to need to worry about is dealing with all the planes on the two crtc's. Suppose we have a userspace-visible state of: Pipe A: off Pipe B: 5000x2000@120 (using round numbers to make example simpler) Plane 0: RGB32: pos (0,0), width=5000, height=2000 Plane 1: RGB32: pos (100,100), width=100, height=100 Plane 2: RGB32: pos (4800,1800), width=100, height=100 Plane 3: RGB32: pos (0,0), width=3000, height=2000 Plane 4: NV12: pos (2000,0), width=1000, height=2000 Plane 5: off Plane 6: off Pipe C: off this means that we need to grab extra planes on the other CRTC and adjust their size, position, and/or surface offsets accordingly. So the internal driver state that we actually program into the hardware needs to be something like: Pipe A: off Pipe B: 2500x1000 (master) Plane 0: pos (0,0), width=2500, height=2000 Plane 1: pos (100,100), width=100, height=100 Plane 2: off Plane 3: pos (0, 0), width=2500, height=2000 Plane 4{UV}: pos (2000, 0), width=500, height=2000 Plane 5{Y}: pos (2000, 0), width=500, height=2000 Plane 6: off Pipe C: 2500x1000 (slave) Plane 0: pos (0,0), offset=(2500,0), width=2500, height=2000 Plane 1: off Plane 2: pos (2300,1800), width=100, height=100 Plane 3: pos (0, 0), offset=(2500, 0), width=500, height=2000 Plane 4{UV}: pos (0, 0), offset=(500,0), width=500, height=2000 Plane 5{Y}: pos (0, 0), offset=(500,0), width=500, height=2000 Plane 6: off So I think Ville is right; we're going to need to really copy a lot of the userspace-facing state data into our own internal state structures so that we can do the various adjustments on it. As you can see above there are cases (2pi1po + nv12) where a single userspace plane request translates into us programming four different sets of plane register values. Matt > --- > 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 >
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@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@linux.intel.com>, > > Matt Roper <matthew.d.roper@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@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
Op 23-01-2019 om 18:31 schreef Matt Roper: > On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote: >> On Gen 11 platform, to enable resolutions like 5K@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. >> >> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, >> Matt Roper <matthew.d.roper@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Looks like the right general approach of using slave/master. I agree > with Ville and Maarten's feedback as well. > > The other thing we're going to need to worry about is dealing with all > the planes on the two crtc's. Suppose we have a userspace-visible state > of: > > Pipe A: off > Pipe B: 5000x2000@120 (using round numbers to make example simpler) > Plane 0: RGB32: pos (0,0), width=5000, height=2000 > Plane 1: RGB32: pos (100,100), width=100, height=100 > Plane 2: RGB32: pos (4800,1800), width=100, height=100 > Plane 3: RGB32: pos (0,0), width=3000, height=2000 > Plane 4: NV12: pos (2000,0), width=1000, height=2000 > Plane 5: off > Plane 6: off > Pipe C: off > > this means that we need to grab extra planes on the other CRTC and > adjust their size, position, and/or surface offsets accordingly. So the > internal driver state that we actually program into the hardware needs > to be something like: > > Pipe A: off > Pipe B: 2500x1000 (master) > Plane 0: pos (0,0), width=2500, height=2000 > Plane 1: pos (100,100), width=100, height=100 > Plane 2: off > Plane 3: pos (0, 0), width=2500, height=2000 > Plane 4{UV}: pos (2000, 0), width=500, height=2000 > Plane 5{Y}: pos (2000, 0), width=500, height=2000 > Plane 6: off > Pipe C: 2500x1000 (slave) > Plane 0: pos (0,0), offset=(2500,0), width=2500, height=2000 > Plane 1: off > Plane 2: pos (2300,1800), width=100, height=100 > Plane 3: pos (0, 0), offset=(2500, 0), width=500, height=2000 > Plane 4{UV}: pos (0, 0), offset=(500,0), width=500, height=2000 > Plane 5{Y}: pos (0, 0), offset=(500,0), width=500, height=2000 > Plane 6: off > > So I think Ville is right; we're going to need to really copy a lot of > the userspace-facing state data into our own internal state structures > so that we can do the various adjustments on it. As you can see above > there are cases (2pi1po + nv12) where a single userspace plane request > translates into us programming four different sets of plane register > values. Yeah I fear you're right. Lets see, we would need to sanitize the driver big time.. struct drm_crtc_state { struct drm_crtc *crtc; bool enable; bool active; bool planes_changed : 1; bool mode_changed : 1; bool active_changed : 1; bool connectors_changed : 1; bool zpos_changed : 1; // unused bool color_mgmt_changed : 1; bool no_vblank : 1; //unused u32 plane_mask; u32 connector_mask; u32 encoder_mask; struct drm_display_mode adjusted_mode; struct drm_display_mode mode; struct drm_property_blob *mode_blob; struct drm_property_blob *degamma_lut; struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut; u32 target_vblank; // unused u32 pageflip_flags; // unused struct drm_pending_vblank_event *event; struct drm_crtc_commit *commit; struct drm_atomic_state *state; }; First the good, it looks like we can safely re-use the following in the slave, without affecting userspace: struct drm_crtc * crtc; u32 last_vblank_count; // ignored in i915 struct drm_display_mode adjusted_mode; // sort of handled by core, but on disable we can write our own mode here struct drm_pending_vblank_event * event; // handled by core struct drm_atomic_state * state; // handled by core Then one we might use inside i915, but should not inside the driver: struct drm_display_mode mode; -> use crtc_state.adjusted_mode.. Then the bad, we can easily get from the master safely. We only need to add special handling in intel_color.c: struct drm_property_blob * degamma_lut; struct drm_property_blob * ctm; struct drm_property_blob * gamma_lut; Also bad, u32 plane_mask; // See below. u32 connector_mask; // Hmm, I don't think we care about connectors on slave, do we? Just special handling in the modeset calculations needed u32 encoder_mask; // Handled by core, but probably fine to leave as 0. Can't touch this. Don't need to either. bool planes_changed:1; bool mode_changed:1; bool active_changed:1; bool connectors_changed:1; bool color_mgmt_changed:1; Whatever for most of those, can ignore the masks probably. Just make sure master plane state checks the slave plane state as well, and ignore direct checks of slave plane state. Because this is a 1:1 mapping, it should be fine. And finally, the ugly: bool enable; bool active; The biggest issues are crtc_state->active and crtc_state->enable. i915 uses it a lot, and even the core makes a lot of decisions based on it. We should probably only steal crtc's that doesn't have crtc_state->enable set. But one problem at a time.. plane_state is relatively easy, because even if we can't touch plane_mask and the atomic details for the planes, we need to derive this from the master plane check function anyway. Fortunately we have our own intel_crtc_state->active_planes already used in the plane update functions we just need to be sure that the planes are part of the state. We should probably completely ignore the slave planes in the plane atomic check called from the core when it's used as a slave.. Once for plane_state: struct drm_plane_state { struct drm_plane *plane; struct drm_crtc *crtc; struct drm_framebuffer *fb; struct dma_fence *fence; int32_t crtc_x; int32_t crtc_y; uint32_t crtc_w, crtc_h; uint32_t src_x; uint32_t src_y; uint32_t src_h, src_w; u16 alpha; uint16_t pixel_blend_mode; unsigned int rotation; unsigned int zpos; unsigned int normalized_zpos; enum drm_color_encoding color_encoding; enum drm_color_range color_range; struct drm_rect src, dst; bool visible; struct drm_crtc_commit *commit; struct drm_atomic_state *state; }; Good: struct drm_plane *plane; struct dma_fence *fence; // must be copied from master plane_state struct drm_rect src, dst; // can fill in what we want here.. bool visible; // ignored by core mostly, but set and used by driver unsigned int zpos; unsigned int normalized_zpos; struct drm_crtc_commit *commit; struct drm_atomic_state *state; Bad: // We don't use the below anyway, we must use the src and dst rects.. int32_t crtc_x; int32_t crtc_y; uint32_t crtc_w, crtc_h; uint32_t src_x; uint32_t src_y; uint32_t src_h, src_w; // Props we should use from master, I think we can ignore most of this by copying intel_plane_state->plane_ctl / intel_plane_state->plane_color_ctl from the master plane. u16 alpha; unsigned int rotation; uint16_t pixel_blend_mode; enum drm_color_encoding color_encoding; enum drm_color_range color_range; On hdr planes only, we program the CSC, so we need special care for those in that case: enum drm_color_encoding color_encoding; enum drm_color_range color_range; Ugly: struct drm_crtc *crtc; // might be NULL struct drm_framebuffer *fb; // is NULL, or even worse it may point to a different FB, so treat anything touching plane_state->fb with suspicion So to sum it all up, we need to be very careful in a lot of places to handle deviating values correctly.. Most important ones are crtc_state->active and crtc_state->enable. For planes, plane_state->crtc might be null, and plane_state->fb anything. Planes are easier to solve though, but crtc_state is all over the place. :( ~Maarten
On Thu, Jan 24, 2019 at 12:23:45PM +0100, Maarten Lankhorst wrote: > Op 23-01-2019 om 18:31 schreef Matt Roper: > > On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote: > >> On Gen 11 platform, to enable resolutions like 5K@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. > >> > >> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, > >> Matt Roper <matthew.d.roper@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Matt Roper <matthew.d.roper@intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Looks like the right general approach of using slave/master. I agree > > with Ville and Maarten's feedback as well. > > > > The other thing we're going to need to worry about is dealing with all > > the planes on the two crtc's. Suppose we have a userspace-visible state > > of: > > > > Pipe A: off > > Pipe B: 5000x2000@120 (using round numbers to make example simpler) > > Plane 0: RGB32: pos (0,0), width=5000, height=2000 > > Plane 1: RGB32: pos (100,100), width=100, height=100 > > Plane 2: RGB32: pos (4800,1800), width=100, height=100 > > Plane 3: RGB32: pos (0,0), width=3000, height=2000 > > Plane 4: NV12: pos (2000,0), width=1000, height=2000 > > Plane 5: off > > Plane 6: off > > Pipe C: off > > > > this means that we need to grab extra planes on the other CRTC and > > adjust their size, position, and/or surface offsets accordingly. So the > > internal driver state that we actually program into the hardware needs > > to be something like: > > > > Pipe A: off > > Pipe B: 2500x1000 (master) > > Plane 0: pos (0,0), width=2500, height=2000 > > Plane 1: pos (100,100), width=100, height=100 > > Plane 2: off > > Plane 3: pos (0, 0), width=2500, height=2000 > > Plane 4{UV}: pos (2000, 0), width=500, height=2000 > > Plane 5{Y}: pos (2000, 0), width=500, height=2000 > > Plane 6: off > > Pipe C: 2500x1000 (slave) > > Plane 0: pos (0,0), offset=(2500,0), width=2500, height=2000 > > Plane 1: off > > Plane 2: pos (2300,1800), width=100, height=100 > > Plane 3: pos (0, 0), offset=(2500, 0), width=500, height=2000 > > Plane 4{UV}: pos (0, 0), offset=(500,0), width=500, height=2000 > > Plane 5{Y}: pos (0, 0), offset=(500,0), width=500, height=2000 > > Plane 6: off > > > > So I think Ville is right; we're going to need to really copy a lot of > > the userspace-facing state data into our own internal state structures > > so that we can do the various adjustments on it. As you can see above > > there are cases (2pi1po + nv12) where a single userspace plane request > > translates into us programming four different sets of plane register > > values. > > Yeah I fear you're right. > Lets see, we would need to sanitize the driver big time.. > > > struct drm_crtc_state { > struct drm_crtc *crtc; > bool enable; > bool active; > bool planes_changed : 1; > bool mode_changed : 1; > bool active_changed : 1; > bool connectors_changed : 1; > bool zpos_changed : 1; // unused > bool color_mgmt_changed : 1; > bool no_vblank : 1; //unused > u32 plane_mask; > u32 connector_mask; > u32 encoder_mask; > struct drm_display_mode adjusted_mode; > struct drm_display_mode mode; > struct drm_property_blob *mode_blob; > struct drm_property_blob *degamma_lut; > struct drm_property_blob *ctm; > struct drm_property_blob *gamma_lut; > u32 target_vblank; // unused > u32 pageflip_flags; // unused > struct drm_pending_vblank_event *event; > struct drm_crtc_commit *commit; > struct drm_atomic_state *state; > }; > > First the good, it looks like we can safely re-use the following in the slave, without affecting userspace: > struct drm_crtc * crtc; > u32 last_vblank_count; // ignored in i915 > struct drm_display_mode adjusted_mode; // sort of handled by core, but on disable we can write our own mode here > struct drm_pending_vblank_event * event; // handled by core > struct drm_atomic_state * state; // handled by core Okay so these above fields can be part of the drm_crtc_uapi_state as suggested by Ville. So that these exposed to the userspace do not change irrespective of master-slave behaviour and can be just used to configure master_crtc and slave_crtc Correct? So these will not be part of drm_master_crtc_state and drm_slave_crtc_state > > > Then one we might use inside i915, but should not inside the driver: > struct drm_display_mode mode; -> use crtc_state.adjusted_mode.. Shouldnt the mode represent the true mode and not the mode/2 going to each of the crtcs? And then the adjusted mode can reflect the mode/2? > > Then the bad, we can easily get from the master safely. We only need to add special handling in intel_color.c: > struct drm_property_blob * degamma_lut; > struct drm_property_blob * ctm; > struct drm_property_blob * gamma_lut; So they can actually be in the drm_crtc_uapi_state and used for both master and slave in i915? > > Also bad, > u32 plane_mask; // See below. > u32 connector_mask; // Hmm, I don't think we care about connectors on slave, do we? Just special handling in the modeset calculations needed > u32 encoder_mask; // Handled by core, but probably fine to leave as 0. Can't touch this. Don't need to either. > bool planes_changed:1; > bool mode_changed:1; > bool active_changed:1; > bool connectors_changed:1; > bool color_mgmt_changed:1; > > Whatever for most of those, can ignore the masks probably. Just make sure master plane state checks > the slave plane state as well, and ignore direct checks of slave plane state. Because this is a 1:1 mapping, > it should be fine. So leave these fields as is for both master and slave crtc states i i915, slave state derives from master's. > > And finally, the ugly: > bool enable; > bool active; > > The biggest issues are crtc_state->active and crtc_state->enable. i915 uses it a lot, and even the core makes > a lot of decisions based on it. We should probably only steal crtc's that doesn't have crtc_state->enable set. > Correct, the stealing crtc logic should check for crtc_state->enable. So if the master and slave crtc states are created right up in DRM, then once slave crtc is stolen its crtc_enable should be set then. So again drm_crtc_master_state and drm_crtc_slave_state each will have these fields, correct? So to summarize your suggestion is that we create a drm_crtc_uapi_state which will be the state common to both master and slave crtcs. Then have the derived states as drm_crtc_master_state and drm_crtc_slave_state Also split the drm_plane_master_state and drm_plane_slave_state to map planes across the two crtcs. Is my understanding correct so far, I analyze your plane_state comments next. First wanted to understand if my interpretation of the crtc_state is correct Manasi > But one problem at a time.. > > plane_state is relatively easy, because even if we can't touch plane_mask and the atomic details for the planes, > we need to derive this from the master plane check function anyway. Fortunately we have our own > intel_crtc_state->active_planes already used in the plane update functions we just need to be sure that the > planes are part of the state. We should probably completely ignore the slave planes in the plane atomic check > called from the core when it's used as a slave.. > > Once for plane_state: > struct drm_plane_state { > struct drm_plane *plane; > struct drm_crtc *crtc; > struct drm_framebuffer *fb; > struct dma_fence *fence; > int32_t crtc_x; > int32_t crtc_y; > uint32_t crtc_w, crtc_h; > uint32_t src_x; > uint32_t src_y; > uint32_t src_h, src_w; > u16 alpha; > uint16_t pixel_blend_mode; > unsigned int rotation; > unsigned int zpos; > unsigned int normalized_zpos; > enum drm_color_encoding color_encoding; > enum drm_color_range color_range; > struct drm_rect src, dst; > bool visible; > struct drm_crtc_commit *commit; > struct drm_atomic_state *state; > }; > > Good: > struct drm_plane *plane; > struct dma_fence *fence; // must be copied from master plane_state > struct drm_rect src, dst; // can fill in what we want here.. > bool visible; // ignored by core mostly, but set and used by driver > unsigned int zpos; > unsigned int normalized_zpos; > struct drm_crtc_commit *commit; > struct drm_atomic_state *state; > > > Bad: > // We don't use the below anyway, we must use the src and dst rects.. > int32_t crtc_x; > int32_t crtc_y; > uint32_t crtc_w, crtc_h; > uint32_t src_x; > uint32_t src_y; > uint32_t src_h, src_w; > > // Props we should use from master, I think we can ignore most of this by copying intel_plane_state->plane_ctl / intel_plane_state->plane_color_ctl from the master plane. > u16 alpha; > unsigned int rotation; > uint16_t pixel_blend_mode; > enum drm_color_encoding color_encoding; > enum drm_color_range color_range; > > On hdr planes only, we program the CSC, so we need special care for those in that case: > enum drm_color_encoding color_encoding; > enum drm_color_range color_range; > > Ugly: > struct drm_crtc *crtc; // might be NULL > struct drm_framebuffer *fb; // is NULL, or even worse it may point to a different FB, so treat anything touching plane_state->fb with suspicion > > So to sum it all up, we need to be very careful in a lot of places to handle deviating values correctly.. > > Most important ones are crtc_state->active and crtc_state->enable. For planes, plane_state->crtc might be null, and plane_state->fb anything. Planes are easier to solve though, but crtc_state is all over the place. :( > > > > ~Maarten >
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 {
On Gen 11 platform, to enable resolutions like 5K@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. Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Matt Roper <matthew.d.roper@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 6 +++ 2 files changed, 69 insertions(+)