On Sun, Jul 2, 2023 at 7:34 AM Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > > On 02/07/2023 17:31, Rob Clark wrote: > > On Sat, Jul 1, 2023 at 5:24 PM Dmitry Baryshkov > > <dmitry.baryshkov@xxxxxxxxxx> wrote: > >> > >> On Sat, 1 Jul 2023 at 18:50, Rob Clark <robdclark@xxxxxxxxx> wrote: > >>> > >>> On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote: > >>>> > >>>> Recently, a WARN_ON() was introduced to ensure that revn is filled before > >>>> adreno_is_aXYZ is called. This however doesn't work very well when revn is > >>>> 0 by design (such as for A635). Fill it in as a stopgap solution for > >>>> -fixes. > >>>> > >>>> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being set") > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> > >>>> --- > >>>> Changes in v2: > >>>> - add fixes > >>>> - Link to v1: https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c08fb@xxxxxxxxxx > >>>> --- > >>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> index cb94cfd137a8..8ea7eae9fc52 100644 > >>>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > >>>> @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = { > >>>> .address_space_size = SZ_16G, > >>>> }, { > >>>> .rev = ADRENO_REV(6, 3, 5, ANY_ID), > >>>> + .revn = 635, > >>>> .fw = { > >>>> [ADRENO_FW_SQE] = "a660_sqe.fw", > >>>> [ADRENO_FW_GMU] = "a660_gmu.bin", > >>>> > >>> > >>> hmm, I realized a problem with this, it would change what > >>> MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return.. The > >>> former should be "harmless", although it isn't a good idea for uabi > >>> changes to be a side effect of a fix. The latter is more problematic. > >> > >> I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0 > >> in this case)? > >> So the new value should be correct. > > > > no, it is very much intentional that GPU_ID returns 0 for newer GPUs, > > userspace should be matching on CHIP_ID. (Also, we should be moving > > away from trying to infer generation/etc from CHIP_ID.. userspace is > > farther ahead of the kernel on this.) > > Thanks for the explanation. So in theory we can change this to always > return 0? Or must we keep it to keep UABI / compatibility? > > I'm trying to understand if we can drop revn at all. I'm not sure how likely it is that someone would try a very old mesa with a new kernel, but that combo would break if we dropped revn completely and returned 0 for GPU_ID. But I would like to move away from using revn internally. (And probably move away from the patch-id wildcard matching, and instead just explicitly list all known patch-id values so we can start treating CHIP_ID as just an opaque identifier.) BR, -R > > > > >> But more importantly, why are we exporting speedbin in > >> MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting > >> the speedbin at all as a part of CHIP_ID? > > > > Basically just being paranoid about not changing uabi. It probably > > would be ok to export the speedbin for all, but I'd have to double > > check mesa version history. > > Thanks! > > > > > BR, > > -R > > > >>> > >>> I think I'm leaning more towards reverting commit cc943f43ece7 > >>> ("drm/msm/adreno: warn if chip revn is verified before being set") for > >>> -fixes. I'm still thinking about options for a longer term fix. > >>> > >>> BR, > >>> -R > >>> > >>> > >>>> --- > >>>> base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2 > >>>> change-id: 20230628-topic-a635-1b3c2c987417 > >>>> > >>>> Best regards, > >>>> -- > >>>> Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> > >>>> > >> > >> > >> > >> -- > >> With best wishes > >> Dmitry > > -- > With best wishes > Dmitry >