On Thu, Apr 24, 2025 at 10:28:58PM +0200, Konrad Dybcio wrote:
> On 4/23/25 5:23 PM, Dmitry Baryshkov wrote:
> > On 23/04/2025 17:55, Rob Clark wrote:
> >> On Tue, Apr 22, 2025 at 4:57 PM Konrad Dybcio
> >> <konrad.dyb...@oss.qualcomm.com> wrote:
> >>>
> >>> On 4/21/25 10:13 PM, Rob Clark wrote:
> >>>> On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen
> >>>> <quic_akhi...@quicinc.com> wrote:
> >>>>>
> >>>>> On 4/18/2025 6:40 AM, Connor Abbott wrote:
> >>>>>> On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen
> >>>>>> <quic_akhi...@quicinc.com> wrote:
> >>>>>>>
> >>>>>>> On 4/17/2025 9:02 PM, Connor Abbott wrote:
> >>>>>>>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen
> >>>>>>>> <quic_akhi...@quicinc.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 4/10/2025 11:13 PM, Konrad Dybcio wrote:
> >>>>>>>>>> From: Konrad Dybcio <konrad.dyb...@oss.qualcomm.com>
> >>>>>>>>>>
> >>>>>>>>>> The Highest Bank address Bit value can change based on memory type
> >>>>>>>>>> used.
> >>>>>>>>>>
> >>>>>>>>>> Attempt to retrieve it dynamically, and fall back to a reasonable
> >>>>>>>>>> default (the one used prior to this change) on error.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dyb...@oss.qualcomm.com>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 15 ++++++++++++++-
> >>>>>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>>>> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>>>> index
> >>>>>>>>>> 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f
> >>>>>>>>>> 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >>>>>>>>>> @@ -13,6 +13,7 @@
> >>>>>>>>>> #include <linux/firmware/qcom/qcom_scm.h>
> >>>>>>>>>> #include <linux/pm_domain.h>
> >>>>>>>>>> #include <linux/soc/qcom/llcc-qcom.h>
> >>>>>>>>>> +#include <linux/soc/qcom/smem.h>
> >>>>>>>>>>
> >>>>>>>>>> #define GPU_PAS_ID 13
> >>>>>>>>>>
> >>>>>>>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu
> >>>>>>>>>> *gpu)
> >>>>>>>>>>
> >>>>>>>>>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
> >>>>>>>>>> {
> >>>>>>>>>> + int hbb;
> >>>>>>>>>> +
> >>>>>>>>>> gpu->ubwc_config.rgb565_predicator = 0;
> >>>>>>>>>> gpu->ubwc_config.uavflagprd_inv = 0;
> >>>>>>>>>> gpu->ubwc_config.min_acc_len = 0;
> >>>>>>>>>> @@ -635,7 +638,6 @@ static void a6xx_calc_ubwc_config(struct
> >>>>>>>>>> adreno_gpu *gpu)
> >>>>>>>>>> adreno_is_a690(gpu) ||
> >>>>>>>>>> adreno_is_a730(gpu) ||
> >>>>>>>>>> adreno_is_a740_family(gpu)) {
> >>>>>>>>>> - /* TODO: get ddr type from bootloader and use 2 for
> >>>>>>>>>> LPDDR4 */
> >>>>>>>>>> gpu->ubwc_config.highest_bank_bit = 16;
> >>>>>>>>>> gpu->ubwc_config.amsbc = 1;
> >>>>>>>>>> gpu->ubwc_config.rgb565_predicator = 1;
> >>>>>>>>>> @@ -664,6 +666,13 @@ static void a6xx_calc_ubwc_config(struct
> >>>>>>>>>> adreno_gpu *gpu)
> >>>>>>>>>> gpu->ubwc_config.highest_bank_bit = 14;
> >>>>>>>>>> gpu->ubwc_config.min_acc_len = 1;
> >>>>>>>>>> }
> >>>>>>>>>> +
> >>>>>>>>>> + /* Attempt to retrieve the data from SMEM, keep the above
> >>>>>>>>>> defaults in case of error */
> >>>>>>>>>> + hbb = qcom_smem_dram_get_hbb();
> >>>>>>>>>> + if (hbb < 0)
> >>>>>>>>>> + return;
> >>>>>>>>>> +
> >>>>>>>>>> + gpu->ubwc_config.highest_bank_bit = hbb;
> >>>>>>>>>
> >>>>>>>>> I am worried about blindly relying on SMEM data directly for HBB for
> >>>>>>>>> legacy chipsets. There is no guarantee it is accurate on every
> >>>>>>>>> chipset
> >>>>>>>>> and every version of firmware. Also, until recently, this value was
> >>>>>>>>> hardcoded in Mesa which matched the value in KMD.
> >>>>>>>>
> >>>>>>>> To be clear about this, from the moment we introduced host image
> >>>>>>>> copies in Mesa we added support for querying the HBB from the kernel,
> >>>>>>>> explicitly so that we could do what this series does without Mesa
> >>>>>>>> ever
> >>>>>>>> breaking. Mesa will never assume the HBB unless the kernel is too old
> >>>>>>>> to support querying it. So don't let Mesa be the thing that stops us
> >>>>>>>> here.
> >>>>>>>
> >>>>>>> Thanks for clarifying about Mesa. I still don't trust a data source
> >>>>>>> that
> >>>>>>> is unused in production.
> >>>>>>
> >>>>>> Fair enough, I'm not going to argue with that part. Just wanted to
> >>>>>> clear up any confusion about Mesa.
> >>>>>>
> >>>>>> Although, IIRC kgsl did set different values for a650 depending on
> >>>>>> memory type... do you know what source that used?
> >>>>>
> >>>>> KGSL relies on an undocumented devicetree node populated by bootloader
> >>>>> to detect ddrtype and calculates the HBB value based on that.
> >>>>
> >>>> Would it be reasonable to use the smem value, but if we find the
> >>>> undocumented dt property, WARN_ON() if it's value disagrees with smem?
> >>>>
> >>>> That would at least give some confidence, or justified un-confidence
> >>>> about the smem values
> >>>
> >>> The aforementioned value is populated based on the data that this
> >>> driver reads out, and only on the same range of platforms that this
> >>> driver happens to cater to
> >>
> >> Did I understand that correctly to mean that the dt property is based
> >> on the same smem value that you are using? In that case, there should
> >> be no argument against using the smem value as the source of truth.
> >
> > It is, but is done by the bootloader that knows exact format of the data.
>
> Right, so the only point of concern here is the handwavy matching-by-size
> logic.
Kind of. The other issue is several cases where GPU and MDSS drivers
disagree about UBWC config. I think the plan might be:
- Introduce UBWC config database, ruling out incoherences between these
drivers
- Parse SMEM and the non-standard OF property, verify both against each
other and against the UBWC database
- One-by-one drop entries from UBWC database as they are verified
against the readout values.
I understand that it is a long-term plan, but granted the issues we've
had before (tiling screen corruptions, which was fixed / worked around
by lowering HBB) I think this is the most viable path forward.
--
With best wishes
Dmitry
