<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols"> <p></p> <div>>From: Chris Wilson <chris@chris-wilson.co.uk></div> <div>></div> <div>>Sent: Monday, December 14, 2020 2:15 PM</div> <div>>To: Chang, Yu bruce; intel-gfx@lists.freedesktop.org</div> <div>>Subject: Re: [Intel-gfx] [PATCH i-g-t] lib: Pass device fd to gem_mmappable_aperture_size()</div> <div>> </div> <div>>Quoting Chang, Yu bruce (2020-12-14 21:52:10)</div> <div>>> </div> <div>>> ></div> <div>>> >From: Chris Wilson <chris@chris-wilson.co.uk></div> <div>>> >Sent: Monday, December 14, 2020 12:48 PM</div> <div>>> >To: Chang, Yu bruce; intel-gfx@lists.freedesktop.org</div> <div>>> >Cc: igt-dev@</div> <div>>> >Subject: Re: [Intel-gfx] [PATCH i-g-t] lib: Pass device fd to</div> <div>>> gem_mmappable_aperture_size()</div> <div>>> > </div> <div>>> >Quoting Chang, Yu bruce (2020-12-14 18:45:04)</div> <div>>> >> +/**</div> <div>>> >> + * gem_mappable_aperture_size:</div> <div>>> >> + *</div> <div>>> >> + * Feature test macro to query the kernel for the mappable gpu aperture</div> <div>>> size.</div> <div>>> >> + * This is the area available for GTT memory mappings.</div> <div>>> >> + *</div> <div>>> > + * Returns: The mappable gtt address space size.</div> <div>>> > + */</div> <div>>> > +uint64_t gem_mappable_aperture_size(int fd)</div> <div>>> > +{</div> <div>>> > + struct pci_device *pci_dev = igt_device_get_pci_device(fd);</div> <div>>> > </div> <div>>> > Does it make sense to eliminate the function intel_get_pci_device() if not</div> <div>>> > being used anymore? But it can be a separate patch.</div> <div>>> ></div> <div>>> >It's still used by tools. The complication there is that we mostly</div> <div>>> >need to lookup the pci device without loading i915.ko. </div> <div>>> >-Chris</div> <div>>> ></div> <div>>> </div> <div>>> That makes sense.</div> <div>>> </div> <div>>> Then we need to make sure not start from a fix slot to look for GPU device in</div> <div>>> the intel_get_pci_device() below as</div> <div>>> it may not work for a discrete GPU as that slot can be a non-vga device but</div> <div>>> with vendor_id 0x8086.</div> <div>>> </div> <div>>> pci_dev = pci_device_find_by_slot(0, 0, 2, 0);</div> <div>>> if (pci_dev == NULL || pci_dev->vendor_id != 0x8086) {</div> <div>>> </div> <div>>> So, either add extra check to make sure it is VGA class or always use </div> <div>>> pci_device_next to search.</div> <div>></div> <div>>It's held true for ~20 years :)</div> <div>></div> <div>>I hear you; for the remaining users, they should probably use the lsgpu</div> <div>>interface to pick the right device to work on (and remove</div> <div>>intel_get_pci_device).</div> <div>></div> <div>>tools/intel_audio_dump.c</div> <div>>tools/intel_backlight.c</div> <div>>tools/intel_display_poller.c</div> <div>>tools/intel_forcewaked.c</div> <div>>tools/intel_gpu_time.c</div> <div>>tools/intel_gtt.c</div> <div>>tools/intel_infoframes.c</div> <div>>tools/intel_lid.c</div> <div>>tools/intel_panel_fitter.c</div> <div>>tools/intel_reg.c</div> <div>>tools/intel_reg_checker.c</div> <div>>tools/intel_watermark.c</div> <div>></div> <div>>A few of those could even be retired.</div> <div>>-Chris</div> <div>></div> <div>></div> <div><br> </div> <div>Sounds reasonably to me. the rest of your changes look good to me, and also fix my issue.</div> <div><br> </div> <div>Thanks,</div> <div>Bruce</div> <div><br> </div> <div><font face="Calibri,Helvetica,sans-serif,EmojiFont,Apple Color Emoji,Segoe UI Emoji,NotoColorEmoji,Segoe UI Symbol,Android Emoji,EmojiSymbols" size="1" style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;"><span style="font-size: 13.32px;">Reviewed-by: </span></font><font face="Calibri,Helvetica,sans-serif,EmojiFont,Apple Color Emoji,Segoe UI Emoji,NotoColorEmoji,Segoe UI Symbol,Android Emoji,EmojiSymbols" size="1" style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;"><span style="font-size: 13.32px;">Bruce Chang <yu.bruce.chang@intel.com></span></font><br> </div> <br> <p></p> <div style="color:rgb(0,0,0)"></div> </div> </body> </html>