| Message ID | 1417254967-27384-4-git-send-email-gaurav.k.singh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote: > For Dual Link MIPI Panels, both Port A and Port C should be enabled > during the MIPI encoder enabling sequence. Similarly, during the > disabling sequence, both ports needs to be disabled. > > v2: Used for_each_dsi_port macro instead of for loop > > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_dsi.c | 39 +++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_dsi.h | 1 + > drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 7 +++++ > 4 files changed, 36 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index dc03fac..c981f5d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6664,6 +6664,7 @@ enum punit_power_well { > #define DPI_ENABLE (1 << 31) /* A + C */ > #define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 > #define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK (0xf << 27) > +#define DUAL_LINK_MODE_SHIFT 26 > #define DUAL_LINK_MODE_MASK (1 << 26) > #define DUAL_LINK_MODE_FRONT_BACK (0 << 26) > #define DUAL_LINK_MODE_PIXEL_ALTERNATIVE (1 << 26) > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 693736b..1163a5b 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); > + enum port port; > u32 temp; > > - /* assert ip_tg_enable signal */ > - temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK; > - temp = temp | intel_dsi->port_bits; > - I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); > - POSTING_READ(MIPI_PORT_CTRL(port)); > + for_each_dsi_port(port, intel_dsi->ports) { > + temp = I915_READ(MIPI_PORT_CTRL(port)); > + > + if (intel_dsi->dual_link) { > + if (port == PORT_A) > + intel_dsi->port_bits |= intel_crtc->pipe ? > + LANE_CONFIGURATION_DUAL_LINK_B : > + LANE_CONFIGURATION_DUAL_LINK_A; > + else > + intel_dsi->port_bits = 0; It feels wrong to clobber intel_dsi->port_bits here; the old code didn't do that either. I think you should either set port_bits somewhere else (or remove it altogether), and then modify temp depending on port. Side note, it seems to me intel_dsi->dual_link is becoming redundant, as intel_dsi->ports will have more than one bit set in the dual link case. This can be a future cleanup though. BR, Jani. > + } else > + temp &= ~LANE_CONFIGURATION_MASK; > + > + /* assert ip_tg_enable signal */ > + temp = temp | intel_dsi->port_bits; > + I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); > + POSTING_READ(MIPI_PORT_CTRL(port)); > + } > } > > static void intel_dsi_port_disable(struct intel_encoder *encoder) > { > struct drm_device *dev = encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + enum port port; > u32 temp; > > - /* de-assert ip_tg_enable signal */ > - temp = I915_READ(MIPI_PORT_CTRL(port)); > - I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); > - POSTING_READ(MIPI_PORT_CTRL(port)); > + for_each_dsi_port(port, intel_dsi->ports) { > + /* de-assert ip_tg_enable signal */ > + temp = I915_READ(MIPI_PORT_CTRL(port)); > + I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); > + POSTING_READ(MIPI_PORT_CTRL(port)); > + } > } > > static void intel_dsi_device_ready(struct intel_encoder *encoder) > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index 7f5d028..f2cc2fc 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -104,6 +104,7 @@ struct intel_dsi { > u8 clock_stop; > > u8 escape_clk_div; > + u8 dual_link; > u32 port_bits; > u32 bw_timer; > u32 dphy_reg; > diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > index 612592f..7f1ba58 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi) > intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; > intel_dsi->lane_count = mipi_config->lane_cnt + 1; > intel_dsi->pixel_format = mipi_config->videomode_color_format << 7; > + intel_dsi->dual_link = mipi_config->dual_link; > + > + if (intel_dsi->dual_link) { > + intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); > + intel_dsi->port_bits = (intel_dsi->dual_link - 1) > + << DUAL_LINK_MODE_SHIFT; > + } > > if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666) > bits_per_pixel = 18; > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 01 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote: >> For Dual Link MIPI Panels, both Port A and Port C should be enabled >> during the MIPI encoder enabling sequence. Similarly, during the >> disabling sequence, both ports needs to be disabled. >> >> v2: Used for_each_dsi_port macro instead of for loop >> >> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_dsi.c | 39 +++++++++++++++++++--------- >> drivers/gpu/drm/i915/intel_dsi.h | 1 + >> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 7 +++++ >> 4 files changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index dc03fac..c981f5d 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6664,6 +6664,7 @@ enum punit_power_well { >> #define DPI_ENABLE (1 << 31) /* A + C */ >> #define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 >> #define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK (0xf << 27) >> +#define DUAL_LINK_MODE_SHIFT 26 >> #define DUAL_LINK_MODE_MASK (1 << 26) >> #define DUAL_LINK_MODE_FRONT_BACK (0 << 26) >> #define DUAL_LINK_MODE_PIXEL_ALTERNATIVE (1 << 26) >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 693736b..1163a5b 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >> + enum port port; >> u32 temp; >> >> - /* assert ip_tg_enable signal */ >> - temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK; >> - temp = temp | intel_dsi->port_bits; >> - I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >> - POSTING_READ(MIPI_PORT_CTRL(port)); >> + for_each_dsi_port(port, intel_dsi->ports) { >> + temp = I915_READ(MIPI_PORT_CTRL(port)); >> + >> + if (intel_dsi->dual_link) { >> + if (port == PORT_A) >> + intel_dsi->port_bits |= intel_crtc->pipe ? >> + LANE_CONFIGURATION_DUAL_LINK_B : >> + LANE_CONFIGURATION_DUAL_LINK_A; >> + else >> + intel_dsi->port_bits = 0; > > It feels wrong to clobber intel_dsi->port_bits here; the old code didn't > do that either. I think you should either set port_bits somewhere else > (or remove it altogether), and then modify temp depending on port. > > Side note, it seems to me intel_dsi->dual_link is becoming redundant, as > intel_dsi->ports will have more than one bit set in the dual link > case. This can be a future cleanup though. Okay, so it's becoming redundant for checking whether we are using dual link or not, but it still has it's place for indicating which dual link mode to use. BR, Jani. > > BR, > Jani. > >> + } else >> + temp &= ~LANE_CONFIGURATION_MASK; >> + >> + /* assert ip_tg_enable signal */ >> + temp = temp | intel_dsi->port_bits; >> + I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >> + POSTING_READ(MIPI_PORT_CTRL(port)); >> + } >> } >> >> static void intel_dsi_port_disable(struct intel_encoder *encoder) >> { >> struct drm_device *dev = encoder->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> + enum port port; >> u32 temp; >> >> - /* de-assert ip_tg_enable signal */ >> - temp = I915_READ(MIPI_PORT_CTRL(port)); >> - I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >> - POSTING_READ(MIPI_PORT_CTRL(port)); >> + for_each_dsi_port(port, intel_dsi->ports) { >> + /* de-assert ip_tg_enable signal */ >> + temp = I915_READ(MIPI_PORT_CTRL(port)); >> + I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >> + POSTING_READ(MIPI_PORT_CTRL(port)); >> + } >> } >> >> static void intel_dsi_device_ready(struct intel_encoder *encoder) >> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >> index 7f5d028..f2cc2fc 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.h >> +++ b/drivers/gpu/drm/i915/intel_dsi.h >> @@ -104,6 +104,7 @@ struct intel_dsi { >> u8 clock_stop; >> >> u8 escape_clk_div; >> + u8 dual_link; >> u32 port_bits; >> u32 bw_timer; >> u32 dphy_reg; >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> index 612592f..7f1ba58 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi) >> intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; >> intel_dsi->lane_count = mipi_config->lane_cnt + 1; >> intel_dsi->pixel_format = mipi_config->videomode_color_format << 7; >> + intel_dsi->dual_link = mipi_config->dual_link; >> + >> + if (intel_dsi->dual_link) { >> + intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); >> + intel_dsi->port_bits = (intel_dsi->dual_link - 1) >> + << DUAL_LINK_MODE_SHIFT; >> + } >> >> if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666) >> bits_per_pixel = 18; >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On 12/1/2014 7:41 PM, Jani Nikula wrote: > On Mon, 01 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote: >>> For Dual Link MIPI Panels, both Port A and Port C should be enabled >>> during the MIPI encoder enabling sequence. Similarly, during the >>> disabling sequence, both ports needs to be disabled. >>> >>> v2: Used for_each_dsi_port macro instead of for loop >>> >>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> >>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>> drivers/gpu/drm/i915/intel_dsi.c | 39 +++++++++++++++++++--------- >>> drivers/gpu/drm/i915/intel_dsi.h | 1 + >>> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 7 +++++ >>> 4 files changed, 36 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>> index dc03fac..c981f5d 100644 >>> --- a/drivers/gpu/drm/i915/i915_reg.h >>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>> @@ -6664,6 +6664,7 @@ enum punit_power_well { >>> #define DPI_ENABLE (1 << 31) /* A + C */ >>> #define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 >>> #define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK (0xf << 27) >>> +#define DUAL_LINK_MODE_SHIFT 26 >>> #define DUAL_LINK_MODE_MASK (1 << 26) >>> #define DUAL_LINK_MODE_FRONT_BACK (0 << 26) >>> #define DUAL_LINK_MODE_PIXEL_ALTERNATIVE (1 << 26) >>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >>> index 693736b..1163a5b 100644 >>> --- a/drivers/gpu/drm/i915/intel_dsi.c >>> +++ b/drivers/gpu/drm/i915/intel_dsi.c >>> @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder) >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >>> + enum port port; >>> u32 temp; >>> >>> - /* assert ip_tg_enable signal */ >>> - temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK; >>> - temp = temp | intel_dsi->port_bits; >>> - I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >>> - POSTING_READ(MIPI_PORT_CTRL(port)); >>> + for_each_dsi_port(port, intel_dsi->ports) { >>> + temp = I915_READ(MIPI_PORT_CTRL(port)); >>> + >>> + if (intel_dsi->dual_link) { >>> + if (port == PORT_A) >>> + intel_dsi->port_bits |= intel_crtc->pipe ? >>> + LANE_CONFIGURATION_DUAL_LINK_B : >>> + LANE_CONFIGURATION_DUAL_LINK_A; >>> + else >>> + intel_dsi->port_bits = 0; >> It feels wrong to clobber intel_dsi->port_bits here; the old code didn't >> do that either. I think you should either set port_bits somewhere else >> (or remove it altogether), and then modify temp depending on port. >> >> Side note, it seems to me intel_dsi->dual_link is becoming redundant, as >> intel_dsi->ports will have more than one bit set in the dual link >> case. This can be a future cleanup though. > Okay, so it's becoming redundant for checking whether we are using dual > link or not, but it still has it's place for indicating which dual link > mode to use. > > BR, > Jani. Hi Jani, I preferred if (intel_dsi->dual_link) than if (intel_dsi->ports ==((1 << PORT_A) | (1 << PORT_C))), as it looks more cleaner than the second one. Regarding port_bits, in the old code for single link DSI panels, this variable was always 0, and it did not had any use. But with dual link configuration, there are few more register bits which we need to configure, since the function name was intel_dsi_port_enable, i thought of setting the corresponding mipi port_bits variable in this function itself. With regards, Gaurav > >> BR, >> Jani. >> >>> + } else >>> + temp &= ~LANE_CONFIGURATION_MASK; >>> + >>> + /* assert ip_tg_enable signal */ >>> + temp = temp | intel_dsi->port_bits; >>> + I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >>> + POSTING_READ(MIPI_PORT_CTRL(port)); >>> + } >>> } >>> >>> static void intel_dsi_port_disable(struct intel_encoder *encoder) >>> { >>> struct drm_device *dev = encoder->base.dev; >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >>> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >>> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>> + enum port port; >>> u32 temp; >>> >>> - /* de-assert ip_tg_enable signal */ >>> - temp = I915_READ(MIPI_PORT_CTRL(port)); >>> - I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >>> - POSTING_READ(MIPI_PORT_CTRL(port)); >>> + for_each_dsi_port(port, intel_dsi->ports) { >>> + /* de-assert ip_tg_enable signal */ >>> + temp = I915_READ(MIPI_PORT_CTRL(port)); >>> + I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >>> + POSTING_READ(MIPI_PORT_CTRL(port)); >>> + } >>> } >>> >>> static void intel_dsi_device_ready(struct intel_encoder *encoder) >>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >>> index 7f5d028..f2cc2fc 100644 >>> --- a/drivers/gpu/drm/i915/intel_dsi.h >>> +++ b/drivers/gpu/drm/i915/intel_dsi.h >>> @@ -104,6 +104,7 @@ struct intel_dsi { >>> u8 clock_stop; >>> >>> u8 escape_clk_div; >>> + u8 dual_link; >>> u32 port_bits; >>> u32 bw_timer; >>> u32 dphy_reg; >>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>> index 612592f..7f1ba58 100644 >>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>> @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi) >>> intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; >>> intel_dsi->lane_count = mipi_config->lane_cnt + 1; >>> intel_dsi->pixel_format = mipi_config->videomode_color_format << 7; >>> + intel_dsi->dual_link = mipi_config->dual_link; >>> + >>> + if (intel_dsi->dual_link) { >>> + intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); >>> + intel_dsi->port_bits = (intel_dsi->dual_link - 1) >>> + << DUAL_LINK_MODE_SHIFT; >>> + } >>> >>> if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666) >>> bits_per_pixel = 18; >>> -- >>> 1.7.9.5 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- >> Jani Nikula, Intel Open Source Technology Center
On Mon, 01 Dec 2014, "Singh, Gaurav K" <gaurav.k.singh@intel.com> wrote: > On 12/1/2014 7:41 PM, Jani Nikula wrote: >> On Mon, 01 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: >>> On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote: >>>> For Dual Link MIPI Panels, both Port A and Port C should be enabled >>>> during the MIPI encoder enabling sequence. Similarly, during the >>>> disabling sequence, both ports needs to be disabled. >>>> >>>> v2: Used for_each_dsi_port macro instead of for loop >>>> >>>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> >>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>>> drivers/gpu/drm/i915/intel_dsi.c | 39 +++++++++++++++++++--------- >>>> drivers/gpu/drm/i915/intel_dsi.h | 1 + >>>> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 7 +++++ >>>> 4 files changed, 36 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index dc03fac..c981f5d 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -6664,6 +6664,7 @@ enum punit_power_well { >>>> #define DPI_ENABLE (1 << 31) /* A + C */ >>>> #define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 >>>> #define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK (0xf << 27) >>>> +#define DUAL_LINK_MODE_SHIFT 26 >>>> #define DUAL_LINK_MODE_MASK (1 << 26) >>>> #define DUAL_LINK_MODE_FRONT_BACK (0 << 26) >>>> #define DUAL_LINK_MODE_PIXEL_ALTERNATIVE (1 << 26) >>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >>>> index 693736b..1163a5b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dsi.c >>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c >>>> @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder) >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >>>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>>> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >>>> + enum port port; >>>> u32 temp; >>>> >>>> - /* assert ip_tg_enable signal */ >>>> - temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK; >>>> - temp = temp | intel_dsi->port_bits; >>>> - I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >>>> - POSTING_READ(MIPI_PORT_CTRL(port)); >>>> + for_each_dsi_port(port, intel_dsi->ports) { >>>> + temp = I915_READ(MIPI_PORT_CTRL(port)); >>>> + >>>> + if (intel_dsi->dual_link) { >>>> + if (port == PORT_A) >>>> + intel_dsi->port_bits |= intel_crtc->pipe ? >>>> + LANE_CONFIGURATION_DUAL_LINK_B : >>>> + LANE_CONFIGURATION_DUAL_LINK_A; >>>> + else >>>> + intel_dsi->port_bits = 0; >>> It feels wrong to clobber intel_dsi->port_bits here; the old code didn't >>> do that either. I think you should either set port_bits somewhere else >>> (or remove it altogether), and then modify temp depending on port. >>> >>> Side note, it seems to me intel_dsi->dual_link is becoming redundant, as >>> intel_dsi->ports will have more than one bit set in the dual link >>> case. This can be a future cleanup though. >> Okay, so it's becoming redundant for checking whether we are using dual >> link or not, but it still has it's place for indicating which dual link >> mode to use. >> >> BR, >> Jani. > Hi Jani, > I preferred if (intel_dsi->dual_link) than if (intel_dsi->ports ==((1 << > PORT_A) | (1 << PORT_C))), as it looks more cleaner than the second one. I was thinking we could add a helper for that, but let's leave it as it is now. > Regarding port_bits, in the old code for single link DSI panels, this > variable was always 0, and it did not had any use. But with dual link > configuration, there are few more register bits which we need to > configure, since the function name was intel_dsi_port_enable, i thought > of setting the corresponding mipi port_bits variable in this function > itself. Here's my thinking: If port_bits is (or will be) used in places other than intel_dsi_port_enable, I think it needs to be set beforehand, because otherwise the other users will depend on intel_dsi_port_enable having been called. On the other hand, if port_bits is not (and will not be) used in other places, intel_dsi_port_enable has all the information to set MIPI_PORT_CTRL, and the whole intel_dsi->port_bits field can be removed. BR, Jani. > > With regards, > Gaurav > >> >>> BR, >>> Jani. >>> >>>> + } else >>>> + temp &= ~LANE_CONFIGURATION_MASK; >>>> + >>>> + /* assert ip_tg_enable signal */ >>>> + temp = temp | intel_dsi->port_bits; >>>> + I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); >>>> + POSTING_READ(MIPI_PORT_CTRL(port)); >>>> + } >>>> } >>>> >>>> static void intel_dsi_port_disable(struct intel_encoder *encoder) >>>> { >>>> struct drm_device *dev = encoder->base.dev; >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >>>> - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); >>>> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>>> + enum port port; >>>> u32 temp; >>>> >>>> - /* de-assert ip_tg_enable signal */ >>>> - temp = I915_READ(MIPI_PORT_CTRL(port)); >>>> - I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >>>> - POSTING_READ(MIPI_PORT_CTRL(port)); >>>> + for_each_dsi_port(port, intel_dsi->ports) { >>>> + /* de-assert ip_tg_enable signal */ >>>> + temp = I915_READ(MIPI_PORT_CTRL(port)); >>>> + I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); >>>> + POSTING_READ(MIPI_PORT_CTRL(port)); >>>> + } >>>> } >>>> >>>> static void intel_dsi_device_ready(struct intel_encoder *encoder) >>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >>>> index 7f5d028..f2cc2fc 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dsi.h >>>> +++ b/drivers/gpu/drm/i915/intel_dsi.h >>>> @@ -104,6 +104,7 @@ struct intel_dsi { >>>> u8 clock_stop; >>>> >>>> u8 escape_clk_div; >>>> + u8 dual_link; >>>> u32 port_bits; >>>> u32 bw_timer; >>>> u32 dphy_reg; >>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>>> index 612592f..7f1ba58 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >>>> @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi) >>>> intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; >>>> intel_dsi->lane_count = mipi_config->lane_cnt + 1; >>>> intel_dsi->pixel_format = mipi_config->videomode_color_format << 7; >>>> + intel_dsi->dual_link = mipi_config->dual_link; >>>> + >>>> + if (intel_dsi->dual_link) { >>>> + intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); >>>> + intel_dsi->port_bits = (intel_dsi->dual_link - 1) >>>> + << DUAL_LINK_MODE_SHIFT; >>>> + } >>>> >>>> if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666) >>>> bits_per_pixel = 18; >>>> -- >>>> 1.7.9.5 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> -- >>> Jani Nikula, Intel Open Source Technology Center >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index dc03fac..c981f5d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6664,6 +6664,7 @@ enum punit_power_well { #define DPI_ENABLE (1 << 31) /* A + C */ #define MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT 27 #define MIPIA_MIPI4DPHY_DELAY_COUNT_MASK (0xf << 27) +#define DUAL_LINK_MODE_SHIFT 26 #define DUAL_LINK_MODE_MASK (1 << 26) #define DUAL_LINK_MODE_FRONT_BACK (0 << 26) #define DUAL_LINK_MODE_PIXEL_ALTERNATIVE (1 << 26) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 693736b..1163a5b 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -108,28 +108,43 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); + enum port port; u32 temp; - /* assert ip_tg_enable signal */ - temp = I915_READ(MIPI_PORT_CTRL(port)) & ~LANE_CONFIGURATION_MASK; - temp = temp | intel_dsi->port_bits; - I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); - POSTING_READ(MIPI_PORT_CTRL(port)); + for_each_dsi_port(port, intel_dsi->ports) { + temp = I915_READ(MIPI_PORT_CTRL(port)); + + if (intel_dsi->dual_link) { + if (port == PORT_A) + intel_dsi->port_bits |= intel_crtc->pipe ? + LANE_CONFIGURATION_DUAL_LINK_B : + LANE_CONFIGURATION_DUAL_LINK_A; + else + intel_dsi->port_bits = 0; + } else + temp &= ~LANE_CONFIGURATION_MASK; + + /* assert ip_tg_enable signal */ + temp = temp | intel_dsi->port_bits; + I915_WRITE(MIPI_PORT_CTRL(port), temp | DPI_ENABLE); + POSTING_READ(MIPI_PORT_CTRL(port)); + } } static void intel_dsi_port_disable(struct intel_encoder *encoder) { struct drm_device *dev = encoder->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); - enum port port = intel_dsi_pipe_to_port(intel_crtc->pipe); + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + enum port port; u32 temp; - /* de-assert ip_tg_enable signal */ - temp = I915_READ(MIPI_PORT_CTRL(port)); - I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); - POSTING_READ(MIPI_PORT_CTRL(port)); + for_each_dsi_port(port, intel_dsi->ports) { + /* de-assert ip_tg_enable signal */ + temp = I915_READ(MIPI_PORT_CTRL(port)); + I915_WRITE(MIPI_PORT_CTRL(port), temp & ~DPI_ENABLE); + POSTING_READ(MIPI_PORT_CTRL(port)); + } } static void intel_dsi_device_ready(struct intel_encoder *encoder) diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 7f5d028..f2cc2fc 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -104,6 +104,7 @@ struct intel_dsi { u8 clock_stop; u8 escape_clk_div; + u8 dual_link; u32 port_bits; u32 bw_timer; u32 dphy_reg; diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c index 612592f..7f1ba58 100644 --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c @@ -287,6 +287,13 @@ static bool generic_init(struct intel_dsi_device *dsi) intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; intel_dsi->lane_count = mipi_config->lane_cnt + 1; intel_dsi->pixel_format = mipi_config->videomode_color_format << 7; + intel_dsi->dual_link = mipi_config->dual_link; + + if (intel_dsi->dual_link) { + intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); + intel_dsi->port_bits = (intel_dsi->dual_link - 1) + << DUAL_LINK_MODE_SHIFT; + } if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666) bits_per_pixel = 18;