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

From: "Singh, Gaurav K" <gaurav.k.singh@intel.com>
To: "Siluvery, Arun" <arun.siluvery@linux.intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Cc: Shobhit Kumar <shobhit.kumar@intel.com>
Subject: Re: [PATCH 04/10] drm/i915: Pixel Clock changes for DSI dual link
Date: Sat, 06 Dec 2014 02:13:05 +0530	[thread overview]
Message-ID: <548218D9.9030205@intel.com> (raw)
In-Reply-To: <5481EFEE.5040302@linux.intel.com>

On 12/5/2014 11:18 PM, Siluvery, Arun wrote:
> 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

Pixel overlap count are fixed values only and it can have values of 0, 1 
or 2. These values will be defined for the particular MIPI DSI panel specs.
These values are programmed in VBT which driver parses and uses it.

With regards,
Gaurav
>
>>
>> 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