Re: [PATCH 09/11] drm/rockchip: vop2: Add support for rk3588

[Andy Yan]

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