Re: [PATCH 04/10] drm/i915: Pixel Clock changes for DSI dual link

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

On 05/12/2014 17:36, Jani Nikula wrote:
> On Fri, 05 Dec 2014, "Siluvery, Arun" <arun.siluvery@linux.intel.com> wrote:
>> On 05/12/2014 16:33, Singh, Gaurav K wrote:
>>>
>>> On 12/4/2014 2:57 PM, Jani Nikula wrote:
>>>> On Thu, 04 Dec 2014, Gaurav K Singh <gaurav.k.singh@intel.com> wrote:
>>>>> For dual link MIPI Panels, each port needs half of pixel clock. Pixel overlap
>>>>> can be enabled if needed by panel, then in that case, pixel clock will be
>>>>> increased for extra pixels.
>>
>> just a question, why do we need pixel overlap?
>> I couldn't find more details from spec other than that when overlap is
>> set some extra pixels are sent.
>
>>From the host perspective a dual link (or dual channel) DSI device is
> two independent peripheral devices. On the peripheral side the display
> has to combine the input from the two links (which may be two
> independent DSI blocks on the peripheral as well) into one contiguous
> display. I don't know the details, but I'm guessing pixel overlap just
> makes it easier for the peripheral implementation to get it all
> together.

Thank you for the details.
I am just wondering how few extra pixels help on the display side unless 
they are fixed values which act like some kind of markers to synchronize 
between two halves.

regards
Arun

>
> BR,
> Jani.
>
>>
>> regards
>> Arun
>>
>>>>>
>>>>> v2 : Address review comments by Jani
>>>>>         - Removed the bit mask used for ->dual_link
>>>>>         - Used DSI instead of MIPI for #define variables
>>>>>
>>>>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_reg.h            |    4 ++++
>>>>>     drivers/gpu/drm/i915/intel_bios.h          |    3 ++-
>>>>>     drivers/gpu/drm/i915/intel_dsi.c           |    8 ++++++++
>>>>>     drivers/gpu/drm/i915/intel_dsi.h           |    6 ++++++
>>>>>     drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |   21 +++++++++++++++++++++
>>>>>     5 files changed, 41 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index c981f5d..87149ba 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -6029,6 +6029,10 @@ enum punit_power_well {
>>>>>     #define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
>>>>>     #define VLV_PWRDWNUPCTL				0xA294
>>>>>
>>>>> +#define VLV_CHICKEN_3				0x7040C
>>>>> +#define  PIXEL_OVERLAP_CNT_MASK			(3 << 30)
>>>>> +#define  PIXEL_OVERLAP_CNT_SHIFT		30
>>>> I didn't find this register, but does it not need + VLV_DISPLAY_BASE?
>>>>
>>>> Given that I can't find the register my review is pretty shallow, but I
>>>> don't spot anything obviously wrong either. With these caveats,
>>>>
>>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>>>
>>>> This reg is available in BSpec though the bit definitions have not been updated in the BSpec. Also, it was communicated by the BIOS team.
>>>>> +
>>>>>     #define GEN6_PMISR				0x44020
>>>>>     #define GEN6_PMIMR				0x44024 /* rps_lock */
>>>>>     #define GEN6_PMIIR				0x44028
>>>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>>>> index de01167..a6a8710 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_bios.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>>>>> @@ -818,7 +818,8 @@ struct mipi_config {
>>>>>     #define DUAL_LINK_PIXEL_ALT	2
>>>>>     	u16 dual_link:2;
>>>>>     	u16 lane_cnt:2;
>>>>> -	u16 rsvd3:12;
>>>>> +	u16 pixel_overlap:3;
>>>>> +	u16 rsvd3:9;
>>>>>
>>>>>     	u16 rsvd4;
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>>>> index dbe52e9..4e18abd 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>>>> @@ -111,6 +111,14 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder)
>>>>>     	enum port port;
>>>>>     	u32 temp;
>>>>>
>>>>> +	if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) {
>>>>> +		temp = I915_READ(VLV_CHICKEN_3);
>>>>> +		temp &= ~PIXEL_OVERLAP_CNT_MASK |
>>>>> +					intel_dsi->pixel_overlap <<
>>>>> +					PIXEL_OVERLAP_CNT_SHIFT;
>>>>> +		I915_WRITE(VLV_CHICKEN_3, temp);
>>>>> +	}
>>>>> +
>>>>>     	for_each_dsi_port(port, intel_dsi->ports) {
>>>>>     		temp = I915_READ(MIPI_PORT_CTRL(port));
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>>>>> index f2cc2fc..8fe2064 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>>>>> @@ -28,6 +28,11 @@
>>>>>     #include <drm/drm_crtc.h>
>>>>>     #include "intel_drv.h"
>>>>>
>>>>> +/* Dual Link support */
>>>>> +#define DSI_DUAL_LINK_NONE		0
>>>>> +#define DSI_DUAL_LINK_FRONT_BACK	1
>>>>> +#define DSI_DUAL_LINK_PIXEL_ALT		2
>>>>> +
>>>>>     struct intel_dsi_device {
>>>>>     	unsigned int panel_id;
>>>>>     	const char *name;
>>>>> @@ -105,6 +110,7 @@ struct intel_dsi {
>>>>>
>>>>>     	u8 escape_clk_div;
>>>>>     	u8 dual_link;
>>>>> +	u8 pixel_overlap;
>>>>>     	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 f60146f..f8c2269 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>>>> @@ -288,6 +288,7 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>>>>     	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;
>>>>> +	intel_dsi->pixel_overlap = mipi_config->pixel_overlap;
>>>>>
>>>>>     	if (intel_dsi->dual_link)
>>>>>     		intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C));
>>>>> @@ -310,6 +311,20 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>>>>
>>>>>     	pclk = mode->clock;
>>>>>
>>>>> +	/* In dual link mode each port needs half of pixel clock */
>>>>> +	if (intel_dsi->dual_link) {
>>>>> +		pclk = pclk / 2;
>>>>> +
>>>>> +		/* we can enable pixel_overlap if needed by panel. In this
>>>>> +		 * case we need to increase the pixelclock for extra pixels
>>>>> +		 */
>>>>> +		if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) {
>>>>> +			pclk += DIV_ROUND_UP(mode->vtotal *
>>>>> +						intel_dsi->pixel_overlap *
>>>>> +						60, 1000);
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>>     	/* Burst Mode Ratio
>>>>>     	 * Target ddr frequency from VBT / non burst ddr freq
>>>>>     	 * multiply by 100 to preserve remainder
>>>>> @@ -504,6 +519,12 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>>>>     	DRM_DEBUG_KMS("Clockstop %s\n", intel_dsi->clock_stop ?
>>>>>     						"disabled" : "enabled");
>>>>>     	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "command" : "video");
>>>>> +	if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK)
>>>>> +		DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_FRONT_BACK\n");
>>>>> +	else if (intel_dsi->dual_link == DSI_DUAL_LINK_PIXEL_ALT)
>>>>> +		DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_PIXEL_ALT\n");
>>>>> +	else
>>>>> +		DRM_DEBUG_KMS("Dual link: NONE\n");
>>>>>     	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
>>>>>     	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
>>>>>     	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
>>>>> --
>>>>> 1.7.9.5
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx