Re: [PATCH 2/3] drm/mst: Refactor the flow for payload allocation/removement
Hi Wayne, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on drm-intel/for-linux-next-fixes drm/drm-next linus/master v6.5-rc4 next-20230804] [cannot apply to drm-intel/for-linux-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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Wayne-Lin/drm-mst-delete-unnecessary-case-in-drm_dp_add_payload_part2/20230804-142405 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20230804062029.5686-3-Wayne.Lin%40amd.com patch subject: [PATCH 2/3] drm/mst: Refactor the flow for payload allocation/removement config: s390-randconfig-r044-20230731 (https://download.01.org/0day-ci/archive/20230804/202308042253.lm5xmnna-...@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230804/202308042253.lm5xmnna-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202308042253.lm5xmnna-...@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/nouveau/dispnv50/disp.c:24: In file included from drivers/gpu/drm/nouveau/dispnv50/disp.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/mem.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/mmu.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/object.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8: In file included from include/linux/pci.h:39: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' 102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) | ^ In file included from drivers/gpu/drm/nouveau/dispnv50/disp.c:24: In file included from drivers/gpu/drm/nouveau/dispnv50/disp.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/mem.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/mmu.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/object.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8: In file included from include/linux/pci.h:39: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) | ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' 115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) | ^ In file included from drivers/gpu/drm/nouveau/dispnv50/disp.c:24: In file included from drivers/gpu/drm/nouveau/dispnv50/disp.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/mem.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/mmu.h:3: In file included from drivers/gpu/drm/nouveau/include/nvif/object.h:4: In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8: In file included from include/linux/pci.h:39: In file included from include/linux/io.h:13: In file included from arc
Re: [PATCH 2/3] drm/mst: Refactor the flow for payload allocation/removement
Hi Wayne,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-intel/for-linux-next-fixes drm/drm-next
linus/master v6.5-rc4 next-20230804]
[cannot apply to drm-intel/for-linux-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#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Wayne-Lin/drm-mst-delete-unnecessary-case-in-drm_dp_add_payload_part2/20230804-142405
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20230804062029.5686-3-Wayne.Lin%40amd.com
patch subject: [PATCH 2/3] drm/mst: Refactor the flow for payload
allocation/removement
config: microblaze-randconfig-r021-20230731
(https://download.01.org/0day-ci/archive/20230804/202308041635.wkgwwx5r-...@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.3.0
reproduce:
(https://download.01.org/0day-ci/archive/20230804/202308041635.wkgwwx5r-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202308041635.wkgwwx5r-...@intel.com/
All warnings (new ones prefixed by >>):
drivers/gpu/drm/nouveau/dispnv50/disp.c: In function 'nv50_msto_prepare':
>> drivers/gpu/drm/nouveau/dispnv50/disp.c:919:53: warning: variable
>> 'old_payload' set but not used [-Wunused-but-set-variable]
919 | struct drm_dp_mst_atomic_payload *payload, *old_payload;
| ^~~
vim +/old_payload +919 drivers/gpu/drm/nouveau/dispnv50/disp.c
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
908
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
909 static void
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
910 nv50_msto_prepare(struct drm_atomic_state *state,
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
911 struct drm_dp_mst_topology_state *mst_state,
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
912 struct drm_dp_mst_topology_mgr *mgr,
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
913 struct nv50_msto *msto)
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
914 {
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
915 struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
916 struct nv50_mstc *mstc = msto->mstc;
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
917 struct nv50_mstm *mstm = mstc->mstm;
8fb3e25c3dd1a2 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2023-06-13
918 struct drm_dp_mst_topology_state *old_mst_state;
8fb3e25c3dd1a2 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2023-06-13
@919 struct drm_dp_mst_atomic_payload *payload, *old_payload;
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
920
f479c0ba4a170a drivers/gpu/drm/nouveau/nv50_display.c Ben Skeggs 2016-11-04
921 NV_ATOMIC(drm, "%s: msto prepare\n", msto->encoder.name);
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
922
8fb3e25c3dd1a2 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2023-06-13
923 old_mst_state = drm_atomic_get_old_mst_topology_state(state, mgr);
8fb3e25c3dd1a2 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2023-06-13
924
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
925 payload = drm_atomic_get_mst_payload_state(mst_state, mstc->port);
8fb3e25c3dd1a2 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2023-06-13
926 old_payload = drm_atomic_get_mst_payload_state(old_mst_state,
mstc->port);
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
927
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
928 // TODO: Figure out if we want to do a better job of handling VCPI
allocation failures here?
4d07b0bc403403 drivers/gpu/drm/nouveau/dispnv50/disp.c Lyude Paul 2022-08-17
929 if (msto->disabled) {
c9e8c7f37133c0 drivers/gpu/drm/nouveau/dispnv50/disp.c Wayne Lin 2023-08-04
930 drm_dp_remove_payload_part1(mgr, mst_state, payload);
8c7d980da9ba3e drivers/gpu/drm/nouveau/dispnv50/disp.c Ben Skeggs 2022-06-01
931
8c7d980da9ba3e drivers/gpu/drm/nouveau/dispnv50/disp.c Ben Skeggs 2022-06-01
932
[PATCH 2/3] drm/mst: Refactor the flow for payload allocation/removement
[Why]
Today, the allocation/deallocation steps and status is a bit unclear.
For instance, payload->vc_start_slot = -1 stands for "the failure of
updating DPCD payload ID table" and can also represent as "payload is not
allocated yet". These two cases should be handled differently and hence
better to distinguish them for better understanding.
[How]
Define enumeration - ALLOCATION_LOCAL, ALLOCATION_DFP and ALLOCATION_REMOTE
to distinguish different allocation status. Adjust the code to handle
different status accordingly for better understanding the sequence of
payload allocation and payload removement.
For payload creation, the procedure should look like this:
DRM part 1:
* step 1 - update sw mst mgr variables to add a new payload
* step 2 - add payload at immediate DFP DPCD payload table
Driver:
* Add new payload in HW and sync up with DFP by sending ACT
DRM Part 2:
* Send ALLOCATE_PAYLOAD sideband message to allocate bandwidth along the
virtual channel.
And as for payload removement, the procedure should look like this:
DRM part 1:
* step 1 - Send ALLOCATE_PAYLOAD sideband message to release bandwidth
along the virtual channel
* step 2 - Clear payload allocation at immediate DFP DPCD payload table
Driver:
* Remove the payload in HW and sync up with DFP by sending ACT
DRM part 2:
* update sw mst mgr variables to remove the payload
Note that it's fine to fail when communicate with the branch device
connected at immediate downstrean-facing port, but updating variables of
SW mst mgr and HW configuration should be conducted anyway. That's because
it's under commit_tail and we need to complete the HW programming.
Signed-off-by: Wayne Lin
---
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 20 ++-
drivers/gpu/drm/display/drm_dp_mst_topology.c | 159 +++---
drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 15 +-
include/drm/display/drm_dp_mst_helper.h | 23 ++-
5 files changed, 152 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index d9a482908380..9ad509279b0a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -219,7 +219,7 @@ static void dm_helpers_construct_old_payload(
/* Set correct time_slots/PBN of old payload.
* other fields (delete & dsc_enabled) in
* struct drm_dp_mst_atomic_payload are don't care fields
-* while calling drm_dp_remove_payload()
+* while calling drm_dp_remove_payload_part2()
*/
for (i = 0; i < current_link_table.stream_count; i++) {
dc_alloc =
@@ -262,13 +262,12 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
mst_mgr = >mst_root->mst_mgr;
mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
-
- /* It's OK for this to fail */
new_payload = drm_atomic_get_mst_payload_state(mst_state,
aconnector->mst_output_port);
if (enable) {
target_payload = new_payload;
+ /* It's OK for this to fail */
drm_dp_add_payload_part1(mst_mgr, mst_state, new_payload);
} else {
/* construct old payload by VCPI*/
@@ -276,7 +275,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
new_payload, _payload);
target_payload = _payload;
- drm_dp_remove_payload(mst_mgr, mst_state, _payload,
new_payload);
+ drm_dp_remove_payload_part1(mst_mgr, mst_state, new_payload);
}
/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or
@@ -342,7 +341,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
struct amdgpu_dm_connector *aconnector;
struct drm_dp_mst_topology_state *mst_state;
struct drm_dp_mst_topology_mgr *mst_mgr;
- struct drm_dp_mst_atomic_payload *payload;
+ struct drm_dp_mst_atomic_payload *new_payload, *old_payload;
enum mst_progress_status set_flag = MST_ALLOCATE_NEW_PAYLOAD;
enum mst_progress_status clr_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
int ret = 0;
@@ -355,15 +354,20 @@ bool dm_helpers_dp_mst_send_payload_allocation(
mst_mgr = >mst_root->mst_mgr;
mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
- payload = drm_atomic_get_mst_payload_state(mst_state,
aconnector->mst_output_port);
+ new_payload = drm_atomic_get_mst_payload_state(mst_state,
aconnector->mst_output_port);
if (!enable) {
set_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
}
- if (enable)
- ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload);
+ if (enable) {
+ ret =
