Hi Sascha:
On 11/16/23 15:50, Sascha Hauer wrote:
On Thu, Nov 16, 2023 at 03:24:54PM +0800, Andy Yan wrote:
case ROCKCHIP_VOP2_EP_HDMI0:
case ROCKCHIP_VOP2_EP_HDMI1:
...
}
would look a bit better overall.
+ /*
+ * K = 2: dclk_core = if_pixclk_rate > if_dclk_rate
+ * K = 1: dclk_core = hdmie_edp_dclk > if_pixclk_rate
+ */
+ if (output_mode == ROCKCHIP_OUT_MODE_YUV420) {
+ dclk_rate = dclk_rate >> 1;
+ K = 2;
+ }
+
+ if_pixclk_rate = (dclk_core_rate << 1) / K;
+ if_dclk_rate = dclk_core_rate / K;
+
+ *if_pixclk_div = dclk_rate / if_pixclk_rate;
+ *if_dclk_div = dclk_rate / if_dclk_rate;
Not sure if this will change with future extensions, but currently
*if_pixclk_div will always be 2 and *if_dclk_div will alway be 4,
so maybe better write it like this
Yes, the calculation of *if_pixclk_div is always 2 and *if_dclk_div is always 4,
I think calculation formula can give us a clear explanation why is 2 or 4.
considering the great power of rk3588, i think it can calculate it very easy.
Sure it can. My concern is not the CPU time it takes to do that
equation, but more the readability of the code. For me as a reader it
might be more easily acceptable that both dividers have fixed values
than it is to understand the equation.
Your mileage may vary, I won't insist on this.
Or I make it as fixed values, and leave the calculation formula as comments ?
+ *dclk_core_div = dclk_rate / dclk_core_rate;
*dclk_core_div is calculated the same way for all cases. You could pull
this out of the if/else.
Okay, will do.
+ } else if (vop2_output_if_is_edp(id)) {
+ /* edp_pixclk = edp_dclk > dclk_core */
+ if_pixclk_rate = v_pixclk / K;
+ if_dclk_rate = v_pixclk / K;
if_dclk_rate is unused here.
It will be removed in next version.
+ dclk_rate = if_pixclk_rate * K;
+ *dclk_core_div = dclk_rate / dclk_core_rate;
+ *if_pixclk_div = dclk_rate / if_pixclk_rate;
+ *if_dclk_div = *if_pixclk_div;
Both *if_pixclk_div and *if_dclk_div will always be 1.
Actually, they will be the value of K here, if it work at split mode(two
edp connect to one VP, one show the image for left half, one for right half,
a function like a dual channel mipi dsi).
I know it split mode is not supported by the current mainline, but i think keep
Ok.
Sascha
