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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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	




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux