2014-09-25 8:27 GMT+02:00 Zhao, Yakui <[email protected]>: > On Wed, 2014-09-24 at 23:35 -0600, Gwenole Beauchesne wrote: >> Hi, >> >> 2014-09-25 3:00 GMT+02:00 Zhao, Yakui <[email protected]>: >> > On Wed, 2014-09-24 at 03:10 -0600, Gwenole Beauchesne wrote: >> >> Hi, >> >> >> >> Great to see that finally fixed. >> >> >> >> 2014-09-24 9:13 GMT+02:00 Zhao Yakui <[email protected]>: >> >> > On some systems there is no access to /proc/cpuinfo. So the inline >> >> > assembly >> >> > is used directly to detect the CPUID string. >> >> > >> >> > Signed-off-by: Zhao Yakui <[email protected]> >> >> > --- >> >> > src/i965_device_info.c | 46 >> >> > ++++++++++++++++++++++++++++++++++++++++++++++ >> >> > 1 file changed, 46 insertions(+) >> >> > >> >> > diff --git a/src/i965_device_info.c b/src/i965_device_info.c >> >> > index 5ebea2a..4ffba61 100755 >> >> > --- a/src/i965_device_info.c >> >> > +++ b/src/i965_device_info.c >> >> > @@ -378,6 +378,7 @@ i965_get_device_info(int devid) >> >> > } >> >> > } >> >> > >> >> > +#if 0 >> >> > static int intel_driver_detect_cpustring(char *model_id) >> >> > { >> >> > FILE *fp; >> >> > @@ -416,7 +417,52 @@ static int intel_driver_detect_cpustring(char >> >> > *model_id) >> >> > else >> >> > return -EINVAL; >> >> > } >> >> > +#else >> >> > +static inline void cpuid(unsigned int op, >> >> > + unsigned int *eax, unsigned int *ebx, >> >> > + unsigned int *ecx, unsigned int *edx) >> >> >> >> Use uint32_t so that to not make the code below confusing when >> >> different 32-bit/64-bit asm is involved and you are sure the operands >> >> are 32-bits, just say so. >> >> >> > >> > OK. I can change it to uint32_t. >> > >> > (In fact the "unsigned int" is treated as 32 bit). >> >> And? You can't claim in a previous thread to aim for better human >> readability, and now claiming you don't care because of unsigned int >> == uint32_t? > > Generally speaking, the unsigned int will always be equal to > "uint32_t". > > I don't know what you want to say. > >> >> > +{ >> >> > + *eax = op; >> >> > + *ecx = 0; >> >> > + /* ecx is often an input as well as an output. */ >> >> > + asm volatile("cpuid" >> >> > + : "=a" (*eax), >> >> > + "=b" (*ebx), >> >> > + "=c" (*ecx), >> >> > + "=d" (*edx) >> >> > + : "0" (*eax), "2" (*ecx) >> >> > + :"memory"); >> >> > +} >> >> >> >> This will likely fail in 32-bit mode. %ebx is the PIC register there. >> >> e.g. use %edi as a temporary in this case. And split into two >> >> versions. >> >> >> > >> > I don't think that it is necessary to split it into two version. It can >> > work on both 32/64-bit version. >> >> I am 100% sure your code won't even compile in 32-bit mode with PIC >> code, which is the default when compiling a DSO. Are you denying this? > > In my test I don't compile it with PIC flag. > But as I mentioned that the above code is similar to the implementation > in Linux kernel, I am sure that it is safe on 32/64.
Try to build libva with your patch applied in 32-bit. It won't compile. I would be very surprised if it does. I mentioned to you that the code was not going to work in 32-bit mode with PIC (to build a DSO). Again, are you denying this? >> Oh, BTW, forgot to mention: use __asm__ __volatile__. Though, I reckon >> this would be too much of nitpicking, :) > > I am OK to use __asm__ __volatile__. It will be updated in next > version. > >> >> > When the inline assembly is handled, the compiler will help to arrange >> > the used register list and do some extra protections. In such case the >> > programmer don't care the used register list. >> >> Is your comment supposed to attest that you still think your code is >> going to work in 32-bit mode and you don't want to change it? >> >> > I suggest that you can take a look at the cpuid implementation in >> > Linux_kernel. >> > The following is another example of cpuid. >> > http://www.ibm.com/developerworks/library/l-ia/ >> > >> > (In fact the above similar code can be found in Linux kernel. Be sure >> > that it can work on 32-bit/64-bit version. And it has no problem about >> > the used register list). >> >> Are you saying that because your code is "similar" to Kernel code, so >> you are sure it works for you here? Honestly? > > Yes. > >> Or, are you saying "similar" so that we don't think you copied GPLv2 >> code snippet as is to bring it into a MIT-like licensed project? > > After reading the spec of GCC inline assembly and CPUID instruction, we > can also write the similar style. But it is accidently the same. > The following is one example about cpuid inline instruction, which is > described in http://www.ibm.com/developerworks/library/l-ia > > asm ("cpuid" > : "=a" (_eax), > "=b" (_ebx), > "=c" (_ecx), > "=d" (_edx) > : "a" (op)); > > After the above exmaple is reworked, it can be similar to the > implementation in kernel. If you really followed the sample code your provided, you wouldn't have that *eax = op; *ecx = 0; code style. You would have been using clear outputs/inputs, i.e. "=a" (*eax) ... "0" (op), not the code you provided in patch. Sorry, but I don't buy your argument, you make impossible statements. > OK. I will use what Matt Turner suggested to obtain the CPUID string. > > Thanks. > Yakui > >> >> Though, cpuid should be trivial code, but here, it's additionally broken. >> >> >> Besides, choose between in/out operands (+) and keep *eax/*ecx >> >> initializations, or use separate outputs and inputs and initialize >> >> eax/ecx there, e.g. "a" (op), "c" (0) and drop the *eax/*ecx lines. >> >> Don't mix both. >> >> >> >> > + >> >> > +/* >> >> > + * This function doesn't check the length. And the caller should >> >> > + * assure that the length of input string should be greater than 48. >> >> > + */ >> >> > +static int intel_driver_detect_cpustring(char *model_id) >> >> >> >> This is totally useless then, worded as is, and doesn't bring in more >> >> robustness. Use dynamic allocation, or model_id + length of model_id. >> >> >> >> > +{ >> >> > + unsigned int *rdata; >> >> > >> >> > + if (model_id == NULL) >> >> > + return -EINVAL; >> >> > + >> >> > + rdata = (unsigned int *)model_id; >> >> > + >> >> > + /* obtain the max supported extended CPUID info */ >> >> > + cpuid(0x80000000, &rdata[0], &rdata[1], &rdata[2], &rdata[3]); >> >> > + >> >> > + /* If the max extended CPUID info is less than 0x80000004, fail */ >> >> > + if (rdata[0] < 0x80000004) >> >> > + return -EINVAL; >> >> > + >> >> > + /* obtain the CPUID string */ >> >> > + cpuid(0x80000002, &rdata[0], &rdata[1], &rdata[2], &rdata[3]); >> >> > + cpuid(0x80000003, &rdata[4], &rdata[5], &rdata[6], &rdata[7]); >> >> > + cpuid(0x80000004, &rdata[8], &rdata[9], &rdata[10], &rdata[11]); >> >> > + >> >> > + *(model_id + 48) = '\0'; >> >> >> >> That's model_id[48] = '\0'; much simpler to read. >> >> >> >> > + return 0; >> >> > +} >> >> > +#endif >> >> > /* >> >> > * the hook_list for HSW. >> >> > * It is captured by /proc/cpuinfo and the space character is stripped. >> >> > -- >> >> > 1.7.10.1 >> >> > >> >> > _______________________________________________ >> >> > Libva mailing list >> >> > [email protected] >> >> > http://lists.freedesktop.org/mailman/listinfo/libva >> >> >> >> Regards, >> > >> > >> >> >> > > -- Gwenole Beauchesne Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France Registration Number (RCS): Nanterre B 302 456 199 _______________________________________________ Libva mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libva
