Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp4068089rwd; Sat, 3 Jun 2023 19:29:30 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6XddjFVSAkZe4lAV2QQH2MOiCYJ1sZBj9tBO4mUiFFTS6IQlAf2Cxl6H8cQ5tO1NFwkbxk X-Received: by 2002:a5d:8d06:0:b0:776:fce3:4769 with SMTP id p6-20020a5d8d06000000b00776fce34769mr8064603ioj.20.1685845769928; Sat, 03 Jun 2023 19:29:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685845769; cv=none; d=google.com; s=arc-20160816; b=EG1ZvfRdnq9jLYDwErsngaygsUgwBelHIoly9e+4Fzsi3gHURQsigx6y+669XUSX6P Tts6qpkOtYrFmumX0h43VXDUccxXlSt3U4uySpBpuISBGbUcCN8+Nnaz6Xf8B8c1z60e M/D1koyt9YZOSJ1BZmteZXriwAjFn6tNCoRWQeYMtVw1H1p1cy2of9qPl9VS4P9HlyWs +kDv8LBl13OQGM+szwwUYJMXohWL4JK97K/iK7Hich9k8BSPS064fH2U6yvbzOt2dwoc 6oBvetNMd0+KDoUQ+bI9ofzyqouKqtAWCUJLB81fUR36TXHPYvy0aCLZxtn0ro+Nu0Y1 Q9dQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to:from :references:cc:to:content-language:subject:user-agent:mime-version :date:message-id:dkim-signature; bh=94K3cHwmH2/Toiu5F32LRWKtL0L6f5qNOB3VUbafcuY=; b=waQe7pm2Fswh48mJQMhJzyIvtRtoxUJb2X6qnGxrLjEQPiJFLtCN/4wS7blo1PYR1t oe/+nO2ss1AdaFb8og4mWHnqCAYLiDS3n45y8W6EFM4ERBMP9S5ADy9MqMxCrpo9Zpp1 FqqZfVvNDfPue56y/ojHItppOaxqsWgsGfX5z0XkOZ4WIG2pSh9wquMhV9k5qQXMMeeb TP8W0gFBQS+hH3+rb1vTMF5VdwWdCZ+G/Q2fBi9B89XJL7R80oFOIkJlSGq7bo7JICQr 6RGkf05YPrh15CH2oBJSJQRTWCgz4vVyxrrVGSya0fDyAtsgb6fQ4e4e+KvhU9aZONP4 cuzg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=N2ijRtfb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c6-20020a6566c6000000b0053440fe0817si3380955pgw.611.2023.06.03.19.29.13; Sat, 03 Jun 2023 19:29:29 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=N2ijRtfb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229510AbjFDCVP (ORCPT + 99 others); Sat, 3 Jun 2023 22:21:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41948 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229485AbjFDCVN (ORCPT ); Sat, 3 Jun 2023 22:21:13 -0400 Received: from mail-lj1-x229.google.com (mail-lj1-x229.google.com [IPv6:2a00:1450:4864:20::229]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 57105DD for ; Sat, 3 Jun 2023 19:21:10 -0700 (PDT) Received: by mail-lj1-x229.google.com with SMTP id 38308e7fff4ca-2b1b92845e1so11545091fa.0 for ; Sat, 03 Jun 2023 19:21:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685845268; x=1688437268; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=94K3cHwmH2/Toiu5F32LRWKtL0L6f5qNOB3VUbafcuY=; b=N2ijRtfbYZj5FEQJlxZIHBL+kA48Sl+YlXAQbkk1k//9Kh36jywXlE+TFhNx+G+rr0 Air2no4CfLHQfHjReRphAr6Os17Y1ZXJxL/XexdEWNnrP101ilaOQFKwNzbCYO1LNbkr 0SWFh1KJ19bABPPX0nNVWfTjqwKmrPOwiLlo6BiaLzAaB2XeVUYElXke/+AM+/AF6Ybc 2pKCghJwGxdDivLSpvPBqpPJDrZd1TeaJAXcPeYQHJqsdRil3r2A/V2q7FV3tvYKYXR+ JDcVMIi+1C6i91+2ooPoWqIta1A9SDy1MQvceca0RDSHkFdvNMy5+9StpnC48qpv+W3L 2u7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685845268; x=1688437268; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=94K3cHwmH2/Toiu5F32LRWKtL0L6f5qNOB3VUbafcuY=; b=RnaojUncgNkLIsnjBhBv/GBWyVlmh9glqBZOBd37BI/3WLezNrR1EFQ+heFuxc8OPA nEuhhVSoHhYodk5R7ExAmO5S7xhyPz6GDKUr1r/1YxAb8Tb6agx+xTGAcfclGG/DC76R g/rEXJHjeJBU5q6d7WF6cIQIoDg3MEE3sqOJQrPTnvFxIoka8OFMlrSnT3f634DX5COv yDdKcp5qMjTTI62VT5BhgexjH6KPYgP5LEve1ougT41nvaDTlnc7ywM4T6g+VUadXdQJ LzTGb3QkF8uYSrUXKyoVIehowkOJ2BrCLrD83K2ZrlKQcwRTXauPczNpj1zH1wmKeSaL 68wg== X-Gm-Message-State: AC+VfDwPGLysYNEwxkbIRfaoshLI80T8zfEnfuDD+l4ihEmRreB1jhHi pe4iAQu6Fgr8VrHgchnq+32HAg== X-Received: by 2002:a2e:918f:0:b0:2a7:6e85:e287 with SMTP id f15-20020a2e918f000000b002a76e85e287mr2252788ljg.45.1685845268559; Sat, 03 Jun 2023 19:21:08 -0700 (PDT) Received: from ?IPV6:2001:14ba:a0db:1f00::8a5? (dzdqv0yyyyyyyyyyybcwt-3.rev.dnainternet.fi. [2001:14ba:a0db:1f00::8a5]) by smtp.gmail.com with ESMTPSA id r23-20020ac24d17000000b004ec84d24818sm647581lfi.282.2023.06.03.19.21.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 03 Jun 2023 19:21:08 -0700 (PDT) Message-ID: <451b114c-05e1-541c-1d3e-26bb81a307c4@linaro.org> Date: Sun, 4 Jun 2023 05:21:07 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog Content-Language: en-GB To: Abhinav Kumar Cc: freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, quic_khsieh@quicinc.com, Rob Clark , Daniel Vetter , quic_jesszhan@quicinc.com, Marijn Suijten , David Airlie , Sean Paul References: <20230531005358.18090-1-quic_abhinavk@quicinc.com> <0af4df3d-8048-98cd-6c91-7cd553f4f65f@quicinc.com> <98e4bda7-19e9-09b6-f008-383adada97cb@linaro.org> From: Dmitry Baryshkov In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 31/05/2023 21:25, Abhinav Kumar wrote: > > > On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote: >> On 31/05/2023 06:05, Abhinav Kumar wrote: >>> >>> >>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote: >>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar >>>> wrote: >>>>> >>>>> With [1] dpu core revision was dropped in favor of using the >>>>> compatible string from the device tree to select the dpu catalog >>>>> being used in the device. >>>>> >>>>> This approach works well however also necessitates adding catalog >>>>> entries for small register level details as dpu capabilities and/or >>>>> features bloating the catalog unnecessarily. Examples include but >>>>> are not limited to data_compress, interrupt register set, widebus etc. >>>>> >>>>> Introduce the dpu core revision back as an entry to the catalog so >>>>> that >>>>> we can just use dpu revision checks and enable those bits which >>>>> should be enabled unconditionally and not controlled by a catalog >>>>> and also simplify the changes to do something like: >>>>> >>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx) >>>>>          enable the bit; >>>>> >>>>> Also, add some of the useful macros back to be able to use dpu core >>>>> revision effectively. >>>>> >>>>> [1]: >>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4 >>>>> >>>>> Signed-off-by: Abhinav Kumar >>>>> --- >>>>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  1 + >>>>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  1 + >>>>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31 >>>>> ++++++++++++++++++- >>>>>   14 files changed, 43 insertions(+), 1 deletion(-) >>>>> >>>> >>>> [skipped catalog changes] >>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>>>> index 677048cc3b7d..cc4aa75a1219 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>>>> @@ -19,6 +19,33 @@ >>>>>    */ >>>>>   #define MAX_BLOCKS    12 >>>>> >>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\ >>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\ >>>>> +                 ((MINOR & 0xFFF) << 16) |\ >>>>> +                 (STEP & 0xFFFF)) >>>>> + >>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28) >>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF) >>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF) >>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16) >>>>> + >>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \ >>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2))) >>>>> + >>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */ >>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */ >>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */ >>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */ >>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */ >>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */ >>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */ >>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /* qcm2290|sm4125 */ >>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */ >>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */ >>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */ >>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */ >>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */ >>>> >>>> Instead of having defines for all SoCs (which can quickly become >>>> unmanageable) and can cause merge conflicts, I'd suggest inlining all >>>> the defines into respective catalog files. >>>> >>> >>> Sure, that can be done. >>> >>>> Also, I'm not sure that the "step" should be a part of the catalog. I >>>> know that this follows the hardware revision. However, please correct >>>> me if I'm wrong, different step levels are used for revisions of the >>>> same SoC. The original code that was reading the hw revision from the >>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150. >>>> >>> >>> This is one of the things i noticed while making this change. >>> >>> Before the catalog rework, we used to handle even steps as we used to >>> read that from the register and match it with the mdss_cfg handler. >>> But after the rework, we dont handle steps anymore. Yes, you are >>> right that different step levels are used for the revisions of the >>> same SOC and so with that, i dont expect or atleast am not aware of >>> DPU differences between steps but I am not able to rule it out. >>> >>> So are you suggesting we drop step altogether and DPU_HW_VER() macro >>> shall only handle major and minor versions? With the current chipsets >>> I see, it should not make a difference . Its just that I am not sure >>> if that will never happen. >> >> Yes. The goal of this rework would be to drop generic features and to >> replace those checks with DPU-revision lookups. Correct? > > Yes thats right. > >> I think that from this perspective having to handle toe step revision >> is a sign of an overkill. Having to handle the step revision is a sign >> of paltform feature (or mis-feature) rather than a generic DPU bit. >> > > Not entirely. Lets not forget that at the moment even dpu_perf_cfg is > part of the catalog. Even if in terms of major HW blocks steps shouldnt > change, there is absolutely no guarantee that perf data cannot. > > This is what is the sticking point for me which is holding me back > against dropping step. Thoughts? We usually do not support ES versions of the chips, only the final version. So supporting the perf data for earlier revisions is also not required. > >> In fact I suppose that even handling a minor revision would be an >> overkill. Why don't we start with .dpu_major instead of .core_rev? We >> can add .dpu_minor if/when required. >> > > No, unfortunately we cannot drop minor version for sure. I am seeing > examples in downstream code where some of the features are available > after a minor verion as well. Can you please give an example? > >>> >>>>> + >>>>>   #define DPU_HW_BLK_NAME_LEN    16 >>>>> >>>>>   #define MAX_IMG_WIDTH 0x3fff >>>>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg { >>>>>   /** >>>>>    * struct dpu_mdss_cfg - information of MDSS HW >>>>>    * This is the main catalog data structure representing >>>>> - * this HW version. Contains number of instances, >>>>> + * this HW version. Contains dpu core revision, number of instances, >>>>>    * register offsets, capabilities of the all MDSS HW sub-blocks. >>>>>    * >>>>>    * @dma_formats        Supported formats for dma pipe >>>>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg { >>>>>    * @mdss_irqs:         Bitmap with the irqs supported by the target >>>>>    */ >>>>>   struct dpu_mdss_cfg { >>>>> +       u32 core_rev; >>>>> + >>>>>          const struct dpu_caps *caps; >>>>> >>>>>          const struct dpu_ubwc_cfg *ubwc; >>>>> -- >>>>> 2.40.1 >>>>> >>>> >>>> >> -- With best wishes Dmitry