| Message ID | 20201204151138.1739736-6-maxime@cerno.tech |
|---|---|
| State | Accepted |
| Commit | 03b03efebeed3a928e168505d32939f35055264a |
| Headers | show |
| Series | vc4: Convert to drm_atomic_helper_commit | expand |
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
On Fri, Dec 04, 2020 at 04:11:36PM +0100, Maxime Ripard wrote: > @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, > struct vc4_hvs_state *hvs_new_state; > struct drm_crtc_state *old_crtc_state, *new_crtc_state; > struct drm_crtc *crtc; > + unsigned int unassigned_channels; This should be initialized to 0, I'll fix it up while applying if there's no other comment. Maxime
Hi Am 04.12.20 um 16:11 schrieb Maxime Ripard: > The HVS state now has both unassigned_channels that reflects the > channels that are not used in the associated state, and the in_use > boolean for each channel that says whether or not a particular channel > is in use. > > Both express pretty much the same thing, and we need the in_use variable > to properly track the commits, so let's get rid of unassigned_channels. > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index fdd698df5fbe..fa40c44eb770 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -39,7 +39,6 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv) > > struct vc4_hvs_state { > struct drm_private_state base; > - unsigned int unassigned_channels; > > struct { > unsigned in_use: 1; > @@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj) > > __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); > > - state->unassigned_channels = old_state->unassigned_channels; > > for (i = 0; i < HVS_NUM_CHANNELS; i++) { > state->fifo_state[i].in_use = old_state->fifo_state[i].in_use; > @@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) > if (!state) > return -ENOMEM; > > - state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0); > drm_atomic_private_obj_init(&vc4->base, &vc4->hvs_channels, > &state->base, > &vc4_hvs_state_funcs); > @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, > struct vc4_hvs_state *hvs_new_state; > struct drm_crtc_state *old_crtc_state, *new_crtc_state; > struct drm_crtc *crtc; > + unsigned int unassigned_channels; > unsigned int i; > > hvs_new_state = vc4_hvs_get_global_state(state); > if (!hvs_new_state) > return -EINVAL; > > + for (i = 0; i < HVS_NUM_CHANNELS; i++) > + if (!hvs_new_state->fifo_state[i].in_use) > + unassigned_channels |= BIT(i); > + More of a nit: I'd turn this block into a helper of struct vc4_hvs_state. That would also remove the need to initialize unassigned_channels to 0 here. For the loop's condition, it might be less error prone to use ARRAY_SIZE(hvs_new_state->fifo_state) instead of HVS_NUM_CHANNEL. In any case Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Best regards Thomas > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > struct vc4_crtc_state *old_vc4_crtc_state = > to_vc4_crtc_state(old_crtc_state); > @@ -918,8 +920,6 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, > /* If we're disabling our CRTC, we put back our channel */ > if (!new_crtc_state->enable) { > channel = old_vc4_crtc_state->assigned_channel; > - > - hvs_new_state->unassigned_channels |= BIT(channel); > hvs_new_state->fifo_state[channel].in_use = false; > new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > continue; > @@ -949,13 +949,13 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, > * the future, we will need to have something smarter, > * but it works so far. > */ > - matching_channels = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels; > + matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels; > if (!matching_channels) > return -EINVAL; > > channel = ffs(matching_channels) - 1; > new_vc4_crtc_state->assigned_channel = channel; > - hvs_new_state->unassigned_channels &= ~BIT(channel); > + unassigned_channels &= ~BIT(channel); > hvs_new_state->fifo_state[channel].in_use = true; > } > >
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index fdd698df5fbe..fa40c44eb770 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -39,7 +39,6 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv) struct vc4_hvs_state { struct drm_private_state base; - unsigned int unassigned_channels; struct { unsigned in_use: 1; @@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj) __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); - state->unassigned_channels = old_state->unassigned_channels; for (i = 0; i < HVS_NUM_CHANNELS; i++) { state->fifo_state[i].in_use = old_state->fifo_state[i].in_use; @@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) if (!state) return -ENOMEM; - state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0); drm_atomic_private_obj_init(&vc4->base, &vc4->hvs_channels, &state->base, &vc4_hvs_state_funcs); @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, struct vc4_hvs_state *hvs_new_state; struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; + unsigned int unassigned_channels; unsigned int i; hvs_new_state = vc4_hvs_get_global_state(state); if (!hvs_new_state) return -EINVAL; + for (i = 0; i < HVS_NUM_CHANNELS; i++) + if (!hvs_new_state->fifo_state[i].in_use) + unassigned_channels |= BIT(i); + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { struct vc4_crtc_state *old_vc4_crtc_state = to_vc4_crtc_state(old_crtc_state); @@ -918,8 +920,6 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, /* If we're disabling our CRTC, we put back our channel */ if (!new_crtc_state->enable) { channel = old_vc4_crtc_state->assigned_channel; - - hvs_new_state->unassigned_channels |= BIT(channel); hvs_new_state->fifo_state[channel].in_use = false; new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; continue; @@ -949,13 +949,13 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, * the future, we will need to have something smarter, * but it works so far. */ - matching_channels = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels; + matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels; if (!matching_channels) return -EINVAL; channel = ffs(matching_channels) - 1; new_vc4_crtc_state->assigned_channel = channel; - hvs_new_state->unassigned_channels &= ~BIT(channel); + unassigned_channels &= ~BIT(channel); hvs_new_state->fifo_state[channel].in_use = true; }
The HVS state now has both unassigned_channels that reflects the channels that are not used in the associated state, and the in_use boolean for each channel that says whether or not a particular channel is in use. Both express pretty much the same thing, and we need the in_use variable to properly track the commits, so let's get rid of unassigned_channels. Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)