| Message ID | 20191016125236.3447-2-vandita.kulkarni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add dsi_state in crtc_state | expand |
On Wed, 16 Oct 2019, Vandita Kulkarni <vandita.kulkarni@intel.com> wrote: > This patch add dsi_state which provides > dsi operation mode and the link mode. > These are needed in order to check if they > were differently configured by GOP. > > In present case the GOP enables dsi in > periodic update mode, whereas we need > to enable it in TE_GATE command mode. > In which case a disable-enable sequence > would be required. I think the high level comment is that if you need to have things in the crtc state, you need to *move* the data there, as single point of truth, not copy. BR, Jani. > --- > drivers/gpu/drm/i915/display/icl_dsi.c | 39 +++++++++++++++++++ > .../drm/i915/display/intel_display_types.h | 12 ++++++ > 2 files changed, 51 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c > index 6e398c33a524..0a9323e95866 100644 > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > @@ -1238,6 +1238,37 @@ static void gen11_dsi_get_timings(struct intel_encoder *encoder, > adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; > } > > +static bool > +gen11_dsi_dual_link_enabled(struct drm_i915_private *dev_priv) > +{ > + u32 val1, val2; > + > + val1 = I915_READ(PIPECONF(TRANSCODER_DSI_0)) & > + I915_READ(PIPECONF(TRANSCODER_DSI_1)); > + val1 &= PIPECONF_ENABLE; > + > + val2 = I915_READ(TRANS_DDI_FUNC_CTL2(TRANSCODER_DSI_0)); > + val2 &= PORT_SYNC_MODE_ENABLE; > + > + return (val1 && val2); > +} > + > +static enum dsi_op_mode > +gen11_dsi_get_op_mode(struct drm_i915_private *dev_priv, > + struct intel_dsi *intel_dsi) > +{ > + u32 val; > + enum transcoder dsi_trans; > + > + if (intel_dsi->ports == BIT(PORT_B)) > + dsi_trans = TRANSCODER_DSI_1; > + else > + dsi_trans = TRANSCODER_DSI_0; > + > + val = I915_READ(DSI_TRANS_FUNC_CONF(dsi_trans)); > + return ((val &= OP_MODE_MASK) >> OP_MODE_SHIFT); > +} > + > static void gen11_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > @@ -1250,6 +1281,12 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder, > cnl_calc_wrpll_link(dev_priv, &pipe_config->dpll_hw_state); > > pipe_config->base.adjusted_mode.crtc_clock = intel_dsi->pclk; > + > + pipe_config->dsi_state.dual_link_mode = > + gen11_dsi_dual_link_enabled(dev_priv); > + pipe_config->dsi_state.op_mode = > + gen11_dsi_get_op_mode(dev_priv, intel_dsi); > + > if (intel_dsi->dual_link) > pipe_config->base.adjusted_mode.crtc_clock *= 2; > > @@ -1283,6 +1320,8 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder, > else > pipe_config->cpu_transcoder = TRANSCODER_DSI_0; > > + pipe_config->dsi_state.op_mode = DSI_CMD_MODE_NO_GATE; > + pipe_config->dsi_state.dual_link_mode = intel_dsi->dual_link; > pipe_config->clock_set = true; > pipe_config->port_clock = intel_dsi_bitrate(intel_dsi) / 5; > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 40390d855815..f89917eb4b94 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -75,6 +75,13 @@ enum hdmi_force_audio { > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > }; > > +enum dsi_op_mode { > + DSI_CMD_MODE_NO_GATE, > + DSI_CMD_MODE_TE_GATE, > + DSI_CMD_MODE_PERIODIC, > + DSI_VIDEO_MODE, > + }; > + > /* "Broadcast RGB" property */ > enum intel_broadcast_rgb { > INTEL_BROADCAST_RGB_AUTO, > @@ -861,6 +868,11 @@ struct intel_crtc_state { > u32 ctrl, div; > } dsi_pll; > > + struct { > + enum dsi_op_mode op_mode; > + bool dual_link_mode; > + } dsi_state; > + > int pipe_bpp; > struct intel_link_m_n dp_m_n;
On Wed, Oct 16, 2019 at 06:22:36PM +0530, Vandita Kulkarni wrote: > This patch add dsi_state which provides > dsi operation mode and the link mode. > These are needed in order to check if they > were differently configured by GOP. > > In present case the GOP enables dsi in > periodic update mode, whereas we need > to enable it in TE_GATE command mode. > In which case a disable-enable sequence > would be required. > --- > drivers/gpu/drm/i915/display/icl_dsi.c | 39 +++++++++++++++++++ > .../drm/i915/display/intel_display_types.h | 12 ++++++ > 2 files changed, 51 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c > index 6e398c33a524..0a9323e95866 100644 > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > @@ -1238,6 +1238,37 @@ static void gen11_dsi_get_timings(struct intel_encoder *encoder, > adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; > } > > +static bool > +gen11_dsi_dual_link_enabled(struct drm_i915_private *dev_priv) > +{ > + u32 val1, val2; > + > + val1 = I915_READ(PIPECONF(TRANSCODER_DSI_0)) & > + I915_READ(PIPECONF(TRANSCODER_DSI_1)); > + val1 &= PIPECONF_ENABLE; > + > + val2 = I915_READ(TRANS_DDI_FUNC_CTL2(TRANSCODER_DSI_0)); > + val2 &= PORT_SYNC_MODE_ENABLE; > + > + return (val1 && val2); > +} > + > +static enum dsi_op_mode > +gen11_dsi_get_op_mode(struct drm_i915_private *dev_priv, > + struct intel_dsi *intel_dsi) > +{ > + u32 val; > + enum transcoder dsi_trans; > + > + if (intel_dsi->ports == BIT(PORT_B)) > + dsi_trans = TRANSCODER_DSI_1; > + else > + dsi_trans = TRANSCODER_DSI_0; > + > + val = I915_READ(DSI_TRANS_FUNC_CONF(dsi_trans)); > + return ((val &= OP_MODE_MASK) >> OP_MODE_SHIFT); > +} > + > static void gen11_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > @@ -1250,6 +1281,12 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder, > cnl_calc_wrpll_link(dev_priv, &pipe_config->dpll_hw_state); > > pipe_config->base.adjusted_mode.crtc_clock = intel_dsi->pclk; > + > + pipe_config->dsi_state.dual_link_mode = > + gen11_dsi_dual_link_enabled(dev_priv); > + pipe_config->dsi_state.op_mode = > + gen11_dsi_get_op_mode(dev_priv, intel_dsi); > + > if (intel_dsi->dual_link) > pipe_config->base.adjusted_mode.crtc_clock *= 2; > > @@ -1283,6 +1320,8 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder, > else > pipe_config->cpu_transcoder = TRANSCODER_DSI_0; > > + pipe_config->dsi_state.op_mode = DSI_CMD_MODE_NO_GATE; > + pipe_config->dsi_state.dual_link_mode = intel_dsi->dual_link; > pipe_config->clock_set = true; > pipe_config->port_clock = intel_dsi_bitrate(intel_dsi) / 5; > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 40390d855815..f89917eb4b94 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -75,6 +75,13 @@ enum hdmi_force_audio { > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > }; > > +enum dsi_op_mode { > + DSI_CMD_MODE_NO_GATE, > + DSI_CMD_MODE_TE_GATE, > + DSI_CMD_MODE_PERIODIC, > + DSI_VIDEO_MODE, > + }; > + > /* "Broadcast RGB" property */ > enum intel_broadcast_rgb { > INTEL_BROADCAST_RGB_AUTO, > @@ -861,6 +868,11 @@ struct intel_crtc_state { > u32 ctrl, div; > } dsi_pll; > > + struct { > + enum dsi_op_mode op_mode; > + bool dual_link_mode; 'dual_link' seems sufficient. And we should share that with LVDS. Another easy target for moving to the crtc state is the lane count. In fact we already have it in the state, but the DSI code has just stubbornly decided to ingore it. > + } dsi_state; > + > int pipe_bpp; > struct intel_link_m_n dp_m_n; > > -- > 2.21.0.5.gaeb582a > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, October 24, 2019 7:06 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; > Syrjala, Ville <ville.syrjala@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dsi: Add dsi_state in crtc_state > > On Wed, Oct 16, 2019 at 06:22:36PM +0530, Vandita Kulkarni wrote: > > This patch add dsi_state which provides dsi operation mode and the > > link mode. > > These are needed in order to check if they were differently configured > > by GOP. > > > > In present case the GOP enables dsi in periodic update mode, whereas > > we need to enable it in TE_GATE command mode. > > In which case a disable-enable sequence would be required. > > --- > > drivers/gpu/drm/i915/display/icl_dsi.c | 39 +++++++++++++++++++ > > .../drm/i915/display/intel_display_types.h | 12 ++++++ > > 2 files changed, 51 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > > b/drivers/gpu/drm/i915/display/icl_dsi.c > > index 6e398c33a524..0a9323e95866 100644 > > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > > @@ -1238,6 +1238,37 @@ static void gen11_dsi_get_timings(struct > intel_encoder *encoder, > > adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; } > > > > +static bool > > +gen11_dsi_dual_link_enabled(struct drm_i915_private *dev_priv) { > > + u32 val1, val2; > > + > > + val1 = I915_READ(PIPECONF(TRANSCODER_DSI_0)) & > > + I915_READ(PIPECONF(TRANSCODER_DSI_1)); > > + val1 &= PIPECONF_ENABLE; > > + > > + val2 = I915_READ(TRANS_DDI_FUNC_CTL2(TRANSCODER_DSI_0)); > > + val2 &= PORT_SYNC_MODE_ENABLE; > > + > > + return (val1 && val2); > > +} > > + > > +static enum dsi_op_mode > > +gen11_dsi_get_op_mode(struct drm_i915_private *dev_priv, > > + struct intel_dsi *intel_dsi) > > +{ > > + u32 val; > > + enum transcoder dsi_trans; > > + > > + if (intel_dsi->ports == BIT(PORT_B)) > > + dsi_trans = TRANSCODER_DSI_1; > > + else > > + dsi_trans = TRANSCODER_DSI_0; > > + > > + val = I915_READ(DSI_TRANS_FUNC_CONF(dsi_trans)); > > + return ((val &= OP_MODE_MASK) >> OP_MODE_SHIFT); } > > + > > static void gen11_dsi_get_config(struct intel_encoder *encoder, > > struct intel_crtc_state *pipe_config) { @@ - > 1250,6 +1281,12 @@ > > static void gen11_dsi_get_config(struct intel_encoder *encoder, > > cnl_calc_wrpll_link(dev_priv, &pipe_config->dpll_hw_state); > > > > pipe_config->base.adjusted_mode.crtc_clock = intel_dsi->pclk; > > + > > + pipe_config->dsi_state.dual_link_mode = > > + gen11_dsi_dual_link_enabled(dev_priv); > > + pipe_config->dsi_state.op_mode = > > + gen11_dsi_get_op_mode(dev_priv, intel_dsi); > > + > > if (intel_dsi->dual_link) > > pipe_config->base.adjusted_mode.crtc_clock *= 2; > > > > @@ -1283,6 +1320,8 @@ static int gen11_dsi_compute_config(struct > intel_encoder *encoder, > > else > > pipe_config->cpu_transcoder = TRANSCODER_DSI_0; > > > > + pipe_config->dsi_state.op_mode = DSI_CMD_MODE_NO_GATE; > > + pipe_config->dsi_state.dual_link_mode = intel_dsi->dual_link; > > pipe_config->clock_set = true; > > pipe_config->port_clock = intel_dsi_bitrate(intel_dsi) / 5; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 40390d855815..f89917eb4b94 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -75,6 +75,13 @@ enum hdmi_force_audio { > > HDMI_AUDIO_ON, /* force turn on HDMI audio > */ > > }; > > > > +enum dsi_op_mode { > > + DSI_CMD_MODE_NO_GATE, > > + DSI_CMD_MODE_TE_GATE, > > + DSI_CMD_MODE_PERIODIC, > > + DSI_VIDEO_MODE, > > + }; > > + > > /* "Broadcast RGB" property */ > > enum intel_broadcast_rgb { > > INTEL_BROADCAST_RGB_AUTO, > > @@ -861,6 +868,11 @@ struct intel_crtc_state { > > u32 ctrl, div; > > } dsi_pll; > > > > + struct { > > + enum dsi_op_mode op_mode; > > + bool dual_link_mode; > > 'dual_link' seems sufficient. And we should share that with LVDS. Ok.. The reason I added dsi_op_mode was to ensure we do a deconfigure and configure the dsi in case any other kind of mode is set by the GOP In current case the GOP puts the dsi in DSI_CMD_MODE_PERIODIC ( which is like updating the panel on every TE by the dsi transcoder itself) To put it in the DSI_CMD_MODE_TE_GATE ( panel update whenever the software wants to update) we need to disable and enable in this mode. So I thought of adding it in the crtc_state. Do you think, if this mismatch in this particular mode can be handle with any other existing state variables.. > > Another easy target for moving to the crtc state is the lane count. Ok.. I can try using this. There is one more requirement on every flip we need to update the FRMCTL register on the port where dsi is present in dsi te gate cmd mode. Do you think if it would look good to use a state variable to represent that frame update? Or should we reach to some encoder function to do this job on every flip? Thanks, Vandita > In fact we already have it in the state, but the DSI code has just stubbornly > decided to ingore it. > > > + } dsi_state; > > + > > int pipe_bpp; > > struct intel_link_m_n dp_m_n; > > > > -- > > 2.21.0.5.gaeb582a > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c index 6e398c33a524..0a9323e95866 100644 --- a/drivers/gpu/drm/i915/display/icl_dsi.c +++ b/drivers/gpu/drm/i915/display/icl_dsi.c @@ -1238,6 +1238,37 @@ static void gen11_dsi_get_timings(struct intel_encoder *encoder, adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; } +static bool +gen11_dsi_dual_link_enabled(struct drm_i915_private *dev_priv) +{ + u32 val1, val2; + + val1 = I915_READ(PIPECONF(TRANSCODER_DSI_0)) & + I915_READ(PIPECONF(TRANSCODER_DSI_1)); + val1 &= PIPECONF_ENABLE; + + val2 = I915_READ(TRANS_DDI_FUNC_CTL2(TRANSCODER_DSI_0)); + val2 &= PORT_SYNC_MODE_ENABLE; + + return (val1 && val2); +} + +static enum dsi_op_mode +gen11_dsi_get_op_mode(struct drm_i915_private *dev_priv, + struct intel_dsi *intel_dsi) +{ + u32 val; + enum transcoder dsi_trans; + + if (intel_dsi->ports == BIT(PORT_B)) + dsi_trans = TRANSCODER_DSI_1; + else + dsi_trans = TRANSCODER_DSI_0; + + val = I915_READ(DSI_TRANS_FUNC_CONF(dsi_trans)); + return ((val &= OP_MODE_MASK) >> OP_MODE_SHIFT); +} + static void gen11_dsi_get_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config) { @@ -1250,6 +1281,12 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder, cnl_calc_wrpll_link(dev_priv, &pipe_config->dpll_hw_state); pipe_config->base.adjusted_mode.crtc_clock = intel_dsi->pclk; + + pipe_config->dsi_state.dual_link_mode = + gen11_dsi_dual_link_enabled(dev_priv); + pipe_config->dsi_state.op_mode = + gen11_dsi_get_op_mode(dev_priv, intel_dsi); + if (intel_dsi->dual_link) pipe_config->base.adjusted_mode.crtc_clock *= 2; @@ -1283,6 +1320,8 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder, else pipe_config->cpu_transcoder = TRANSCODER_DSI_0; + pipe_config->dsi_state.op_mode = DSI_CMD_MODE_NO_GATE; + pipe_config->dsi_state.dual_link_mode = intel_dsi->dual_link; pipe_config->clock_set = true; pipe_config->port_clock = intel_dsi_bitrate(intel_dsi) / 5; diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 40390d855815..f89917eb4b94 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -75,6 +75,13 @@ enum hdmi_force_audio { HDMI_AUDIO_ON, /* force turn on HDMI audio */ }; +enum dsi_op_mode { + DSI_CMD_MODE_NO_GATE, + DSI_CMD_MODE_TE_GATE, + DSI_CMD_MODE_PERIODIC, + DSI_VIDEO_MODE, + }; + /* "Broadcast RGB" property */ enum intel_broadcast_rgb { INTEL_BROADCAST_RGB_AUTO, @@ -861,6 +868,11 @@ struct intel_crtc_state { u32 ctrl, div; } dsi_pll; + struct { + enum dsi_op_mode op_mode; + bool dual_link_mode; + } dsi_state; + int pipe_bpp; struct intel_link_m_n dp_m_n;