Re: [PATCH v8 2/2] drm: add kms driver for loongson display controller

From: Sui Jingfeng
Date: Tue Mar 28 2023 - 22:27:10 EST


On 2023/3/29 01:06, Nathan Chancellor wrote:
On Tue, Mar 28, 2023 at 11:22:50PM +0800, Sui Jingfeng wrote:
HI,

On 2023/3/28 17:27, kernel test robot wrote:
Hi Sui,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/MAINTAINERS-add-maintainers-for-DRM-LOONGSON-driver/20230320-180408
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230320100131.1277034-3-15330273260%40189.cn
patch subject: [PATCH v8 2/2] drm: add kms driver for loongson display controller
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230328/202303281754.jWI20j2C-lkp@xxxxxxxxx/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/80b4115f44993f4ebf47b1cb9e8f02953575b977
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sui-Jingfeng/MAINTAINERS-add-maintainers-for-DRM-LOONGSON-driver/20230320-180408
git checkout 80b4115f44993f4ebf47b1cb9e8f02953575b977
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/accel/ drivers/gpu/drm/loongson/ drivers/iio/light/ drivers/media/pci/intel/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@xxxxxxxxx>
| Link: https://lore.kernel.org/oe-kbuild-all/202303281754.jWI20j2C-lkp@xxxxxxxxx/

All warnings (new ones prefixed by >>):

drivers/gpu/drm/loongson/lsdc_drv.c:232:11: warning: variable 'gpu' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
else if (descp->chip == CHIP_LS7A2000)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/loongson/lsdc_drv.c:235:7: note: uninitialized use occurs here
if (!gpu) {
^~~
drivers/gpu/drm/loongson/lsdc_drv.c:232:7: note: remove the 'if' if its condition is always true
else if (descp->chip == CHIP_LS7A2000)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/loongson/lsdc_drv.c:217:21: note: initialize the variable 'gpu' to silence this warning
struct pci_dev *gpu;
^
= NULL
1 warning generated.
--
In practice,  either  descp->chip == CHIP_LS7A2000 or descp->chip ==
CHIP_LS7A1000 will be happened at runtime.

the variable 'gpu' is guaranteed to be initialized when code run at
drivers/gpu/drm/loongson/lsdc_drv.c:235

This warnning is almost wrong here.
Clang's semantic analysis happens before optimizations, meaning it does
not perform interprocedural analysis, so it does not have enough
information at this point to tell that. Either just initialize gpu to
NULL and let the existing 'if (!gpu)' handle it or add a separate else
branch that warns about an unhandled chip value so that it is obvious
what needs to be done if someone forgets to update this statement when a
new chip is supported by this driver.

Right,  I overlook the point you mentioned previously.

And I just have a new idea,  using pci_get_domain_bus_and_slot function

to handle this.  the DC and the GPU have the same pci bus number and  domain number.

The slot number of the dc and gpu is also same(6), only the function number is different.


For ls7a1000,  what lspci -t -nnn -vvv show is:

-[0000:00]-+-00.0  Loongson Technology LLC Hyper Transport Bridge Controller [0014:7a00]

           ...

           +-06.0  Loongson Technology LLC Vivante GPU (Graphics Processing Unit) [0014:7a15]
           +-06.1  Loongson Technology LLC DC (Display Controller) [0014:7a06]

           ...


For ls7a2000, what lspci -t -nnn -vvv show is:

-[0000:00]-+-00.0  Loongson Technology LLC Hyper Transport Bridge Controller [0014:7a00]
           +-00.1  Loongson Technology LLC Hyper Transport Bridge Controller [0014:7a10]
           ...
           +-06.0  Loongson Technology LLC LoongGPU Device [0014:7a25]
           +-06.1  Loongson Technology LLC DC (Display Controller) Device [0014:7a36]
           +-06.2  Loongson Technology LLC Audio Device [0014:7a37]

            ...

So

pdev_gpu = pci_get_domain_bus_and_slot(pci_domain_nr(pdev_dc->bus),
                           pdev_dc->bus->number,
                           PCI_DEVFN(6, 0));


can help to handle all case, will this be ok?


drivers/gpu/drm/loongson/lsdc_pll.c:188:14: warning: variable 'diff' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
else if (clock_khz < computed)
^~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/loongson/lsdc_pll.c:191:9: note: uninitialized use occurs here
if (diff < min) {
^~~~
drivers/gpu/drm/loongson/lsdc_pll.c:188:10: note: remove the 'if' if its condition is always true
else if (clock_khz < computed)
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/loongson/lsdc_pll.c:177:22: note: initialize the variable 'diff' to silence this warning
unsigned int diff;
^
= 0
1 warning generated.
Here the robot is also wrong here in practice,

because either  if (clock_khz >= computed) or else if (clock_khz < computed)
will be happen.

'diff' variable is guaranteed to be initialized.
Make that clearer by turning 'else if (clock_khz < computed)' into just
'else' as the warning suggests? I do not see why the condition is
specified at all if it is just an 'else' in practice.

Yes, you are right.

The test robot do find something,  i will fix this at next version.

Cheers,
Nathan

vim +232 drivers/gpu/drm/loongson/lsdc_drv.c

212
213 static int lsdc_get_dedicated_vram(struct lsdc_device *ldev,
214 const struct lsdc_desc *descp)
215 {
216 struct drm_device *ddev = &ldev->base;
217 struct pci_dev *gpu;
218 resource_size_t base, size;
219
220 /*
221 * The GPU and display controller in LS7A1000/LS7A2000 are separated
222 * PCIE devices, they are two devices not one. The DC does not has a
223 * dedicate VRAM bar, because the BIOS engineer choose to assign the
224 * VRAM to the GPU device. Sadly, after years application, this form
225 * as a convention for loongson integrated graphics. Bar 2 of the GPU
226 * device contain the base address and size of the VRAM, both the GPU
227 * and the DC can access the on-board VRAM as long as the DMA address
228 * emitted fall in [base, base + size).
229 */
230 if (descp->chip == CHIP_LS7A1000)
231 gpu = pci_get_device(PCI_VENDOR_ID_LOONGSON, 0x7A15, NULL);
> 232 else if (descp->chip == CHIP_LS7A2000)
233 gpu = pci_get_device(PCI_VENDOR_ID_LOONGSON, 0x7A25, NULL);
234
235 if (!gpu) {
236 drm_warn(ddev, "No GPU device found\n");
237 return -ENODEV;
238 }
239
240 base = pci_resource_start(gpu, 2);
241 size = pci_resource_len(gpu, 2);
242
243 ldev->vram_base = base;
244 ldev->vram_size = size;
245
246 drm_info(ddev, "dedicated vram start: 0x%llx, size: %uMB\n",
247 (u64)base, (u32)(size >> 20));
248
249 return 0;
250 }
251