Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <d6c77064-bae5-41c3-b49f-8c5c3a076a6b@rock-chips.com>
Date:   Thu, 16 Nov 2023 16:00:06 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 09/11] drm/rockchip: vop2: Add support for rk3588
Content-Language: en-US
To:     Sascha Hauer <s.hauer@pengutronix.de>
Cc:     Andy Yan <andyshrk@163.com>, heiko@sntech.de, hjc@rock-chips.com,
        dri-devel@lists.freedesktop.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        krzysztof.kozlowski+dt@linaro.org, robh+dt@kernel.org,
        devicetree@vger.kernel.org, sebastian.reichel@collabora.com,
        kever.yang@rock-chips.com, chris.obbard@collabora.com
References: <20231114112534.1770731-1-andyshrk@163.com>
 <20231114112855.1771372-1-andyshrk@163.com>
 <20231115090823.GY3359458@pengutronix.de>
 <229557d7-beec-44e0-9ee6-4a962b33ec79@rock-chips.com>
 <20231116075015.GG3359458@pengutronix.de>
From:   Andy Yan <andy.yan@rock-chips.com>
In-Reply-To: <20231116075015.GG3359458@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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