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

[Sascha Hauer]

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.

> 
> > 
> > 
> > > +		*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


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |