Re: [PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

[kernel test robot <lkp@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 14220 bytes --]

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on linus/master v5.10-rc6 next-20201204]
[cannot apply to drm-intel/for-linux-next anholt/for-next]
[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]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/vc4-Convert-to-drm_atomic_helper_commit/20201204-231528
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: riscv-randconfig-r014-20201204 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 32c501dd88b62787d3a5ffda7aabcf4650dbe3cd)
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
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/1ac52fbc9e5e40633ecb194184b4ba69937e8b55
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maxime-Ripard/vc4-Convert-to-drm_atomic_helper_commit/20201204-231528
        git checkout 1ac52fbc9e5e40633ecb194184b4ba69937e8b55
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:556:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inb(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:55:76: note: expanded from macro 'inb'
   #define inb(c)          ({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:87:48: note: expanded from macro 'readb_cpu'
   #define readb_cpu(c)            ({ u8  __r = __raw_readb(c); __r; })
                                                            ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inw(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw'
   #define inw(c)          ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
   #define readw_cpu(c)            ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inl(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl'
   #define inl(c)          ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
   #define readl_cpu(c)            ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outb(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb'
   #define outb(v,c)       ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
   #define writeb_cpu(v, c)        ((void)__raw_writeb((v), (c)))
                                                             ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outw(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw'
   #define outw(v,c)       ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
   #define writew_cpu(v, c)        ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
                                                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outl(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl'
   #define outl(v,c)       ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
   #define writel_cpu(v, c)        ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
                                                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/gpu/drm/vc4/vc4_kms.c:902:4: warning: variable 'unassigned_channels' is uninitialized when used here [-Wuninitialized]
                           unassigned_channels |= BIT(i);
                           ^~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vc4/vc4_kms.c:893:34: note: initialize the variable 'unassigned_channels' to silence this warning
           unsigned int unassigned_channels;
                                           ^
                                            = 0
   8 warnings generated.

vim +/unassigned_channels +902 drivers/gpu/drm/vc4/vc4_kms.c

   856	
   857	/*
   858	 * The BCM2711 HVS has up to 7 outputs connected to the pixelvalves and
   859	 * the TXP (and therefore all the CRTCs found on that platform).
   860	 *
   861	 * The naive (and our initial) implementation would just iterate over
   862	 * all the active CRTCs, try to find a suitable FIFO, and then remove it
   863	 * from the pool of available FIFOs. However, there are a few corner
   864	 * cases that need to be considered:
   865	 *
   866	 * - When running in a dual-display setup (so with two CRTCs involved),
   867	 *   we can update the state of a single CRTC (for example by changing
   868	 *   its mode using xrandr under X11) without affecting the other. In
   869	 *   this case, the other CRTC wouldn't be in the state at all, so we
   870	 *   need to consider all the running CRTCs in the DRM device to assign
   871	 *   a FIFO, not just the one in the state.
   872	 *
   873	 * - To fix the above, we can't use drm_atomic_get_crtc_state on all
   874	 *   enabled CRTCs to pull their CRTC state into the global state, since
   875	 *   a page flip would start considering their vblank to complete. Since
   876	 *   we don't have a guarantee that they are actually active, that
   877	 *   vblank might never happen, and shouldn't even be considered if we
   878	 *   want to do a page flip on a single CRTC. That can be tested by
   879	 *   doing a modetest -v first on HDMI1 and then on HDMI0.
   880	 *
   881	 * - Since we need the pixelvalve to be disabled and enabled back when
   882	 *   the FIFO is changed, we should keep the FIFO assigned for as long
   883	 *   as the CRTC is enabled, only considering it free again once that
   884	 *   CRTC has been disabled. This can be tested by booting X11 on a
   885	 *   single display, and changing the resolution down and then back up.
   886	 */
   887	static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
   888					      struct drm_atomic_state *state)
   889	{
   890		struct vc4_hvs_state *hvs_new_state;
   891		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
   892		struct drm_crtc *crtc;
   893		unsigned int unassigned_channels;
   894		unsigned int i;
   895	
   896		hvs_new_state = vc4_hvs_get_global_state(state);
   897		if (!hvs_new_state)
   898			return -EINVAL;
   899	
   900		for (i = 0; i < HVS_NUM_CHANNELS; i++)
   901			if (!hvs_new_state->fifo_state[i].in_use)
 > 902				unassigned_channels |= BIT(i);
   903	
   904		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
   905			struct vc4_crtc_state *old_vc4_crtc_state =
   906				to_vc4_crtc_state(old_crtc_state);
   907			struct vc4_crtc_state *new_vc4_crtc_state =
   908				to_vc4_crtc_state(new_crtc_state);
   909			struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
   910			unsigned int matching_channels;
   911			unsigned int channel;
   912	
   913			/* Nothing to do here, let's skip it */
   914			if (old_crtc_state->enable == new_crtc_state->enable)
   915				continue;
   916	
   917			/* Muxing will need to be modified, mark it as such */
   918			new_vc4_crtc_state->update_muxing = true;
   919	
   920			/* If we're disabling our CRTC, we put back our channel */
   921			if (!new_crtc_state->enable) {
   922				channel = old_vc4_crtc_state->assigned_channel;
   923				hvs_new_state->fifo_state[channel].in_use = false;
   924				new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
   925				continue;
   926			}
   927	
   928			/*
   929			 * The problem we have to solve here is that we have
   930			 * up to 7 encoders, connected to up to 6 CRTCs.
   931			 *
   932			 * Those CRTCs, depending on the instance, can be
   933			 * routed to 1, 2 or 3 HVS FIFOs, and we need to set
   934			 * the change the muxing between FIFOs and outputs in
   935			 * the HVS accordingly.
   936			 *
   937			 * It would be pretty hard to come up with an
   938			 * algorithm that would generically solve
   939			 * this. However, the current routing trees we support
   940			 * allow us to simplify a bit the problem.
   941			 *
   942			 * Indeed, with the current supported layouts, if we
   943			 * try to assign in the ascending crtc index order the
   944			 * FIFOs, we can't fall into the situation where an
   945			 * earlier CRTC that had multiple routes is assigned
   946			 * one that was the only option for a later CRTC.
   947			 *
   948			 * If the layout changes and doesn't give us that in
   949			 * the future, we will need to have something smarter,
   950			 * but it works so far.
   951			 */
   952			matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
   953			if (!matching_channels)
   954				return -EINVAL;
   955	
   956			channel = ffs(matching_channels) - 1;
   957			new_vc4_crtc_state->assigned_channel = channel;
   958			unassigned_channels &= ~BIT(channel);
   959			hvs_new_state->fifo_state[channel].in_use = true;
   960		}
   961	
   962		return 0;
   963	}
   964	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29635 bytes --]