On Mon, 01 Dec 2014, "Singh, Gaurav K" <gaurav.k.singh@xxxxxxxxx> wrote:
> On 12/1/2014 7:41 PM, Jani Nikula wrote:
>> On Mon, 01 Dec 2014, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>>> On Sat, 29 Nov 2014, Gaurav K Singh <gaurav.k.singh@xxxxxxxxx> 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@xxxxxxxxx>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> --
>>> Jani Nikula, Intel Open Source Technology Center
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
