On 8 November 2016 at 08:44, Michel Dänzer <michel at daenzer.net> wrote: > On 07/11/16 08:30 PM, Emil Velikov wrote: >> On 7 November 2016 at 09:14, Michel Dänzer <michel at daenzer.net> wrote: >>> On 05/11/16 03:14 AM, Emil Velikov wrote: >>>> On 2 November 2016 at 03:07, Michel Dänzer <michel at daenzer.net> wrote: >>>>> >>>>> The first attached patch will result in drmParsePciDeviceInfo always >>>>> reporting revision 0 on kernels without the second attached patch. Will >>>>> that be an issue for the amdgpu-pro stack? >>>>> >>>>> Please follow up directly to the patch e-mails with any comments on the >>>>> patches. >>>>> >>>> Fleshing out the question from the actual patches: >>>> >>>> Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision >>>> field as returned by the drmDevice API ? >>> >>> One answer is that https://patchwork.freedesktop.org/patch/120132/ uses >>> the revision ID. In this case a wrong revision ID would only cause a >>> cosmetic issue, but I can imagine that other code in the amdgpu-pro >>> stack really needs the correct revision ID to accurately identify the GPU. >>> >> Don't mean to sound rude, but I was hoping for a definite answer. > > So was I. :} > > Digging further, the above patch actually doesn't use the revision_id > field but amdgpu kernel driver functionality to determine the revision. > Given that such functionality exists, I don't think we have do any more > special consideration of either amdgpu stack. > > >> Regardless, do you/fellow AMD devs, any preference on how to go with >> this bug [1] ? >> >> Add an override to force use of the revision file - be that envvar, >> new API {drmDeviceUseRevisionFile, drmDevice...v2}, or revert the 12 + >> commits (pulling only the offending one won't cut it). Obviously I'm >> not a huge fan of the last one :-\ >> >> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > I'm afraid I don't have any particularly strong opinion to offer here. > But it seems weird to me to have an API which pretends to provide the > revision ID, but it can actually be incorrect. (The same would apply to > any other information, not just the revision ID in particular) > Indeed it is quite nasty. A quick and short solution, which doesn't break existing users, is to have drmDeviceForceUseRevisionFile(). I'll give it a stab, adding a juicy comment/doc about it. As we get to libdrm3 we can remove this/other hacks, but until then this will do just fine ? Thanks Emil
