RE: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge

[Milind Parab]

Hi Tomi,

>-----Original Message-----
>From: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>Sent: Tuesday, September 1, 2020 2:05 PM
>To: Swapnil Kashinath Jakhade <sjakhade@xxxxxxxxxxx>; airlied@xxxxxxxx;
>daniel@xxxxxxxx; Laurent.pinchart@xxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
>a.hajda@xxxxxxxxxxx; narmstrong@xxxxxxxxxxxx; jonas@xxxxxxxxx;
>jernej.skrabec@xxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Cc: Milind Parab <mparab@xxxxxxxxxxx>; Yuti Suresh Amonkar
><yamonkar@xxxxxxxxxxx>; jsarha@xxxxxx; nsekhar@xxxxxx;
>praneeth@xxxxxx; nikhil.nd@xxxxxx
>Subject: Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546
>DPI/DP bridge
>
>EXTERNAL MAIL
>
>
>Hi Swapnil,
>
>On 31/08/2020 11:23, Swapnil Jakhade wrote:
>
>> +	line_thresh1 = ((vs + 1) << 5) * 8 / bpp;
>> +	line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5);
>> +	line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes;
>> +	line_thresh = (line_thresh >> 5) + 2;
>
>These calculations do not seem to go correctly. There's no comment what's
>the logic here, but e.g.
>for 640x480 (pxlclock 31500) with 1.62Gbps link, I get vs=4, and then the
>second line above comes to:
>
>(31500 << 5) / 1000 / 162 * (4+1) - (1<<5) = -0.8888888888888893
>
>The result is line_thresh of 100663299.
>

Yes, there is a mistake in the above equations. I will detailed it here
Before that there are other related issues which also needs clarification

First is about the TU_SIZE.
The DP spec says, in SST mode the TU_SIZE can take any even value from 32 to 64.
The advantages of having a smaller TU size is not explicitly mentioned, however logically it helps stream down-spreading and would be beneficial in low bandwidth low buffer applications.
Hence, we can consider that selecting a lower TU size is an optimization we can consider implementing later.
For now let us fix TU_SIZE = 64

Second, is Valid Symbol Length "vs"
For a fixed TU valid symbol length will depend in the Ratio or Pixel clock and Link symbol Rate (lanes * rate)
vs = 64 * required_bandwidth  / available_bandwidth

Where, 
required_bandwidth = pxlclock * (bpp/8)
available_bandwidth = mhdp->link.num_lanes * mhdp->link.rate

Also, note that CDNS MHDP implements DP_FRAMER_TU_p where bits 5:0 is tu_valid_symbols. So max programmable value is 63.
Register document gives following explanation 
"Number of valid symbols per Transfer Unit (TU). Rounded down to lower integer value (Allowed values are 1 to (TU_size-1)"

So, it says in case vs calculates to 64 (where Avail BW and Req BW are same) we program tu_valid_symbols = 63

Third, is about the line_threshold calculation
Unlike TU_SIZE and Valid_Symbols, line_threshold is implementation dependent

CDNS MHDP register specs gives the definition as " Video FIFO latency threshold" 
Bits 5:0, Name "cfg_active_line_tresh", Description "Video Fifo Latency threshold. Defines the number of FIFO rows before reading starts. This setting depends on the transmitted video format and link rate."

This parameter is the Threshold of the FIFO. For optimal performance (considering equal write and read clock) we normally put the threshold in the mid of the FIFO.
Hence the reset value is fixed as 32.
Since symbol FIFO is accessed by Pxl clock and Symbol Link Clock the Threshold is set to a value which is dependent on the ratio of these clocks

line_threshold = full_fifo - fifo_ratio_due_to_clock_diff + 2
where,
full_fifo = (vs+1) * (8/bpp)
fifo_ratio_due_to_clock_diff = ((vs+1) * pxlclock/mhdp->link.rate - 1) / mhdp->link.num_lanes 

Note that line_threshold can take a max value of 63


> Tomi
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki