On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > > On Thu, 27 Jul 2023 at 01:04, Rob Clark <robdclark@xxxxxxxxx> wrote: > > > > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov > > <dmitry.baryshkov@xxxxxxxxxx> wrote: > > > > > > On 26/07/2023 23:11, Rob Clark wrote: > > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov > > > > <dmitry.baryshkov@xxxxxxxxxx> wrote: > > > >> > > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark <robdclark@xxxxxxxxx> wrote: > > > >>> > > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen <quic_akhilpo@xxxxxxxxxxx> wrote: > > > >>>> > > > >>>> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote: > > > >>>>> > > > >>>>> On 07/07/2023 00:10, Rob Clark wrote: > > > >>>>>> From: Rob Clark <robdclark@xxxxxxxxxxxx> > > > >>>>>> > > > >>>>>> There are cases where there are differences due to SoC integration. > > > >>>>>> Such as cache-coherency support, and (in the next patch) e-fuse to > > > >>>>>> speedbin mappings. > > > >>>>> > > > >>>>> I have the feeling that we are trying to circumvent the way DT works. I'd > > > >>>>> suggest adding explicit SoC-compatible strings to Adreno bindings and then > > > >>>>> using of_device_id::data and then of_device_get_match_data(). > > > >>>>> > > > >>>> Just thinking, then how about a unique compatible string which we match > > > >>>> to identify gpu->info and drop chip-id check completely here? > > > >>> > > > >>> Ok, I think we could do this, so something like: > > > >>> > > > >>> compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno" > > > >>> > > > >>> ? > > > >>> > > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350 > > > >>> or sm6375, so I suppose we could get away with this change > > > >> > > > >> I think we can even skip the 619.0 part in the SoC compat string. > > > >> So it will be: > > > >> > > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno"; > > > >> > > > >> In future we can drop the chipid part completely and handle that as a > > > >> part of SoC data: > > > >> > > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno"; > > > >> > > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0) > > > >> > > > > > > > > I don't think we can do that, there are cases where the same SoC had > > > > multiple revisions of adreno. > > > > > > Is that the case for the production versions of the SoC? In other > > > subsystems what we usually do is that we add support only for the latest > > > SoC revision (which would probably mean the latest GPU patch revision). > > > Previous GPU revisions can be added in the following way (pure example): > > > > > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial > > > sample > > > qcom,sm4350-v1-adreno -> 6,1,9,0 > > > > > > > My recollection was that nexus4 shipped with an early version of 8064 > > which needed userspace workarounds that later 8064 did not. Not sure > > if that is the only such example, but it is one that userspace needed > > to be aware of. > > Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine. > > And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string. I no longer have a n4 that boots.. but if I did both it and the later ones should work properly if they expose the appropriate chip id I do still prefer parsing the chip-id out of the compatible. It avoids needing separate table entries just to have a different chip-id. Maybe the scheme that is used elsewhere makes sense when it is only the kernel that needs to be aware of the device-id. And maybe we could just done matching based on compat-id in userspace as well, but (a) msm and freedreno pre-date dt, and (b) that ship has already sailed. BR, -R > > > > Anyways, future things, it sounds like we'll be able to read the id > > from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, > > so I don't want to change any of the existing compat strings. > > I think so too. Current compat strings should stay. > > > > > BR, > > -R > > > > > > We could possibly do that with future > > > > things where we can read the chip-id from fw. > > > > > > > > What we _could_ do at the expense of making the compatible parsing a > > > > tiny bit more complex is something like: > > > > > > > > compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno" > > > > > > > > BR, > > > > -R > > > > > > > >>> > > > >>> BR, > > > >>> -R > > > >>> > > > >>>> -Akhil > > > >>>> > > > >>>>>> > > > >>>>>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx> > > > >>>>>> --- > > > >>>>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++--- > > > >>>>>> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 + > > > >>>>>> 2 files changed, 31 insertions(+), 4 deletions(-) > > > >>>>>> > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > >>>>>> index 3c531da417b9..e62bc895a31f 100644 > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > > >>>>>> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = { > > > >>>>>> .inactive_period = DRM_MSM_INACTIVE_PERIOD, > > > >>>>>> .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT, > > > >>>>>> .init = a6xx_gpu_init, > > > >>>>>> + }, { > > > >>>>>> + .machine = "qcom,sm4350", > > > >>>>>> + .rev = ADRENO_REV(6, 1, 9, ANY_ID), > > > >>>>>> + .revn = 619, > > > >>>>>> + .fw = { > > > >>>>>> + [ADRENO_FW_SQE] = "a630_sqe.fw", > > > >>>>>> + [ADRENO_FW_GMU] = "a619_gmu.bin", > > > >>>>>> + }, > > > >>>>>> + .gmem = SZ_512K, > > > >>>>>> + .inactive_period = DRM_MSM_INACTIVE_PERIOD, > > > >>>>>> + .init = a6xx_gpu_init, > > > >>>>>> + .zapfw = "a615_zap.mdt", > > > >>>>>> + .hwcg = a615_hwcg, > > > >>>>>> + }, { > > > >>>>>> + .machine = "qcom,sm6375", > > > >>>>>> + .rev = ADRENO_REV(6, 1, 9, ANY_ID), > > > >>>>>> + .revn = 619, > > > >>>>>> + .fw = { > > > >>>>>> + [ADRENO_FW_SQE] = "a630_sqe.fw", > > > >>>>>> + [ADRENO_FW_GMU] = "a619_gmu.bin", > > > >>>>>> + }, > > > >>>>>> + .gmem = SZ_512K, > > > >>>>>> + .inactive_period = DRM_MSM_INACTIVE_PERIOD, > > > >>>>>> + .init = a6xx_gpu_init, > > > >>>>>> + .zapfw = "a615_zap.mdt", > > > >>>>>> + .hwcg = a615_hwcg, > > > >>>>>> }, { > > > >>>>>> .rev = ADRENO_REV(6, 1, 9, ANY_ID), > > > >>>>>> .revn = 619, > > > >>>>>> @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev) > > > >>>>>> /* identify gpu: */ > > > >>>>>> for (i = 0; i < ARRAY_SIZE(gpulist); i++) { > > > >>>>>> const struct adreno_info *info = &gpulist[i]; > > > >>>>>> + if (info->machine && !of_machine_is_compatible(info->machine)) > > > >>>>>> + continue; > > > >>>>>> if (adreno_cmp_rev(info->rev, rev)) > > > >>>>>> return info; > > > >>>>>> } > > > >>>>>> @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) > > > >>>>>> config.rev.minor, config.rev.patchid); > > > >>>>>> priv->is_a2xx = config.rev.core == 2; > > > >>>>>> + priv->has_cached_coherent = > > > >>>>>> + !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT); > > > >>>>>> gpu = info->init(drm); > > > >>>>>> if (IS_ERR(gpu)) { > > > >>>>>> @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) > > > >>>>>> if (ret) > > > >>>>>> return ret; > > > >>>>>> - priv->has_cached_coherent = > > > >>>>>> - !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) && > > > >>>>>> - !adreno_has_gmu_wrapper(to_adreno_gpu(gpu)); > > > >>>>>> - > > > >>>>>> return 0; > > > >>>>>> } > > > >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > > >>>>>> index e08d41337169..d5335b99c64c 100644 > > > >>>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > > >>>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h > > > >>>>>> @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h > > > >>>>>> extern const struct adreno_reglist a660_hwcg[], a690_hwcg[]; > > > >>>>>> struct adreno_info { > > > >>>>>> + const char *machine; > > > >>>>>> struct adreno_rev rev; > > > >>>>>> uint32_t revn; > > > >>>>>> const char *fw[ADRENO_FW_MAX]; > > > >>>>> > > > >>>>> -- > > > >>>>> With best wishes > > > >>>>> Dmitry > > > >>>>> > > > >> > > > >> > > > >> > > > >> -- > > > >> With best wishes > > > >> Dmitry > > > > > > -- > > > With best wishes > > > Dmitry > > > > > > > -- > With best wishes > Dmitry