linux-kernel - Re: [PATCH v2 2/2] drm/mediatek: dpi/dsi: fix possible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <19cac03b-5c3a-43d0-9043-0bf003ce2722@notapiano>
Date:   Fri, 1 Sep 2023 12:17:05 -0400
From:   Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
To:     Michael Walle <mwalle@...nel.org>
Cc:     Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        "Nancy . Lin" <nancy.lin@...iatek.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Jitao Shi <jitao.shi@...iatek.com>,
        Stu Hsieh <stu.hsieh@...iatek.com>,
        dri-devel@...ts.freedesktop.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] drm/mediatek: dpi/dsi: fix possible_crtcs
 calculation

On Fri, Sep 01, 2023 at 11:59:16AM +0200, Michael Walle wrote:
> mtk_drm_find_possible_crtc_by_comp() assumed that the main path will
> always have the CRTC with id 0, the ext id 1 and the third id 2. This
> is only true if the paths are all available. But paths are optional (see
> also comment in mtk_drm_kms_init()), e.g. the main path might not be
> enabled or available at all. Then the CRTC IDs will shift one up, e.g.
> ext will be 1 and the third path will be 2.

ext will be 0 and third will be 1.

> 
> To fix that, dynamically calculate the IDs by the precence of the paths.

precence -> presence

> 
[..]
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -507,6 +507,27 @@ static bool mtk_drm_find_comp_in_ddp(struct device *dev,
[..]
> @@ -526,21 +547,44 @@ unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,
>  						struct device *dev)
>  {
>  	struct mtk_drm_private *private = drm->dev_private;
> -	unsigned int ret = 0;
> -
> -	if (mtk_drm_find_comp_in_ddp(dev, private->data->main_path, private->data->main_len,
> -				     private->ddp_comp))
> -		ret = BIT(0);
> -	else if (mtk_drm_find_comp_in_ddp(dev, private->data->ext_path,
> -					  private->data->ext_len, private->ddp_comp))
> -		ret = BIT(1);
> -	else if (mtk_drm_find_comp_in_ddp(dev, private->data->third_path,
> -					  private->data->third_len, private->ddp_comp))
> -		ret = BIT(2);
> -	else
> -		DRM_INFO("Failed to find comp in ddp table\n");
> +	const struct mtk_mmsys_driver_data *data;
> +	struct mtk_drm_private *priv_n;
> +	int i = 0, j;
> +
> +	for (j = 0; j < private->data->mmsys_dev_num; j++) {
> +		priv_n = private->all_drm_private[j];
> +		data = priv_n->data;
> +
> +		if (mtk_ddp_path_available(data->main_path, data->main_len,
> +					   priv_n->comp_node)) {
> +			if (mtk_drm_find_comp_in_ddp(dev, priv_n->data->main_path,
> +						     priv_n->data->main_len,
> +						     priv_n->ddp_comp))

My only nit is that you're sometimes using data, sometimes using priv_n->data.
Just use data everywhere.

Otherwise,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>

Tested on both mt8192-asurada-spherion and mt8195-cherry-tomato. Confirmed that
all configurations work on both machines: internal+external, only internal and
only external display.

(Like you mentioned, I also noticed that when only the external display is
enabled, it works during early boot, but then it stops working, and only after 
reconnecting it does it work again, but that's a separate issue)

Thanks,
Nícolas