From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751800AbdEVFqL (ORCPT <rfc822;w@1wt.eu>);
        Mon, 22 May 2017 01:46:11 -0400
Received: from mailgw01.mediatek.com ([210.61.82.183]:64271 "EHLO
        mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org
        with ESMTP id S1751710AbdEVFqK (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 22 May 2017 01:46:10 -0400
Message-ID: <1495431964.20811.8.camel@mtksdaap41>
Subject: Re: [PATCH v2] drm: mediatek: change the variable type of rdma
 threshold
From: CK Hu <ck.hu@mediatek.com>
To: Bibby Hsieh <bibby.hsieh@mediatek.com>
CC: David Airlie <airlied@linux.ie>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        <dri-devel@lists.freedesktop.org>,
        <linux-mediatek@lists.infradead.org>,
        Yingjoe Chen <yingjoe.chen@mediatek.com>,
        Cawa Cheng <cawa.cheng@mediatek.com>,
        Daniel Kurtz <djkurtz@chromium.org>,
        "Philipp Zabel" <p.zabel@pengutronix.de>,
        YT Shen <yt.shen@mediatek.com>,
        "Thierry Reding" <thierry.reding@gmail.com>,
        Mao Huang <littlecvr@chromium.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        "Sascha Hauer" <kernel@pengutronix.de>
Date: Mon, 22 May 2017 13:46:04 +0800
In-Reply-To: <1495187843-6882-1-git-send-email-bibby.hsieh@mediatek.com>
References: <1495187843-6882-1-git-send-email-bibby.hsieh@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.2.3-0ubuntu6 
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MTK: N
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi, Bibby:

One comment inline.

On Fri, 2017-05-19 at 17:57 +0800, Bibby Hsieh wrote:
> For some greater resolution, the rdma threshold
> variable will overflow.
> 
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> index 0df05f9..9afdcd7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> @@ -37,7 +37,7 @@
>  #define DISP_REG_RDMA_FIFO_CON			0x0040
>  #define RDMA_FIFO_UNDERFLOW_EN				BIT(31)
>  #define RDMA_FIFO_PSEUDO_SIZE(bytes)			(((bytes) / 16) << 16)
> -#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes)		((bytes) / 16)
> +#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)

I think it's not necessary to do this mask operation. Before calling
RDMA_OUTPUT_VALID_FIFO_THRESHOLD(), you should make sure that width,
height, and vrefresh matches the HW spec, so the result of threshold
likely does not exceed 0x3ff. If width, height, and vrefresh matches the
HW spec but threshold exceed 0x3ff, maybe you should limited it to 0x3ff
rather than truncating it.

Regards,
CK

>  
>  /**
>   * struct mtk_disp_rdma - DISP_RDMA driver structure
> @@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
>  			    unsigned int height, unsigned int vrefresh,
>  			    unsigned int bpc)
>  {
> -	unsigned int threshold;
> +	unsigned long long threshold;
>  	unsigned int reg;
>  
>  	rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
> @@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
>  	 * output threshold to 6 microseconds with 7/6 overhead to
>  	 * account for blanking, and with a pixel depth of 4 bytes:
>  	 */
> -	threshold = width * height * vrefresh * 4 * 7 / 1000000;
> +	threshold = (unsigned long long)width * height * vrefresh *
> +		    4 * 7 / 1000000;
>  	reg = RDMA_FIFO_UNDERFLOW_EN |
>  	      RDMA_FIFO_PSEUDO_SIZE(SZ_8K) |
>  	      RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);