On 27.07.2023 00:53, Rob Clark wrote: > 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. I think a per-soc dt would be the better approach.. We could probably solve the revision issue with a socid readout of the silicon revision and override based on that? Konrad
