dri-devel April 2022

dri-devel@lists.freedesktop.org

351 participants
951 discussions

[PATCH v10, 00/15] media: mtk-vcodec: support for M8192 decoder
by Yunfei Dong 06 May '22

06 May '22

This series adds support for mt8192 h264/vp8/vp9 decoder drivers. Firstly, refactor power/clock/interrupt interfaces for mt8192 is lat and core architecture. Secondly, add new functions to get frame buffer size and resolution according to decoder capability from scp side. Then add callback function to get/put capture buffer in order to enable lat and core decoder in parallel, need to adjust GStreamer at the same time. Then add to support MT21C compressed mode and fix v4l2-compliance fail. Next, extract H264 request api driver to let mt8183 and mt8192 use the same code, and adds mt8192 frame based h264 driver for stateless decoder. Lastly, add vp8 and vp9 stateless decoder drivers. Patches 1 refactor power/clock/interrupt interface. Patches 2~4 get frame buffer size and resolution according to decoder capability. Patches 5 set capture queue bytesused. Patches 6 adjust GStreamer. Patch 7~11 add to support MT21C compressed mode and fix v4l2-compliance fail. patch 12 record capture queue format type. Patch 13~14 extract h264 driver and add mt8192 frame based driver for h264 decoder. Patch 15~16 add vp8 and vp9 stateless decoder drivers. Patch 17 prevent kernel crash when rmmod mtk-vcodec-dec.ko --- changes compared with v9: - fix kernel robot build fail for patch 16 changes compared with v8: - fix vp9 build error - fix kernel-doc fail changes compared with v7: - adjust GStreamer, separate src buffer done with v4l2_ctrl_request_complete for patch 6. - remove v4l2_m2m_set_dst_buffered. - add new patch to set each plane bytesused in buf prepare for patch 5. - using upstream interface to update vp9 prob tables for patch 16. - fix maintainer comments. - test the driver with chrome VD and GStreamer(H264/VP9/VP8/AV1). changes compared with v6: - rebase to the latest media stage and fix conficts - fix memcpy to memcpy_fromio or memcpy_toio - fix h264 crash when test field bitstream changes compared with v5: - fix vp9 comments for patch 15 - fix vp8 comments for patch 14. - fix comments for patch 12. - fix build errors. changes compared with v4: - fix checkpatch.pl fail. - fix kernel-doc fail. - rebase to the latest media codec driver. changes compared with v3: - remove enum mtk_chip for patch 2. - add vp8 stateless decoder drivers for patch 14. - add vp9 stateless decoder drivers for patch 15. changes compared with v2: - add new patch 11 to record capture queue format type. - separate patch 4 according to tzung-bi's suggestion. - re-write commit message for patch 5 according to tzung-bi's suggestion. changes compared with v1: - rewrite commit message for patch 12. - rewrite cover-letter message. --- Yunfei Dong (17): media: mediatek: vcodec: Add vdec enable/disable hardware helpers media: mediatek: vcodec: Using firmware type to separate different firmware architecture media: mediatek: vcodec: get capture queue buffer size from scp media: mediatek: vcodec: Read max resolution from dec_capability media: mediatek: vcodec: set each plane bytesused in buf prepare media: mediatek: vcodec: Refactor get and put capture buffer flow media: mediatek: vcodec: Refactor supported vdec formats and framesizes media: mediatek: vcodec: Getting supported decoder format types media: mediatek: vcodec: Add format to support MT21C media: mediatek: vcodec: disable vp8 4K capability media: mediatek: vcodec: Fix v4l2-compliance fail media: mediatek: vcodec: record capture queue format type media: mediatek: vcodec: Extract H264 common code media: mediatek: vcodec: support stateless H.264 decoding for mt8192 media: mediatek: vcodec: support stateless VP8 decoding media: mediatek: vcodec: support stateless VP9 decoding media: mediatek: vcodec: prevent kernel crash when rmmod mtk-vcodec-dec.ko .../media/platform/mediatek/vcodec/Makefile | 4 + .../platform/mediatek/vcodec/mtk_vcodec_dec.c | 62 +- .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 8 +- .../mediatek/vcodec/mtk_vcodec_dec_pm.c | 166 +- .../mediatek/vcodec/mtk_vcodec_dec_pm.h | 6 +- .../mediatek/vcodec/mtk_vcodec_dec_stateful.c | 19 +- .../vcodec/mtk_vcodec_dec_stateless.c | 257 ++- .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 41 +- .../mediatek/vcodec/mtk_vcodec_enc_drv.c | 5 - .../platform/mediatek/vcodec/mtk_vcodec_fw.c | 6 + .../platform/mediatek/vcodec/mtk_vcodec_fw.h | 1 + .../vcodec/vdec/vdec_h264_req_common.c | 310 +++ .../vcodec/vdec/vdec_h264_req_common.h | 274 +++ .../mediatek/vcodec/vdec/vdec_h264_req_if.c | 438 +--- .../vcodec/vdec/vdec_h264_req_multi_if.c | 626 +++++ .../mediatek/vcodec/vdec/vdec_vp8_req_if.c | 437 ++++ .../vcodec/vdec/vdec_vp9_req_lat_if.c | 2031 +++++++++++++++++ .../platform/mediatek/vcodec/vdec_drv_if.c | 37 +- .../platform/mediatek/vcodec/vdec_drv_if.h | 3 + .../platform/mediatek/vcodec/vdec_ipi_msg.h | 36 + .../platform/mediatek/vcodec/vdec_msg_queue.c | 2 + .../platform/mediatek/vcodec/vdec_msg_queue.h | 2 + .../platform/mediatek/vcodec/vdec_vpu_if.c | 53 +- .../platform/mediatek/vcodec/vdec_vpu_if.h | 15 + .../platform/mediatek/vcodec/venc_vpu_if.c | 2 +- include/linux/remoteproc/mtk_scp.h | 2 + 26 files changed, 4242 insertions(+), 601 deletions(-) create mode 100644 drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.c create mode 100644 drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_common.h create mode 100644 drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c create mode 100644 drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_req_if.c create mode 100644 drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c -- 2.18.0

4 21

[PATCH resend v8 0/5] phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support
by Liu Ying 06 May '22

06 May '22

Hi, This is the v8 series to add i.MX8qxp LVDS PHY mode support for the Mixel PHY in the Freescale i.MX8qxp SoC. The Mixel PHY is MIPI DPHY + LVDS PHY combo, which can works in either MIPI DPHY mode or LVDS PHY mode. The PHY mode is controlled by i.MX8qxp SCU firmware. The PHY driver would call a SCU function to configure the mode. The PHY driver is already supporting the Mixel MIPI DPHY in i.MX8mq SoC, where it appears to be a single MIPI DPHY. Patch 1/5 sets PHY mode in the Northwest Logic MIPI DSI host controller bridge driver, since i.MX8qxp SoC embeds this controller IP to support MIPI DSI displays together with the Mixel PHY. Patch 2/5 allows LVDS PHYs to be configured through the generic PHY functions and through a custom structure added to the generic PHY configuration union. Patch 3/5 converts mixel,mipi-dsi-phy plain text dt binding to json-schema. Patch 4/5 adds dt binding support for the Mixel combo PHY in i.MX8qxp SoC. Patch 5/5 adds the i.MX8qxp LVDS PHY mode support in the Mixel PHY driver. Welcome comments, thanks. v7->v8: * Trivial kernel doc style fix for patch 2/5 - add '*'. * Resend with reviewer mail addresses updated for patch 1/5. v6->v7: * Update the year of copyright for patch 2/5. * Better variable explanation for bits_per_lane_and_dclk_cycle in patch 2/5. * Use marco instead of magic number for CCM and CA values for patch 5/5. * Suppress 'checkpatch --strict' warnings for patch 5/5. v5->v6: * Rebase the series upon v5.17-rc1. * Set PHY mode in ->mode_set() instead of ->pre_enable() in the nwl-dsi bridge driver in patch 1/5 due to the rebase. * Drop Guido's R-b tag on patch 1/5 due to the rebase. v4->v5: * Align kernel-doc style of include/linux/phy/phy-lvds.h to include/linux/phy/phy.h for patch 2/5. (Vinod) * Trivial tweaks on patch 2/5. * Drop Robert's R-b tag on patch 2/5. v3->v4: * Add all R-b tags received from v3 on relevant patches and respin. (Robert) v2->v3: * Improve readability of mixel_dphy_set_mode() in the Mixel PHY driver. (Guido) * Improve the 'clock-names' property in the PHY dt binding. v1->v2: * Convert mixel,mipi-dsi-phy plain text dt binding to json-schema. (Guido) * Print invalid PHY mode in dmesg from the Mixel PHY driver. (Guido) * Add Guido's R-b tag on the patch for the nwl-dsi drm bridge driver. Liu Ying (5): drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_mode_set() phy: Add LVDS configuration options dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for i.MX8qxp phy: freescale: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support .../bindings/phy/mixel,mipi-dsi-phy.txt | 29 -- .../bindings/phy/mixel,mipi-dsi-phy.yaml | 107 +++++++ drivers/gpu/drm/bridge/nwl-dsi.c | 6 + .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 276 +++++++++++++++++- include/linux/phy/phy-lvds.h | 32 ++ include/linux/phy/phy.h | 4 + 6 files changed, 414 insertions(+), 40 deletions(-) delete mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml create mode 100644 include/linux/phy/phy-lvds.h -- 2.25.1

4 11

Re: [PATCH] WIP: drm/dp_mst: Add support for dumping topology ref histories from debugfs
by Lyude Paul 06 May '22

06 May '22

(Adding this back to the dri-devel mailing list since I didn't notice it got dropped from there) Hm, some comments on this issue down below. Sorry for the delayed response, I was going to try this right after I finished the MST legacy removal but that's ending up taking longer than I hoped. On Tue, 2022-03-08 at 01:46 +0000, Lin, Wayne wrote: > [AMD Official Use Only] > > Oops... > I didn't notice that I replied to wrong mail previously : P > Sorry for the confusion and reply to the correct thread. > > Good day! > I probably know why you can't reproduce the issue there. Please refer to the > attached > patch file which makes me easier to explain this. > > In dm_dp_mst_get_modes(), we will create a new dc_sink by calling > dc_link_add_remote_sink() and add that dc_sink into the array dc_link- > >remote_sinks[]. > However, if we find that aconnector->dc_sink is not null, then we won't > create a new > dc_sink for that aconnector and won't add the sink to the array dc_link- > >remote_sinks[]. > > When the issue that I mentioned before occurs, we won't be able to release > the dc_sink > for the aconnector when we unplug the sst monitor. Hence, we can't create > new dc_sink > for the latter on connected new stream sink of that port. Which leads to two > issues > 1. Unplug monitor and plug in another monitor "to the same port" > => Since we use the same dc_sink to reflect the capability of the new > connected stream > sink, we might send out inappropriate stream to the new connected sink. > 2. Because we can't release dc_sink in the array dc_link->remote_sinks[], > when we > plug/unplug sst monitor to more than 4 different mst port, we will break the > criteria > "dc_link->sink_count >= MAX_SINKS_PER_LINK" in > link_add_remote_sink_helper(). Ok, a lot of this seems to be happening in amdgpu which certainly explains why I had so much trouble following along with this originally (also, having learned a bit more about how DC works definitely has helped a bit). I already see a bunch of issues though with how this code is structured that would definitely break things, though note I haven't sat down and tried this on a real machine yet - will try tomorrow. So - it seems like dc_sink == a bunch of hotplugging state for a dm connector, which actually tells me for one that this is definitely the wrong spot for this code. get_modes() really should just handle filling out DRM modes and pretty much nothing else, because callers from DRM aren't going to expect side-effects like this when get_modes() is called - which could potentially lead to all sorts of weirdness down the line if the DRM call paths that use this ever change. i915 and nouveau have good examples of what these functions should typically look like, and amdgpu actually seems to mostly follow this advice for it's other get_modes() callback. Note there's also another problem here: the assumption "no EDID == disconnected" isn't correct. It's totally possible to run into EDID-less displays if the display is some ancient pre-historic relic, or if the ROM (or EEPROM? can't remember what type of chip computer displays tend to use…) chip in the monitor containing the EDID has died. Note that the second situation is suprisingly more common then one might think! I ran into a 140Hz 4k ASUS display where this happened, and I know another friend of mine who had the ROM in their display die randomly as well. So with this being said: I, think the solution here is actually just to add/remove dc_sink from dm_dp_mst_detect() based on the CSN, and drop the calls from get_modes()? detect() should always be called before the connector has been removed, and we could use the return status from drm_dp_mst_detect_port() in order to tell whether or not we should add/remove the dc_sink. If you don't get to it first tomorrow, I'll see if I can reproduce this locally and try writing up a patch. Also a final note: I don't think it's a good idea in general for there to be a fixed size array of dc_sinks like this. Those probably should just be stored somehow on a per-connector basis. I say this because in addition to the fact MST connectors aren't destroyed immediately (which could lead to a situation where we have more then 6 connected MST ports briefly before their destructors run, e.g. during suspend/resume reprobing). Also, one question below: > > Anyway, these are my steps to reproduce the issue. > I use a startech mst 1to4 mst hub, a sst monitor and a mst monitor. > > 1. boot up with sst monitor connected to the hub and plug the hub to the > system. > 2. unplug/plug sst monitor to all left three different DP output ports. > Which is used to > Pollute all four aconnectors to have them keep the stale dc_sinks. > 3. plug in the mst monitor to arbitrary ports. Since the new connected mst > monitor will > have us add new mst port to the topology and attach a new aconnector for > that mst port, > we'll try to create a new dc_sink to that new aconnector and break the > sink_count > limitation. As you can see in the log the error message : "*ERROR* Unable to > add a > remote sink" > > (Note that different hub might influence the result. Some hubs will pull > down the HPD > when there is no monitor connected to its downstream ports which will run > into different > code flow) by "pull down" I assume you mean doing a long HPD instead of short (or vice versa?). > > I also attach the dmesg log under different debug level (0x104 & 0x116) for > your reference. > > Please let me know if you can reproduce then : ) > > Sorry about that Lyude and hope this time I send to the right thread.. > Really appreciate for your help!!! > > Regards, > Wayne > > -----Original Message----- > > From: Lyude Paul <lyude(a)redhat.com> > > Sent: Wednesday, March 2, 2022 9:10 AM > > To: Lin, Wayne <Wayne.Lin(a)amd.com> > > Subject: Re: [PATCH] WIP: drm/dp_mst: Add support for dumping topology ref > > histories from debugfs > > > > Ran into some issues with my system yesterday (hooray being the first to > > find > > a rather annoying zsh bug that broke my scripts!), but I've finally got > > things > > setup. I'm not able to reproduce this issue though (I've tried hotplugging > > the > > way you mentioned here, but nothing seems to leak), which makes me think > > there's something special in your setup we might not have noticed. > > > > Do you think you could get some drm logs with drm.debug=0x116 > > log_buf_len=50M > > from when this happens so I can get a better idea of the drm events > > leading up > > to this? Also, if you've been testing this in a GUI could you confirm > > whether > > this happens when you're just in a plain old VT? > > > > On Mon, 2022-02-28 at 16:43 -0500, Lyude Paul wrote: > > > Alright - starting to look into this now and setting up a system to try > > > reproducing this, will let you know as soon as I've got results. Sorry > > > again > > > about all of the delays > > > > > > On Fri, 2022-02-25 at 15:47 -0500, Lyude Paul wrote: > > > > I didn't get a chance yet, but I've gotten through most of my backlog > > > > now. > > > > Sorry about the delay - I've been catching up since I had to take two > > > > weeks > > > > off work very unexpectedly. I'll put aside time to look at this first > > > > thing > > > > monday. > > > > > > > > On Fri, 2022-02-25 at 01:33 +0000, Lin, Wayne wrote: > > > > > [AMD Official Use Only] > > > > > > > > > > Hi Lyude, > > > > > > > > > > Good day! > > > > > Sorry to bother you, but would like to know if you can reproduce the > > > > > issue? > > > > > > > > > > Thanks! > > > > > > > > > > Regards, > > > > > Wayne > > > > > > > > > > > -----Original Message----- > > > > > > From: Lyude Paul <lyude(a)redhat.com> > > > > > > Sent: Thursday, February 3, 2022 4:12 AM > > > > > > To: Lin, Wayne <Wayne.Lin(a)amd.com> > > > > > > Cc: dri-devel(a)lists.freedesktop.org > > > > > > Subject: Re: [PATCH] WIP: drm/dp_mst: Add support for dumping > > > > > > topology > > > > > > ref > > > > > > histories from debugfs > > > > > > > > > > > > JFYI I should be able to respond to this soon, I think I'm > > > > > > following > > > > > > your > > > > > > issue a bit more (haven't gotten to go through the patches too so > > > > > > I'm > > > > > > probably > > > > > > going to see if I can reproduce it locally as well. Sorry for the > > > > > > delay > > > > > > - > > > > > > dealing with some X.org stuff and work issues at the moment > > > > > > > > > > > > On Tue, 2022-01-25 at 08:32 +0000, Lin, Wayne wrote: > > > > > > > [Public] > > > > > > > > > > > > > > Hey Lyude, > > > > > > > > > > > > > > Sorry, just got chance to get back to this and BIG THANKS to > > > > > > > elaborating > > > > > > > in > > > > > > > such details! > > > > > > > Following are some of my comments : ) > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Lyude Paul <lyude(a)redhat.com> > > > > > > > > Sent: Wednesday, January 19, 2022 9:59 AM > > > > > > > > To: Lin, Wayne <Wayne.Lin(a)amd.com> > > > > > > > > Cc: dri-devel(a)lists.freedesktop.org > > > > > > > > Subject: Re: [PATCH] WIP: drm/dp_mst: Add support for dumping > > > > > > > > topology > > > > > > > > ref > > > > > > > > histories from debugfs > > > > > > > > > > > > > > > > Hey - so, the original issue here was that payloads were not > > > > > > > > always > > > > > > > > deleted when we expected them to be - correct? > > > > > > > > > > > > > > Actually, what I was trying to do is not relevant to payload > > > > > > > maintenance. > > > > > > > What I wanted to do is having a way to notify > > > > > > > amdgpu to release its enumerated resource "dc_sink" when unplug > > > > > > > event > > > > > > > occurs. Currently, under specific scenarios, > > > > > > > we can't release remote "dc_sink" and ends up can't allocate > > > > > > > resource > > > > > > > for > > > > > > > new connected monitors. I'll explain the scenario > > > > > > > underneath. > > > > > > > > > > > > > > So, get back to the main issue that I was trying to solve when I > > > > > > > pushed > > > > > > > the > > > > > > > patchset: > > > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork… > > ayne.lin%40amd.com%7Cbbc27043edc04bf8067c08d9fbe9620c%7C3dd8961fe4884e608e > > 11a82d994e183d%7C0%7C0%7C6378178020798 > > 98960%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6 > > Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdat > > a=RVUc8KgTO3mhFL9qemshHJqiyXJs%2Fvytx21AMyUaW%2Bs%3D&reserved=0 > > > > > > > - > > > > > > devel%2Fcover%2F20210720160342.11415-1- > > > > > > Wayne.Lin%40amd.com%2F&data=04%7C01%7Cwayne.lin%40amd.com%7C2e > > > > > > 19d9 > > > > > > 32 > > > > > > 1b > > > > > > 5f4e948c1208d9e6884f24%7C3dd8961fe488 > > > > > > 4e608e11a82d994e183d%7C0%7C0%7C637794295422403301%7CUnknown%7CTWFp > > > > > > bGZs > > > > > > b3 > > > > > > d8 > > > > > > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL > > > > > > CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=rvtwQioO3pT1BoHb7S5 > > > > > > 32ms > > > > > > Ra > > > > > > 61 > > > > > > gP1vIyE8AB7M9I4E%3D&reserved=0 > > > > > > > > > > > > > > I was trying to fix some problems that I observed after these 2 > > > > > > > patches > > > > > > > * 09b974e8983 drm/amd/amdgpu_dm/mst: Remove - > > > > > > > >destroy_connector() > > > > > > > callback > > > > > > > * 72dc0f51591 drm/dp_mst: Remove > > > > > > > drm_dp_mst_topology_cbs.destroy_connector > > > > > > > > > > > > > > With above patches, it changes our flow to remove dc_sink (by > > > > > > > calling > > > > > > > dc_link_remote_sink()) when connector is about to be > > > > > > > destroyed. And let's see below use cases: > > > > > > > a). Disconnection happens between mst branch devices > > > > > > > e.g. > > > > > > > src - 1st_mstb - 2nd_mstb - sst_monitor => src - 1st_mstb > > > > > > > (disconnect) > > > > > > > 2nd_mstb - sst_monitor > > > > > > > > > > > > > > In above case, after receiving CSN, we will put topology > > > > > > > reference > > > > > > > count > > > > > > > of > > > > > > > 2nd mstb and its port which connected to the sst monitor. > > > > > > > As you suggested before, we will destroy 2nd mst branch device > > > > > > > and > > > > > > > its > > > > > > > all > > > > > > > ports due to there is no more mst path can route to them. > > > > > > > As the result of that, we at end call > > > > > > > drm_dp_delayed_destroy_port() > > > > > > > and > > > > > > > unregister/put the drm connector and eventually call > > > > > > > dc_link_remote_sink() to release the dc_sinck we enumerate for > > > > > > > the > > > > > > > specific > > > > > > > stream sink of sst_monitor. > > > > > > > > > > > > > > However, in below case: > > > > > > > b). Disconnection happens between mst branch devices and SST > > > > > > > monitor > > > > > > > e.g. > > > > > > > src - 1st_mstb - sst_monitor => src - 1st_mstb (disconnect) > > > > > > > sst_monitor > > > > > > > > > > > > > > In this case, which is the case having problem, it definitely > > > > > > > won't > > > > > > > decrease > > > > > > > the topology references count of the port of 1st_mstb which > > > > > > > was connected to the sst monitor to zero since the port is still > > > > > > > existing in > > > > > > > the topology. Same as the malloc reference since the port can't > > > > > > > get > > > > > > > destroyed. Hence, the port still exists and we won't call > > > > > > > drm_dp_delayed_destroy_port() to unregister and put the drm > > > > > > > connector. > > > > > > > As > > > > > > > the > > > > > > > result of that, we can't release corresponding remote dc_sink > > > > > > > under > > > > > > > this > > > > > > > case. And after few times hotplugs, we can't create any new > > > > > > > dc_sink since the number of stream sink is exceeding our > > > > > > > limitation. > > > > > > > > > > > > > > Hence, what I'm thinking here is to register a callback function > > > > > > > for > > > > > > > notifying us that the remote sink is detached. This just aligns > > > > > > > what > > > > > > > we > > > > > > > do > > > > > > > for handling long HPD event (unplug monitor from system) of > > > > > > > legacy > > > > > > > DP > > > > > > > (none > > > > > > > mst). For which case, once we detect long HPD event > > > > > > > indicating the monitor is detached, we will immediately try to > > > > > > > release > > > > > > > the > > > > > > > dc_link->local_sink and fire hotplug event to upper layer. > > > > > > > Same as here, once receives a CSN message notifying a drm > > > > > > > connector > > > > > > > is > > > > > > > changed from connected to disconnected, trigger the callback > > > > > > > function and we can try to release the dc_sink resource. > > > > > > > > > > > > > > > > > > > > > > > If I'm remembering that correctly, then (and I'm not 100% sure > > > > > > > > on > > > > > > > > this > > > > > > > > yet) I might already have noticed something very possibly > > > > > > > > wonky in > > > > > > > > how we handle payload allocations currently, that I found > > > > > > > > while > > > > > > > > working on > > > > > > > > the non-atomic removal branch that I linked to you in my > > > > > > > > previous response. Basically, in the > > > > > > > > step1() function it looks like that we follow this general > > > > > > > > flow > > > > > > > > with > > > > > > > > updating > > > > > > > > payloads: > > > > > > > > > > > > > > > > * Loop through proposed payloads and compare to previously > > > > > > > > programmed > > > > > > > > payloads > > > > > > > > - If a payload has a different allocation then it > > > > > > > > previously > > > > > > > > did, > > > > > > > > update the > > > > > > > > payload > > > > > > > > - If the payload is new, create it > > > > > > > > - If a payload no longer has an allocation, remove the > > > > > > > > payload > > > > > > > > > > > > > > > > At first glance this seems totally correct - but I'm not > > > > > > > > actually > > > > > > > > entirely > > > > > > > > convinced it is? Particularly because it seems like the order > > > > > > > > in > > > > > > > > which > > > > > > > > we do creation/deletion of payloads is totally arbitrary. To > > > > > > > > explain > > > > > > > > what > > > > > > > > I mean by that, consider a state transition like this: > > > > > > > > > > > > > > > > vcpi_slots=15 vcpi_slots=35 vcpi_slots=14 > > > > > > > > > 1 | 2 |xxxxxxxx| > > > > > > > > > > > > > > > > Let's say we want to increase payload #1 from 15 to 50, and > > > > > > > > disable > > > > > > > > payload #2 in the same atomic commit on DRM's side. If the > > > > > > > > order > > > > > > > > we > > > > > > > > update payloads is entirely arbitrary, we could end up doing > > > > > > > > this: > > > > > > > > > > > > > > > > * Increase VCPI slots payload #1 from 15->50 (total VCPI > > > > > > > > slots=99, > > > > > > > > overflow!) > > > > > > > > * Decrease VCPI slots payload #2 from 35->0 (total VCPI > > > > > > > > slots=50) > > > > > > > > > > > > > > > > Notice on the first step, we've technically overflowed the > > > > > > > > available > > > > > > > > number of VCPI slots in the payload table. This is still > > > > > > > > before > > > > > > > > step > > > > > > > > 2 > > > > > > > > though, and I could be totally wrong here - perhaps this is > > > > > > > > entirely > > > > > > > > OK in > > > > > > > > the real world, and we're allowed to overflow VC slots as long > > > > > > > > as > > > > > > > > > > > > > > I think it's forbidden to allocate time slots beyond 64 when > > > > > > > we're > > > > > > > updating > > > > > > > payload ID table of the branch in the real world. Branch > > > > > > > probably > > > > > > > will NACK the DPCD write. > > > > > > > > > > > > > > > we repair the issue before step 2. But so far I can't seem to > > > > > > > > find > > > > > > > > anything in the DP 2.0 spec that explicitly states this would > > > > > > > > be > > > > > > > > OK > > > > > > > > - > > > > > > > > which > > > > > > > > makes me think we might fail some payload allocations if we > > > > > > > > don't > > > > > > > > always > > > > > > > > ensure we avoid overflows in between VC payload table changes > > > > > > > > > > > > > > Agree. > > > > > > > For amdgpu, I think we always do deallocation (call hws- > > > > > > > > funcs.reset_hw_ctx_wrap()) before allocation > > > > > > > (apply_single_controller_ctx_to_hw()). > > > > > > > > > > > > > > > Note that avoiding overflows would be as simple as just making > > > > > > > > sure > > > > > > > > we > > > > > > > > send all VC payload table changes that free VC slots before > > > > > > > > sending > > > > > > > > any that take new slots. > > > > > > > > > > > > > > > > Again - I haven't actually confirmed this yet and am hoping to > > > > > > > > test > > > > > > > > stuff > > > > > > > > like this very soon as I'm pretty close finishing up the > > > > > > > > initial > > > > > > > > attempt > > > > > > > > at removing the non-atomic MST code in the DRM helpers now. If > > > > > > > > my > > > > > > > > theory > > > > > > > > ends up being correct here, I can fix this in my rewrite of > > > > > > > > the > > > > > > > > MST payload management code. But I figured it might be worth > > > > > > > > mentioning in > > > > > > > > the mean time in case you think it might be relevant to the > > > > > > > > problem here :). > > > > > > > > > > > > > > > > On Wed, 2022-01-12 at 16:47 -0500, Lyude Paul wrote: > > > > > > > > > (CC'ing this to dri-devel, since this is basically patch > > > > > > > > > review) > > > > > > > > > > > > > > > > > > Alright - so first off sorry about the broken debugging > > > > > > > > > patch! I > > > > > > > > > thought I had tested that but I guess I hadn't tested it > > > > > > > > > well > > > > > > > > > enough, > > > > > > > > > I'll get it fixed asap, but feel free to try getting to it > > > > > > > > > before > > > > > > > > > I > > > > > > > > > do. > > > > > > > > > > > > > > > > > > As for this patch... (comments below) > > > > > > > > > > > > > > > > > > On Mon, 2021-12-20 at 02:17 +0000, Lin, Wayne wrote: > > > > > > > > > > [AMD Official Use Only] > > > > > > > > > > > > > > > > > > > > Hi Lyude, > > > > > > > > > > > > > > > > > > > > No rush and thanks for your time! : ) Just want to help > > > > > > > > > > clarify > > > > > > > > > > what > > > > > > > > > > I mean that "registering a callback function" > > > > > > > > > > for CSN of disconnecting > > > > > > > > > > stream sink device (not mst branch case). Attach below the > > > > > > > > > > tentative > > > > > > > > > > patch that I planned to do. Thanks so much! > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > Wayne > > > > > > > > > > --- > > > > > > > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 53 > > > > > > > > > > +++++++++++++++++++ > > > > > > > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 16 +++++- > > > > > > > > > > include/drm/drm_dp_mst_helper.h | 1 + > > > > > > > > > > 3 files changed, 68 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type > > > > > > > > > > s.c > > > > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type > > > > > > > > > > s.c > > > > > > > > > > index cc34a35d0bcb..d7343c64908c 100644 > > > > > > > > > > --- > > > > > > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type > > > > > > > > > > s.c > > > > > > > > > > +++ > > > > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type > > > > > > > > > > s.c > > > > > > > > > > @@ -472,8 +472,61 @@ dm_dp_add_mst_connector(struct > > > > > > > > > > drm_dp_mst_topology_mgr *mgr, > > > > > > > > > > return connector; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +void dm_dp_notify_csn_disconnection(struct drm_connector > > > > > > > > > > +*connector) { > > > > > > > > > > + struct amdgpu_dm_connector *aconnector = > > > > > > > > > > + to_amdgpu_dm_connector(connector); > > > > > > > > > > + struct dc_sink *dc_sink = aconnector->dc_sink; > > > > > > > > > > + struct dc_link *dc_link = aconnector->dc_link; > > > > > > > > > > + struct amdgpu_device *adev = drm_to_adev(ddev); > > > > > > > > > > + > > > > > > > > > > + ASSERT(dc_link); > > > > > > > > > > + > > > > > > > > > > + if (dc_sink) { > > > > > > > > > > + mutex_lock(&adev->dm.dc_lock); > > > > > > > > > > + > > > > > > > > > > + /*clear the remote sink of the link*/ > > > > > > > > > > + dc_link_remove_remote_sink(dc_link, > > > > > > > > > > dc_sink); > > > > > > > > > > + dc_sink_release(dc_sink); > > > > > > > > > > + aconnector->dc_sink = NULL; > > > > > > > > > > + > > > > > > > > > > + mutex_unlock(&adev->dm.dc_lock); > > > > > > > > > > + } > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > static const struct drm_dp_mst_topology_cbs dm_mst_cbs = > > > > > > > > > > { > > > > > > > > > > .add_connector = dm_dp_add_mst_connector, > > > > > > > > > > + .notify_csn_disconnection = > > > > > > > > > > dm_dp_notify_csn_disconnection, > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > I still don't really think this is a good idea. This seems > > > > > > > > > like > > > > > > > > > it's > > > > > > > > > just adding another hotplugging path to the code in order to > > > > > > > > > avoid > > > > > > > > > sending hotplugs for non-endpoint devices. In addition to > > > > > > > > > the > > > > > > > > > > > > > > So, what I wanted to do is to add a callback function for > > > > > > > endpoint > > > > > > > devices > > > > > > > case as described above. For disconnection of non-endpoint > > > > > > > devices > > > > > > > is > > > > > > > already taken care by current code. > > > > > > > > > > > > > > > > drm_connector issues I mentioned before, we also really need > > > > > > > > > to > > > > > > > > > stop > > > > > > > > > doing any kind of payload maintence in hotplugging paths. > > > > > > > > > The > > > > > > > > > reality > > > > > > > > > is any kind of payload maintanence we do outside of normal > > > > > > > > > modesetting > > > > > > > > > paths is a relic from legacy modesetting that I'm dropping > > > > > > > > > ASAP > > > > > > > > > (more > > > > > > > > > on this below), and we can't keep adding to it because it > > > > > > > > > dramatically > > > > > > > > > complicates maintanence as well. > > > > > > > > > > > > > > > > > > > > > > > > > Sorry for repeating this point so often but - the biggest > > > > > > > > > issue > > > > > > > > > too > > > > > > > > > is > > > > > > > > > I'm still not sure what it is we're even avoiding here. We > > > > > > > > > know > > > > > > > > > resources aren't released consistently, and that we're able > > > > > > > > > to > > > > > > > > > reproduce the behavior with repeated hotplugs. We also know > > > > > > > > > that > > > > > > > > > if > > > > > > > > > we > > > > > > > > > skip sending certain hotplug events, that fixes the issue. > > > > > > > > > And > > > > > > > > > we > > > > > > > > > know > > > > > > > > > we can workaround it by adding a special case for forcing a > > > > > > > > > payload > > > > > > > > > release in DC. But none of those actually tell us exactly > > > > > > > > > what > > > > > > > > > piece > > > > > > > > > of code is leaking and why, which means that any workarounds > > > > > > > > > we're > > > > > > > > > putting in to avoid this mysterious guilty section of code > > > > > > > > > we > > > > > > > > > don't > > > > > > > > > entirely understand either - which means we're just adding > > > > > > > > > more > > > > > > > > > code > > > > > > > > > in that no one actually fully understands. This just ends up > > > > > > > > > making > > > > > > > > > maintence difficult because every change in code nearby > > > > > > > > > workarounds > > > > > > > > > like this has to strugle to try to figure out said > > > > > > > > > workarounds > > > > > > > > > in > > > > > > > > > order to avoid breaking things. > > > > > > > > > > > > > > Thank you Lyude for the reminder and I totally agree with you by > > > > > > > avoiding > > > > > > > adding workaround since it will get the code maintenance harder. > > > > > > > And > > > > > > > I > > > > > > > think the root cause of the issue that I observed before is > > > > > > > quite > > > > > > > concrete. > > > > > > > There is no code path for us to release dc_sink under the unplug > > > > > > > scenario > > > > > > > that I described above. > > > > > > > > > > > > > > > > > > > > > > > > > I'm actually currently running into these "later" issues > > > > > > > > > right > > > > > > > > > now, > > > > > > > > > as > > > > > > > > > recently I've (-actively-, finally!!!) been working on > > > > > > > > > trying to > > > > > > > > > remove as much non-atomic MST as I can because. As it turns > > > > > > > > > out > > > > > > > > > - > > > > > > > > > a > > > > > > > > > huge amount of the payload maintanence code just isn't > > > > > > > > > needed at > > > > > > > > > all > > > > > > > > > when we stop caring about non-atomic drivers and stick > > > > > > > > > everything > > > > > > > > > in > > > > > > > > > atomic state structs. Step 1 for updating updating the > > > > > > > > > payload > > > > > > > > > table, > > > > > > > > > e.g. drm_dp_update_payload_part1(), is a great example of > > > > > > > > > how > > > > > > > > > messy > > > > > > > > > things have become. Here's a small sample of some of the > > > > > > > > > stuff > > > > > > > > > I've > > > > > > > > > seen from just that one function so far that either just > > > > > > > > > don't > > > > > > > > > make > > > > > > > > > sense here or is totally redundant. I should note that a lot > > > > > > > > > of > > > > > > > > > these > > > > > > > > > things also come from patches I reviwed, but didn't really > > > > > > > > > look > > > > > > > > > at > > > > > > > > > as > > > > > > > > > closely as I should have because I was swamped at work, some > > > > > > > > > are > > > > > > > > > historical artifacts, and others are less-than-ideal patches > > > > > > > > > I > > > > > > > > > got > > > > > > > > > wrote myself when I was first started working on MST and > > > > > > > > > didn't > > > > > > > > > know > > > > > > > > > the code as well as I do > > > > > > > > > now: > > > > > > > > > > > > > > > > > > * We try to avoid some sort of userspace issue by using > > > > > > > > > drm_dp_mst_port_downstream_of_branch() to avoid releasing > > > > > > > > > payloads > > > > > > > > > for a > > > > > > > > > branch if we can't prove it's downstream of the top of > > > > > > > > > the > > > > > > > > > topology. This > > > > > > > > > seems to workaround either a userspace bug. This is a > > > > > > > > > redundant, > > > > > > > > > since > > > > > > > > > that's what topology refs are already supposed to be > > > > > > > > > doing to > > > > > > > > > the > > > > > > > > > extent is > > > > > > > > > reasonably possibly. It's also unfortunately racy > > > > > > > > > solution > > > > > > > > > because > > > > > > > > > we have > > > > > > > > > to be able to handle the situation where a connector is > > > > > > > > > removed > > > > > > > > > from under > > > > > > > > > us. That can happen at any time, including _immediately_ > > > > > > > > > after > > > > > > > > > we > > > > > > > > > call > > > > > > > > > drm_dp_mst_port_downstream_of_branch() - rendering the > > > > > > > > > call > > > > > > > > > not > > > > > > > > > really > > > > > > > > > useful. > > > > > > > > > * If we fail to validate the sink in > > > > > > > > > drm_dp_update_payload_part, > > > > > > > > > we > > > > > > > > > don't > > > > > > > > > update the payload table. I think at best this solution > > > > > > > > > is > > > > > > > > > racy > > > > > > > > > and > > > > > > > > > not > > > > > > > > > useful, at worst it leaves us with a payload table that > > > > > > > > > doesn't > > > > > > > > > match what > > > > > > > > > we attempted to set in the atomic state - which at worst > > > > > > > > > brings > > > > > > > > > us > > > > > > > > > into > > > > > > > > > undefined territory where we're just plain out of sync > > > > > > > > > with > > > > > > > > > the > > > > > > > > > reality in > > > > > > > > > hw. > > > > > > > > > * Actually fun fact - mgr->payloads and mgr->proposed_vcpis > > > > > > > > > both > > > > > > > > > can > > > > > > > > > and > > > > > > > > > definitely should be removed entirely. All of the info > > > > > > > > > for > > > > > > > > > mgr->payloads > > > > > > > > > could just be in the atomic state, because that + the > > > > > > > > > magic > > > > > > > > > of > > > > > > > > > atomic state > > > > > > > > > duplication means we'll also have an accurate view of the > > > > > > > > > previous > > > > > > > > > state's > > > > > > > > > payload allocations: which renders mgr->proposed_vcpis > > > > > > > > > redundant. > > > > > > > > > > > > > > > > > > Apologies for the long explanation again, but I hope that > > > > > > > > > explains > > > > > > > > > my > > > > > > > > > > > > > > Really really thanks for your kindly help on this : ) > > > > > > > I'll try my best to get to your WIP branch soon. Thank you > > > > > > > Lyude! > > > > > > > > > > > > > > > > point here a bit. I'm going to be trying to get to moving > > > > > > > > > amdgpu's > > > > > > > > > DSC > > > > > > > > > code out of amdgpu and into DRM helpers as well soon, so I'm > > > > > > > > > really > > > > > > > > > determined to clean stuff up beforehand as every time I've > > > > > > > > > done > > > > > > > > > so > > > > > > > > > it's become substantially easier to make changes to this > > > > > > > > > code. > > > > > > > > > Things > > > > > > > > > used to be even worse before I started cleaning things up 2 > > > > > > > > > or 3 > > > > > > > > > years > > > > > > > > > ago, where simple changes would end up getting me stuck > > > > > > > > > spending > > > > > > > > > hours > > > > > > > > > trying to dig through lockdep or memory manegement issues. > > > > > > > > > As > > > > > > > > > well, > > > > > > > > > I > > > > > > > > > would be entirely unsurprised if bugs like this very payload > > > > > > > > > leak > > > > > > > > > we're working on just disappear once we've gotten rid of all > > > > > > > > > the > > > > > > > > > extraneous workarounds and state tracking here - especially > > > > > > > > > with > > > > > > > > > how > > > > > > > > > many special cases we have for maintaining the payload table > > > > > > > > > right > > > > > > > > > now. That's certainly ended up being the case in the past > > > > > > > > > with a > > > > > > > > > number of other difficult to track down issues I've dealt > > > > > > > > > with > > > > > > > > > in > > > > > > > > > MST. > > > > > > > > > > > > > > > > > > Anyhow. I've been way more productive this year then last > > > > > > > > > because > > > > > > > > > I > > > > > > > > > had over a month off work and am finally not super burnt out > > > > > > > > > from > > > > > > > > > my > > > > > > > > > job, and so far I've been making progress on the payload > > > > > > > > > state > > > > > > > > > cleanup > > > > > > > > > far faster then I was last year :). I think if you can't > > > > > > > > > figure > > > > > > > > > out > > > > > > > > > where the leak is coming from even once I get the debugging > > > > > > > > > patches > > > > > > > > > I > > > > > > > > > mentioned fixed up, it might be a good idea for us to try > > > > > > > > > again > > > > > > > > > after > > > > > > > > > I've got some of this code cleaned up. I've got a currently > > > > > > > > > WIP > > > > > > > > > branch > > > > > > > > > here: > > > > > > > > > > > > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl > > > > > > > > > ab.freedesktop.org%2Flyudess%2Flinux%2F- > > > > > > > > > %2Fcommit%2F624dd68fa804e64b5b > > > > > > > > > 2060e4735d7317090692b5&data=04%7C01%7Cwayne.lin%40amd.co > > > > > > > > > m%7C > > > > > > > > > d4 > > > > > > > > > bd > > > > > > > > > c7 > > > > > > > > > 59eb274bfccc8208d9daef41fe%7C3dd8961fe4884e608e11a82d994e183 > > > > > > > > > d%7C > > > > > > > > > 0% > > > > > > > > > 7C > > > > > > > > > 0% > > > > > > > > > 7C637781543416430632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > > > > > > > > > wMDA > > > > > > > > > iL > > > > > > > > > CJ > > > > > > > > > QI > > > > > > > > > joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=n > > > > > > > > > FpVl > > > > > > > > > %2 > > > > > > > > > BV > > > > > > > > > oF > > > > > > > > > 40JPabfRMcR6Cz7ZHDwt%2BBpLDBNdXftJaA%3D&reserved=0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > void amdgpu_dm_initialize_dp_connector(struct > > > > > > > > > > amdgpu_display_manager *dm, diff --git > > > > > > > > > > a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > index 857c5d15e81d..a70e26c5d084 100644 > > > > > > > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > @@ -2508,6 +2508,7 @@ drm_dp_mst_handle_conn_stat(struct > > > > > > > > > > drm_dp_mst_branch *mstb, > > > > > > > > > > u8 new_pdt; > > > > > > > > > > bool new_mcs; > > > > > > > > > > bool dowork = false, create_connector = false; > > > > > > > > > > + bool notify_unplug_event = false; > > > > > > > > > > > > > > > > > > > > port = drm_dp_get_port(mstb, conn_stat- > > > > > > > > > > >port_number); > > > > > > > > > > if (!port) > > > > > > > > > > @@ -2520,6 +2521,9 @@ drm_dp_mst_handle_conn_stat(struct > > > > > > > > > > drm_dp_mst_branch *mstb, > > > > > > > > > > * port, so just throw the port > > > > > > > > > > out > > > > > > > > > > and > > > > > > > > > > make > > > > > > > > > > sure we > > > > > > > > > > * reprobe the link address of > > > > > > > > > > it's > > > > > > > > > > parent > > > > > > > > > > MSTB > > > > > > > > > > */ > > > > > > > > > > + /* should also consider > > > > > > > > > > notify_unplug_event > > > > > > > > > > here. > > > > > > > > > > + * but it's not a normal case for > > > > > > > > > > products > > > > > > > > > > +in the > > > > > > > > > > market > > > > > > > > > > + */ > > > > > > > > > > > > > > > > > > > > drm_dp_mst_topology_unlink_port(mgr, > > > > > > > > > > port); > > > > > > > > > > mstb->link_address_sent = false; > > > > > > > > > > dowork = true; @@ -2541,10 > > > > > > > > > > +2545,14 @@ > > > > > > > > > > drm_dp_mst_handle_conn_stat(struct > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > *mstb, > > > > > > > > > > port->ddps = conn_stat- > > > > > > > > > > > displayport_device_plug_status; > > > > > > > > > > > > > > > > > > > > if (old_ddps != port->ddps) { > > > > > > > > > > - if (port->ddps && !port->input) > > > > > > > > > > + if (port->ddps && !port->input) { > > > > > > > > > > > > > > > > > > > > drm_dp_send_enum_path_resources(mgr, > > > > > > > > > > mstb, > > > > > > > > > > port); > > > > > > > > > > - else > > > > > > > > > > + } else { > > > > > > > > > > port->full_pbn = 0; > > > > > > > > > > + if (port->connector && > > > > > > > > > > + > > > > > > > > > > drm_dp_mst_is_end_device(port- > > > > > > > > > > > pdt, > > > > > > > > > > +port- > > > > > > > > > > > mcs)) > > > > > > > > > > + notify_unplug_event = > > > > > > > > > > true; > > > > > > > > > > + } > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > new_pdt = port->input ? DP_PEER_DEVICE_NONE : > > > > > > > > > > conn_stat- > > > > > > > > > > > peer_device_type; > > > > > > > > > > @@ -2557,11 +2565,15 @@ drm_dp_mst_handle_conn_stat(struct > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > *mstb, > > > > > > > > > > dowork = false; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > + if (notify_unplug_event && > > > > > > > > > > +mgr->cbs->notify_csn_disconnection) > > > > > > > > > > + mgr->cbs->notify_csn_disconnection(port- > > > > > > > > > > > connector); > > > > > > > > > > + > > > > > > > > > > if (port->connector) > > > > > > > > > > drm_modeset_unlock(&mgr->base.lock); > > > > > > > > > > else if (create_connector) > > > > > > > > > > drm_dp_mst_port_add_connector(mstb, port); > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > out: > > > > > > > > > > drm_dp_mst_topology_put_port(port); > > > > > > > > > > if (dowork) > > > > > > > > > > diff --git a/include/drm/drm_dp_mst_helper.h > > > > > > > > > > b/include/drm/drm_dp_mst_helper.h index > > > > > > > > > > 78044ac5b59b..ff9e47729841 > > > > > > > > > > 100644 > > > > > > > > > > --- a/include/drm/drm_dp_mst_helper.h > > > > > > > > > > +++ b/include/drm/drm_dp_mst_helper.h > > > > > > > > > > @@ -525,6 +525,7 @@ struct drm_dp_mst_topology_cbs { > > > > > > > > > > * IRQ pulse handler. > > > > > > > > > > */ > > > > > > > > > > void (*poll_hpd_irq)(struct > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > *mgr); > > > > > > > > > > + void (*notify_csn_disconnection)(struct > > > > > > > > > > drm_connector > > > > > > > > > > +*connector); > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > #define DP_MAX_PAYLOAD (sizeof(unsigned long) * 8) > > > > > > > > > > -- > > > > > > > > > > 2.31.0 > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > > From: Lin, Wayne > > > > > > > > > > > Sent: Wednesday, December 8, 2021 11:39 AM > > > > > > > > > > > To: 'Lyude Paul' <lyude(a)redhat.com> > > > > > > > > > > > Subject: RE: [PATCH] WIP: drm/dp_mst: Add support for > > > > > > > > > > > dumping > > > > > > > > > > > topology ref histories from debugfs > > > > > > > > > > > > > > > > > > > > > > No worries Lyude! > > > > > > > > > > > Thanks for keeping helping on this. Take your time : ) > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > > > From: Lyude Paul <lyude(a)redhat.com> > > > > > > > > > > > > Sent: Wednesday, December 8, 2021 7:05 AM > > > > > > > > > > > > To: Lin, Wayne <Wayne.Lin(a)amd.com> > > > > > > > > > > > > Subject: Re: [PATCH] WIP: drm/dp_mst: Add support for > > > > > > > > > > > > dumping > > > > > > > > > > > > topology ref histories from debugfs > > > > > > > > > > > > > > > > > > > > > > > > Sorry! I will try to get to this tomorrow, if not then > > > > > > > > > > > > sometime > > > > > > > > > > > > this week. > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2021-11-30 at 08:41 +0000, Lin, Wayne wrote: > > > > > > > > > > > > > [Public] > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Lyude, > > > > > > > > > > > > > > > > > > > > > > > > > > Finally have some bandwidth to get back to this > > > > > > > > > > > > > problem! > > > > > > > > > > > > > I roughly went through this patch and I'm just aware > > > > > > > > > > > > > that > > > > > > > > > > > > > we > > > > > > > > > > > > > already have such kind of convenient tool for a > > > > > > > > > > > > > while. > > > > > > > > > > > > > I think it's definitely useful for us to track > > > > > > > > > > > > > port/mstb > > > > > > > > > > > > > reference count issues and I'll start to embrace > > > > > > > > > > > > > this > > > > > > > > > > > > > feature > > > > > > > > > > > > > for cleaning up those issues. Thank you Lyude! > > > > > > > > > > > > > > > > > > > > > > > > > > However, I think the issue that I was trying to fix > > > > > > > > > > > > > is > > > > > > > > > > > > > not > > > > > > > > > > > > > related to what you suggested: > > > > > > > > > > > > > " The idea here is that if stream resources aren't > > > > > > > > > > > > > being > > > > > > > > > > > > > released, my guess would be that we're not dropping > > > > > > > > > > > > > topology > > > > > > > > > > > > > references for the port which means the connector > > > > > > > > > > > > > never > > > > > > > > > > > > > goes > > > > > > > > > > > > > away." > > > > > > > > > > > > > The issue I was trying to fix is about releasing > > > > > > > > > > > > > dc_link->remote_sinks while receiving a CSN message > > > > > > > > > > > > > notifying > > > > > > > > > > > > > the connection status of a sst connector of a port > > > > > > > > > > > > > changed > > > > > > > > > > > > > from connected to disconnected. Not the connection > > > > > > > > > > > > > status > > > > > > > > > > > > > changed of a mst branch device. > > > > > > > > > > > > > e.g. > > > > > > > > > > > > > src - 1st_mstb - 2nd_mstb - sst_monitor => src - > > > > > > > > > > > > > 1st_mstb > > > > > > > > > > > > > (disconnect) 2nd_mstb - sst_monitor > > > > > > > > > > > > > > > > > > > > > > > > > > In above case, after receiving CSN, we will put > > > > > > > > > > > > > topology > > > > > > > > > > > > > references of 2nd mstb and its port which is > > > > > > > > > > > > > connected > > > > > > > > > > > > > with > > > > > > > > > > > > > the sst monitor. As the result of that, we can call > > > > > > > > > > > > > drm_dp_delayed_destroy_port() to unregister and put > > > > > > > > > > > > > the > > > > > > > > > > > > > drm > > > > > > > > > > > > > connector. > > > > > > > > > > > > > > > > > > > > > > > > > > However, in below case: > > > > > > > > > > > > > e.g. > > > > > > > > > > > > > src - 1st_mstb - sst_monitor => src - 1st_mstb > > > > > > > > > > > > > (disconnect) > > > > > > > > > > > > > sst_monitor > > > > > > > > > > > > > > > > > > > > > > > > > > In this case, which is the case having problem, it > > > > > > > > > > > > > definitely > > > > > > > > > > > > > won't decrease the topology references count of the > > > > > > > > > > > > > port > > > > > > > > > > > > > which > > > > > > > > > > > > > was connected to the sst monitor to zero since the > > > > > > > > > > > > > port > > > > > > > > > > > > > is > > > > > > > > > > > > > still existing in the topology. Same as the malloc > > > > > > > > > > > > > reference > > > > > > > > > > > > > since the port can't get destroyed. Hence, the port > > > > > > > > > > > > > still > > > > > > > > > > > > > exists and we won't call > > > > > > > > > > > > > drm_dp_delayed_destroy_port() to unregister and put > > > > > > > > > > > > > the > > > > > > > > > > > > > drm > > > > > > > > > > > > > connector. > > > > > > > > > > > > > I looked up the code and > > > > > > > > > > > > > drm_dp_delayed_destroy_port() > > > > > > > > > > > > > seems > > > > > > > > > > > > > like the only place to call drm_connector_put() > > > > > > > > > > > > > which > > > > > > > > > > > > > means > > > > > > > > > > > > > we > > > > > > > > > > > > > can't put reference count of drm connector under > > > > > > > > > > > > > this > > > > > > > > > > > > > case > > > > > > > > > > > > > and > > > > > > > > > > > > > can't release dc_sink resource by destroying drm > > > > > > > > > > > > > connector. > > > > > > > > > > > > > > > > > > > > > > > > > > I would also like to point out that this resource > > > > > > > > > > > > > (remote_sinks) is specific to different stream > > > > > > > > > > > > > sinks. So > > > > > > > > > > > > > if > > > > > > > > > > > > > we're trying to release this dc_sink resource by > > > > > > > > > > > > > destroying > > > > > > > > > > > > > the drm connector, it conflicts the idea that you > > > > > > > > > > > > > suggested > > > > > > > > > > > > > before that we should always keep the drm connector > > > > > > > > > > > > > until > > > > > > > > > > > > > it's > > > > > > > > > > > > > no longer reachable in the topology. > > > > > > > > > > > > > Releasing dc_sink should be binding with the > > > > > > > > > > > > > disconnection > > > > > > > > > > > > > event. > > > > > > > > > > > > > > > > > > > > > > > > > > I understand your concern that we should not just > > > > > > > > > > > > > easily > > > > > > > > > > > > > change the logic here since it's the result after > > > > > > > > > > > > > solving > > > > > > > > > > > > > tons > > > > > > > > > > > > > of bugs before and might cause other side effect. > > > > > > > > > > > > > So, > > > > > > > > > > > > > just > > > > > > > > > > > > > my > > > > > > > > > > > > > 2 cents, what I'm thinking is to register a callback > > > > > > > > > > > > > function > > > > > > > > > > > > > for our driver to notify us that the remote sink is > > > > > > > > > > > > > detached. > > > > > > > > > > > > > This just aligns our flow handling long HPD event of > > > > > > > > > > > > > legacy > > > > > > > > > > > > > (sst) DP. > > > > > > > > > > > > > For sst case, once we detect long HPD event > > > > > > > > > > > > > indicating > > > > > > > > > > > > > the > > > > > > > > > > > > > monitor is detached, we will immediately try to > > > > > > > > > > > > > release > > > > > > > > > > > > > the > > > > > > > > > > > > > dc_link->local_sink and fire hotplug event to upper > > > > > > > > > > > > > layer. > > > > > > > > > > > > > Same as here, once receives a CSN message notifying > > > > > > > > > > > > > a > > > > > > > > > > > > > drm > > > > > > > > > > > > > connector is changed from connected to disconnected, > > > > > > > > > > > > > trigger > > > > > > > > > > > > > the callback function and we can try to release the > > > > > > > > > > > > > dc_sink > > > > > > > > > > > > > resource. > > > > > > > > > > > > > > > > > > > > > > > > > > Would like to know your thought and insight please : > > > > > > > > > > > > > ) > > > > > > > > > > > > > > > > > > > > > > > > > > Btw, I got some errors and warnings while building > > > > > > > > > > > > > and > > > > > > > > > > > > > have > > > > > > > > > > > > > some code adjustments as below : ) Thank you Lyude > > > > > > > > > > > > > for > > > > > > > > > > > > > your > > > > > > > > > > > > > always kindly help!! > > > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > Wayne > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > > > > > From: Lyude Paul <lyude(a)redhat.com> > > > > > > > > > > > > > > Sent: Wednesday, November 3, 2021 7:15 AM > > > > > > > > > > > > > > To: Lin, Wayne <Wayne.Lin(a)amd.com> > > > > > > > > > > > > > > Subject: [PATCH] WIP: drm/dp_mst: Add support for > > > > > > > > > > > > > > dumping > > > > > > > > > > > > > > topology ref histories from debugfs > > > > > > > > > > > > > > > > > > > > > > > > > > > > TODO: > > > > > > > > > > > > > > * Implement support for i915 > > > > > > > > > > > > > > * Finish writing this commit message > > > > > > > > > > > > > > * ??? > > > > > > > > > > > > > > * profit > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Lyude Paul <lyude(a)redhat.com> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hey wayne! SO-hopefully if I did this correctly > > > > > > > > > > > > > > then > > > > > > > > > > > > > > this > > > > > > > > > > > > > > should just work on amdgpu. What this patch should > > > > > > > > > > > > > > do > > > > > > > > > > > > > > is > > > > > > > > > > > > > > add > > > > > > > > > > > > > > a debugfs file to amdgpu called > > > > > > > > > > > > > > "amdgpu_dp_mst_topology_refs", and when you read > > > > > > > > > > > > > > the > > > > > > > > > > > > > > file > > > > > > > > > > > > > > it > > > > > > > > > > > > > > should print out the topology reference history of > > > > > > > > > > > > > > every > > > > > > > > > > > > > > MSTB and Port in memory, along with how many times > > > > > > > > > > > > > > we've > > > > > > > > > > > > > > hit > > > > > > > > > > > > > > the codepath in each backtrace. An example: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Port DP-5 (0000000005c37748) topology ref history: > > > > > > > > > > > > > > 1 gets (last at 58.468973): > > > > > > > > > > > > > > drm_dp_send_link_address+0x6a5/0xa00 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > drm_dp_check_and_send_link_address+0xad/0xd0 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > drm_dp_mst_link_probe_work+0x14e/0x1a0 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > process_one_work+0x1e3/0x390 > > > > > > > > > > > > > > worker_thread+0x50/0x3a0 > > > > > > > > > > > > > > kthread+0x124/0x150 > > > > > > > > > > > > > > ret_from_fork+0x1f/0x30 > > > > > > > > > > > > > > 1 puts (last at 58.469357): > > > > > > > > > > > > > > drm_dp_mst_topology_put_port+0x6a/0x210 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > drm_dp_send_link_address+0x39e/0xa00 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > drm_dp_check_and_send_link_address+0xad/0xd0 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > drm_dp_mst_link_probe_work+0x14e/0x1a0 > > > > > > > > > > > > > > [drm_kms_helper] > > > > > > > > > > > > > > process_one_work+0x1e3/0x390 > > > > > > > > > > > > > > worker_thread+0x50/0x3a0 > > > > > > > > > > > > > > kthread+0x124/0x150 > > > > > > > > > > > > > > ret_from_fork+0x1f/0x30 > > > > > > > > > > > > > > > > > > > > > > > > > > > > The idea here is that if stream resources aren't > > > > > > > > > > > > > > being > > > > > > > > > > > > > > released, my guess would be that we're not > > > > > > > > > > > > > > dropping > > > > > > > > > > > > > > topology > > > > > > > > > > > > > > references for the port which means the connector > > > > > > > > > > > > > > never > > > > > > > > > > > > > > goes > > > > > > > > > > > > > > away. So, if that's really the case then once we > > > > > > > > > > > > > > unplug > > > > > > > > > > > > > > the > > > > > > > > > > > > > > offending connector we should be able to find a > > > > > > > > > > > > > > pair > > > > > > > > > > > > > > of > > > > > > > > > > > > > > gets/puts for the offending leaked connector where > > > > > > > > > > > > > > the > > > > > > > > > > > > > > get/put count doesn't match up. Also, if the frame > > > > > > > > > > > > > > count > > > > > > > > > > > > > > on > > > > > > > > > > > > > > the backtrace isn't long enough you can increase > > > > > > > > > > > > > > the > > > > > > > > > > > > > > value > > > > > > > > > > > > > > of STACK_DEPTH in > > > > > > > > > > > > > > drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > > > > > and > > > > > > > > > > > > > > recompile to get more frames. > > > > > > > > > > > > > > > > > > > > > > > > > > > > To enable this, first enable CONFIG_EXPERT for > > > > > > > > > > > > > > your > > > > > > > > > > > > > > kernel > > > > > > > > > > > > > > which will unhide > > > > > > > > > > > > > > CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS. > > > > > > > > > > > > > > Then just enable > > > > > > > > > > > > > > CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS, > > > > > > > > > > > > > > recompile, and it should be good to go. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Let me know if this works for you, and hopefully > > > > > > > > > > > > > > this > > > > > > > > > > > > > > should > > > > > > > > > > > > > > tell us exactly what the problem actually is here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | > > > > > > > > > > > > > > 35 > > > > > > > > > > > > > > ++++ > > > > > > > > > > > > > > drivers/gpu/drm/drm_dp_mst_topology.c | > > > > > > > > > > > > > > 173 > > > > > > > > > > > > > > ++++++++++++++---- > > > > > > > > > > > > > > drivers/gpu/drm/nouveau/nouveau_debugfs.c | > > > > > > > > > > > > > > 35 > > > > > > > > > > > > > > ++++ > > > > > > > > > > > > > > include/drm/drm_dp_mst_helper.h | > > > > > > > > > > > > > > 18 > > > > > > > > > > > > > > ++ > > > > > > > > > > > > > > 4 files changed, 228 insertions(+), 33 > > > > > > > > > > > > > > deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ > > > > > > > > > > > > > > debu > > > > > > > > > > > > > > gf > > > > > > > > > > > > > > s. > > > > > > > > > > > > > > c > > > > > > > > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ > > > > > > > > > > > > > > debu > > > > > > > > > > > > > > gf > > > > > > > > > > > > > > s. > > > > > > > > > > > > > > c > > > > > > > > > > > > > > index 1a68a674913c..1a14732c52b4 100644 > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ > > > > > > > > > > > > > > debu > > > > > > > > > > > > > > gf > > > > > > > > > > > > > > s. > > > > > > > > > > > > > > c > > > > > > > > > > > > > > +++ > > > > > > > > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ > > > > > > > > > > > > > > debu > > > > > > > > > > > > > > gf > > > > > > > > > > > > > > +++ s.c > > > > > > > > > > > > > > @@ -3063,6 +3063,37 @@ static int > > > > > > > > > > > > > > mst_topo_show(struct > > > > > > > > > > > > > > seq_file *m, void > > > > > > > > > > > > > > *unused) > > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > +#ifdef CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > > > > > > > > > > > > +static int mst_topology_ref_dump_show(struct > > > > > > > > > > > > > > seq_file > > > > > > > > > > > > > > *m, > > > > > > > > > > > > > > +void > > > > > > > > > > > > > > +*unused) { > > > > > > > > > > > > > > + struct amdgpu_device *adev = (struct > > > > > > > > > > > > > > amdgpu_device > > > > > > > > > > > > > > +*)m->private; > > > > > > > > > > > > > > + struct drm_device *dev = adev_to_drm(adev); > > > > > > > > > > > > > > + struct drm_connector *connector; > > > > > > > > > > > > > > + struct drm_connector_list_iter conn_iter; > > > > > > > > > > > > > > + struct amdgpu_dm_connector *aconnector; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + drm_connector_list_iter_begin(dev, > > > > > > > > > > > > > > &conn_iter); > > > > > > > > > > > > > > + drm_for_each_connector_iter(connector, > > > > > > > > > > > > > > &conn_iter) > > > > > > > > > > > > > > { > > > > > > > > > > > > > > + if (connector->connector_type != > > > > > > > > > > > > > > DRM_MODE_CONNECTOR_DisplayPort) > > > > > > > > > > > > > > + continue; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + aconnector = > > > > > > > > > > > > > > +to_amdgpu_dm_connector(connector); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + /* Ensure we're only dumping the > > > > > > > > > > > > > > topology > > > > > > > > > > > > > > of > > > > > > > > > > > > > > a > > > > > > > > > > > > > > +root mst node > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > + if (!aconnector- > > > > > > > > > > > > > > >mst_mgr.max_payloads) > > > > > > > > > > > > > > + continue; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + seq_printf(m, "\nMST topology for > > > > > > > > > > > > > > connector > > > > > > > > > > > > > > +%d\n", > > > > > > > > > > > > > > aconnector->connector_id); > > > > > > > > > > > > > > + drm_dp_mst_dump_topology_refs(m, > > > > > > > > > > > > > > +&aconnector->mst_mgr); > > > > > > > > > > > > > > + } > > > > > > > > > > > > > > + drm_connector_list_iter_end(&conn_iter); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + return 0; > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +DEFINE_SHOW_ATTRIBUTE(mst_topology_ref_dump); > > > > > > > > > > > > > > +#endif > > > > > > > > > > > > > > + > > > > > > > > > > > > > > /* > > > > > > > > > > > > > > * Sets trigger hpd for MST topologies. > > > > > > > > > > > > > > * All connected connectors will be rediscovered > > > > > > > > > > > > > > and > > > > > > > > > > > > > > re > > > > > > > > > > > > > > started as needed if val of 1 is sent. > > > > > > > > > > > > > > @@ -3299,6 +3330,10 @@ void > > > > > > > > > > > > > > dtn_debugfs_init(struct > > > > > > > > > > > > > > amdgpu_device > > > > > > > > > > > > > > *adev) > > > > > > > > > > > > > > > > > > > > > > > > > > > > debugfs_create_file("amdgpu_mst_topology", > > > > > > > > > > > > > > 0444, > > > > > > > > > > > > > > root, > > > > > > > > > > > > > > adev, &mst_topo_fops); > > > > > > > > > > > > > > +#ifdef CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > > > > > > > > > > > > + > > > > > > > > > > > > > > debugfs_create_file("amdgpu_dp_mst_topology_refs", > > > > > > > > > > > > > > +0444, root, adev, > > > > > > > > > > > > > > + > > > > > > > > > > > > > > &mst_topology_ref_dump_fops); > > > > > > > > > > > > > > +#endif > > > > > > > > > > > > > > debugfs_create_file("amdgpu_dm_dtn_log", > > > > > > > > > > > > > > 0644, > > > > > > > > > > > > > > root, > > > > > > > > > > > > > > adev, > > > > > > > > > > > > > > &dtn_log_fops); > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > > > > > index 1aa8702383d4..0159828c494d 100644 > > > > > > > > > > > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > > > > > > > > > > @@ -1366,23 +1366,6 @@ static int > > > > > > > > > > > > > > drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch > > > > > > > > > > > > > > *mstb, > > > > > > > > > > > > > > return ret; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > -static struct drm_dp_mst_branch > > > > > > > > > > > > > > *drm_dp_add_mst_branch_device(u8 lct, u8 > > > > > > > > > > > > > > *rad) -{ > > > > > > > > > > > > > > - struct drm_dp_mst_branch *mstb; > > > > > > > > > > > > > > - > > > > > > > > > > > > > > - mstb = kzalloc(sizeof(*mstb), GFP_KERNEL); > > > > > > > > > > > > > > - if (!mstb) > > > > > > > > > > > > > > - return NULL; > > > > > > > > > > > > > > - > > > > > > > > > > > > > > - mstb->lct = lct; > > > > > > > > > > > > > > - if (lct > 1) > > > > > > > > > > > > > > - memcpy(mstb->rad, rad, lct / 2); > > > > > > > > > > > > > > - INIT_LIST_HEAD(&mstb->ports); > > > > > > > > > > > > > > - kref_init(&mstb->topology_kref); > > > > > > > > > > > > > > - kref_init(&mstb->malloc_kref); > > > > > > > > > > > > > > - return mstb; > > > > > > > > > > > > > > -} > > > > > > > > > > > > > > - > > > > > > > > > > > > > > static void drm_dp_free_mst_branch_device(struct > > > > > > > > > > > > > > kref > > > > > > > > > > > > > > *kref) { > > > > > > > > > > > > > > struct drm_dp_mst_branch *mstb = @@ -1642,12 > > > > > > > > > > > > > > +1625,20 > > > > > > > > > > > > > > @@ topology_ref_type_to_str(enum > > > > > > > > > > > > > > drm_dp_mst_topology_ref_type type) > > > > > > > > > > > > > > return "put"; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > +static const char > > > > > > > > > > > > > > *topology_ref_history_type_to_str(enum > > > > > > > > > > > > > > +drm_dp_mst_topology_history_type type) { > > > > > > > > > > > > > > + if (type == > > > > > > > > > > > > > > DRM_DP_MST_TOPOLOGY_HISTORY_PORT) > > > > > > > > > > > > > > + return "Port"; > > > > > > > > > > > > > > + else > > > > > > > > > > > > > > + return "MSTB"; } > > > > > > > > > > > > > > + > > > > > > > > > > > > > > static void > > > > > > > > > > > > > > -__dump_topology_ref_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_ref_history *history, > > > > > > > > > > > > > > - void *ptr, const char > > > > > > > > > > > > > > *type_str) > > > > > > > > > > > > > > +dump_topology_ref_history(struct > > > > > > > > > > > > > > +drm_dp_mst_topology_ref_history *history, struct > > > > > > > > > > > > > > +drm_printer p) > > > > > > > > > > > > > > { > > > > > > > > > > > > > > - struct drm_printer p = > > > > > > > > > > > > > > drm_debug_printer(DBG_PREFIX); > > > > > > > > > > > > > > + char *connector_name = NULL; > > > > > > > > > > > > > > char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL); > > > > > > > > > > > > > > + void *ptr; > > > > > > > > > > > > > > int i; > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (!buf) > > > > > > > > > > > > > > @@ -1656,14 +1647,29 @@ > > > > > > > > > > > > > > __dump_topology_ref_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_ref_history *history, > > > > > > > > > > > > > > if (!history->len) > > > > > > > > > > > > > > goto out; > > > > > > > > > > > > > > > > > > > > > > > > > > > > + /* Get a pointer to the actual MSTB/port so > > > > > > > > > > > > > > we > > > > > > > > > > > > > > can > > > > > > > > > > > > > > the > > > > > > > > > > > > > > +memory > > > > > > > > > > > > > > address to the kernel log */ > > > > > > > > > > > > > > + if (history->type == > > > > > > > > > > > > > > DRM_DP_MST_TOPOLOGY_HISTORY_PORT) > > > > > > > > > > > > > > + ptr = container_of(history, struct > > > > > > > > > > > > > > +drm_dp_mst_port, > > > > > > > > > > > > > > topology_ref_history); > > > > > > > > > > > > > > + else > > > > > > > > > > > > > > + ptr = container_of(history, struct > > > > > > > > > > > > > > +drm_dp_mst_branch, topology_ref_history); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > /* First, sort the list so that it goes from > > > > > > > > > > > > > > oldest > > > > > > > > > > > > > > to > > > > > > > > > > > > > > newest > > > > > > > > > > > > > > * reference entry > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > sort(history->entries, history->len, > > > > > > > > > > > > > > sizeof(*history->entries), > > > > > > > > > > > > > > topology_ref_history_cmp, NULL); > > > > > > > > > > > > > > > > > > > > > > > > > > > > - drm_printf(&p, "%s (%p) topology count > > > > > > > > > > > > > > reached > > > > > > > > > > > > > > 0, > > > > > > > > > > > > > > dumping history:\n", > > > > > > > > > > > > > > - type_str, ptr); > > > > > > > > > > > > > > + if (history->type == > > > > > > > > > > > > > > DRM_DP_MST_TOPOLOGY_HISTORY_PORT) > > > > > > > > > > > > > > +{ > > > > > > > > > > > > > > + struct drm_dp_mst_port *port = ptr; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + if (port->connector) > > > > > > > > > > > > > > + connector_name = > > > > > > > > > > > > > > +port->connector->name; > > > > > > > > > > > > > > + } > > > > > > > > > > > > > > + if (connector_name) > > > > > > > > > > > > > > + drm_printf(&p, "Port %s (%p) > > > > > > > > > > > > > > topology > > > > > > > > > > > > > > ref > > > > > > > > > > > > > > +history:\n", > > > > > > > > > > > > > > connector_name, ptr); > > > > > > > > > > > > > > + else > > > > > > > > > > > > > > + drm_printf(&p, "%s (%p) topology ref > > > > > > > > > > > > > > +history:\n", > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +topology_ref_history_type_to_str(history->type), > > > > > > > > > > > > > > ptr); > > > > > > > > > > > > > > > > > > > > > > > > > > > > for (i = 0; i < history->len; i++) { > > > > > > > > > > > > > > const struct > > > > > > > > > > > > > > drm_dp_mst_topology_ref_entry > > > > > > > > > > > > > > *entry = @@ > > > > > > > > > > > > > > - > > > > > > > > > > > > > > 1682,24 +1688,92 @@ > > > > > > > > > > > > > > __dump_topology_ref_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_ref_history *history, > > > > > > > > > > > > > > ts_nsec, rem_nsec / 1000, > > > > > > > > > > > > > > buf); > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > - /* Now free the history, since this is the > > > > > > > > > > > > > > only > > > > > > > > > > > > > > time > > > > > > > > > > > > > > we expose it */ > > > > > > > > > > > > > > - kfree(history->entries); > > > > > > > > > > > > > > out: > > > > > > > > > > > > > > kfree(buf); > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > > > > + * drm_dp_mst_dump_topology_refs - helper > > > > > > > > > > > > > > function > > > > > > > > > > > > > > for > > > > > > > > > > > > > > +dumping the topology ref history > > > > > > > > > > > > > > + * @m: File to print to > > > > > > > > > > > > > > + * @mgr: &struct drm_dp_mst_topology_mgr to use > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > + * Prints the topology ref history of all ports > > > > > > > > > > > > > > and > > > > > > > > > > > > > > MSTBs > > > > > > > > > > > > > > +on @mgr that are still in memory, > > > > > > > > > > > > > > + * regardless of whether they're actually still > > > > > > > > > > > > > > reachable > > > > > > > > > > > > > > +through the topology or not. Only enabled > > > > > > > > > > > > > > + * when %CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS is > > > > > > > > > > > > > > enabled. > > > > > > > > > > > > > > +Can be implemented by drivers to assist > > > > > > > > > > > > > > + * with debugging leaks in the DP MST helpers. > > > > > > > > > > > > > > + */ > > > > > > > > > > > > > > +void drm_dp_mst_dump_topology_refs(struct > > > > > > > > > > > > > > seq_file > > > > > > > > > > > > > > *m, > > > > > > > > > > > > > > +struct drm_dp_mst_topology_mgr *mgr) { > > > > > > > > > > > > > > + struct drm_dp_mst_topology_ref_history > > > > > > > > > > > > > > *history; > > > > > > > > > > > > > > + struct drm_printer p = > > > > > > > > > > > > > > drm_seq_file_printer(m); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + mutex_lock(&mgr->topology_ref_history_lock); > > > > > > > > > > > > > > + list_for_each_entry(history, > > > > > > > > > > > > > > +&mgr->topology_ref_history_list, > > > > > > > > > > > > > > +node) > > > > > > > > > > > > > > + dump_topology_ref_history(history, > > > > > > > > > > > > > > p); > > > > > > > > > > > > > > + mutex_unlock(&mgr- > > > > > > > > > > > > > > >topology_ref_history_lock); > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > +EXPORT_SYMBOL(drm_dp_mst_dump_topology_refs); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static void > > > > > > > > > > > > > > +__init_topology_ref_history(struct > > > > > > > > > > > > > > +drm_dp_mst_topology_ref_history > > > > > > > > > > > > > > *history, > > > > > > > > > > > > > > + struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, > > > > > > > > > > > > > > + enum > > > > > > > > > > > > > > +drm_dp_mst_topology_history_type > > > > > > > > > > > > > > +type) { > > > > > > > > > > > > > > + history->type = type; > > > > > > > > > > > > > > + INIT_LIST_HEAD(&history->node); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + mutex_lock(&mgr->topology_ref_history_lock); > > > > > > > > > > > > > > + list_add(&history->node, > > > > > > > > > > > > > > +&mgr->topology_ref_history_list); > > > > > > > > > > > > > > + mutex_unlock(&mgr- > > > > > > > > > > > > > > >topology_ref_history_lock); > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static void > > > > > > > > > > > > > > +__destroy_topology_ref_history(struct > > > > > > > > > > > > > > +drm_dp_mst_topology_ref_history > > > > > > > > > > > > > > *history, > > > > > > > > > > > > > > + struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr) { > > > > > > > > > > > > > > + mutex_lock(&mgr->topology_ref_history_lock); > > > > > > > > > > > > > > + list_del(&mgr->topology_ref_history_list); > > > > > > > > > > > > > > + mutex_unlock(&mgr- > > > > > > > > > > > > > > >topology_ref_history_lock); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + kfree(history->entries); } > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static __always_inline void > > > > > > > > > > > > > > +init_port_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, struct drm_dp_mst_port *port) { > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +__init_topology_ref_history(&port- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +mgr, > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +DRM_DP_MST_TOPOLOGY_HISTORY_PORT); > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static __always_inline void > > > > > > > > > > > > > > +init_mstb_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, struct drm_dp_mst_branch *mstb) { > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +__init_topology_ref_history(&mstb- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +mgr, > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +DRM_DP_MST_TOPOLOGY_HISTORY_MSTB); > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static __always_inline void > > > > > > > > > > > > > > +destroy_port_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_port > > > > > > > > > > > > > > *port) > > > > > > > > > > > > > > +{ > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +__destroy_topology_ref_history(&port- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +port->mgr); } > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +static __always_inline void > > > > > > > > > > > > > > +destroy_mstb_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > > > > > +*mstb) { > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +__destroy_topology_ref_history(&mstb- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +mstb->mgr); } > > > > > > > > > > > > > > + > > > > > > > > > > > > > > static __always_inline void > > > > > > > > > > > > > > dump_mstb_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > > > > > *mstb) > > > > > > > > > > > > > > { > > > > > > > > > > > > > > - > > > > > > > > > > > > > > __dump_topology_ref_history(&mstb- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > mstb, > > > > > > > > > > > > > > - "MSTB"); > > > > > > > > > > > > > > + dump_topology_ref_history(&mstb- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +drm_debug_printer(DBG_PREFIX)); > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > static __always_inline void > > > > > > > > > > > > > > dump_port_topology_history(struct drm_dp_mst_port > > > > > > > > > > > > > > *port) > > > > > > > > > > > > > > { > > > > > > > > > > > > > > - > > > > > > > > > > > > > > __dump_topology_ref_history(&port- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > port, > > > > > > > > > > > > > > - "Port"); > > > > > > > > > > > > > > + dump_topology_ref_history(&port- > > > > > > > > > > > > > > > topology_ref_history, > > > > > > > > > > > > > > +drm_debug_printer(DBG_PREFIX)); > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > static __always_inline void @@ -1729,6 +1803,14 > > > > > > > > > > > > > > @@ > > > > > > > > > > > > > > topology_ref_history_unlock(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr) } #else static > > > > > > > > > > > > > > inline > > > > > > > > > > > > > > void > > > > > > > > > > > > > > +init_port_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, struct drm_dp_mst_port *port); static > > > > > > > > > > > > > > inline > > > > > > > > > > > > > > void > > > > > > > > > > > > > Should also add the bracket, otherwise will get > > > > > > > > > > > > > warnings. > > > > > > > > > > > > > => static inline void > > > > > > > > > > > > > init_port_topology_history(struct > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port > > > > > > > > > > > > > *port) > > > > > > > > > > > > > {}; > > > > > > > > > > > > > > > > > > > > > > > > > > > +init_mstb_topology_history(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, struct drm_dp_mst_branch *mstb); static > > > > > > > > > > > > > > inline > > > > > > > > > > > > > > void > > > > > > > > > > > > > Same as above > > > > > > > > > > > > > => static inline void > > > > > > > > > > > > > init_mstb_topology_history(struct > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr, struct > > > > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > > > > *mstb) > > > > > > > > > > > > > {}; > > > > > > > > > > > > > > > > > > > > > > > > > > > +destroy_port_topology_history(struct > > > > > > > > > > > > > > +drm_dp_mst_topology_mgr *mgr, struct > > > > > > > > > > > > > > drm_dp_mst_port > > > > > > > > > > > > > > +*port); static inline void > > > > > > > > > > > > > destroy_port_topology_history() takes one parameter > > > > > > > > > > > > > only > > > > > > > > > > > > > which > > > > > > > > > > > > > is port. > > > > > > > > > > > > > => destroy_port_topology_history(struct > > > > > > > > > > > > > drm_dp_mst_port > > > > > > > > > > > > > *port) > > > > > > > > > > > > > {}; > > > > > > > > > > > > > > > > > > > > > > > > > > > +destroy_mstb_topology_history(struct > > > > > > > > > > > > > > +drm_dp_mst_topology_mgr *mgr, struct > > > > > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > > > > > +*mstb); static inline void > > > > > > > > > > > > > destroy_mstb_topology_history() takes one parameter > > > > > > > > > > > > > only > > > > > > > > > > > > > which > > > > > > > > > > > > > is mstb => destroy_mstb_topology_history(struct > > > > > > > > > > > > > drm_dp_mst_branch > > > > > > > > > > > > > *mstb) {}; > > > > > > > > > > > > > > > > > > > > > > > > > > > topology_ref_history_lock(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > *mgr) {} static inline void > > > > > > > > > > > > > > topology_ref_history_unlock(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr) {} @@ -1740,6 > > > > > > > > > > > > > > +1822,25 > > > > > > > > > > > > > > @@ > > > > > > > > > > > > > > dump_port_topology_history(struct drm_dp_mst_port > > > > > > > > > > > > > > *port) > > > > > > > > > > > > > > {} > > > > > > > > > > > > > > #define save_port_topology_ref(port, type) #endif > > > > > > > > > > > > > > > > > > > > > > > > > > > > +static struct drm_dp_mst_branch * > > > > > > > > > > > > > > +drm_dp_add_mst_branch_device(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr > > > > > > > > > > > > > > +*mgr, > > > > > > > > > > > > > > +u8 lct, u8 *rad) { > > > > > > > > > > > > > > + struct drm_dp_mst_branch *mstb; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + mstb = kzalloc(sizeof(*mstb), GFP_KERNEL); > > > > > > > > > > > > > > + if (!mstb) > > > > > > > > > > > > > > + return NULL; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + mstb->lct = lct; > > > > > > > > > > > > > > + if (lct > 1) > > > > > > > > > > > > > > + memcpy(mstb->rad, rad, lct / 2); > > > > > > > > > > > > > > + INIT_LIST_HEAD(&mstb->ports); > > > > > > > > > > > > > > + kref_init(&mstb->topology_kref); > > > > > > > > > > > > > > + kref_init(&mstb->malloc_kref); > > > > > > > > > > > > > > + init_mstb_topology_history(mgr, mstb); > > > > > > > > > > > > > > + return mstb; > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > + > > > > > > > > > > > > > > static void > > > > > > > > > > > > > > drm_dp_destroy_mst_branch_device(struct > > > > > > > > > > > > > > kref > > > > > > > > > > > > > > *kref) { > > > > > > > > > > > > > > struct drm_dp_mst_branch *mstb = @@ -1747,6 > > > > > > > > > > > > > > +1848,7 > > > > > > > > > > > > > > @@ > > > > > > > > > > > > > > static void > > > > > > > > > > > > > > drm_dp_destroy_mst_branch_device(struct > > > > > > > > > > > > > > kref *kref) > > > > > > > > > > > > > > struct drm_dp_mst_topology_mgr *mgr = mstb- > > > > > > > > > > > > > > >mgr; > > > > > > > > > > > > > > > > > > > > > > > > > > > > dump_mstb_topology_history(mstb); > > > > > > > > > > > > > > + destroy_mstb_topology_history(mstb); > > > > > > > > > > > > > > > > > > > > > > > > > > > > INIT_LIST_HEAD(&mstb->destroy_next); > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1856,6 +1958,7 @@ static void > > > > > > > > > > > > > > drm_dp_destroy_port(struct > > > > > > > > > > > > > > kref > > > > > > > > > > > > > > *kref) > > > > > > > > > > > > > > struct drm_dp_mst_topology_mgr *mgr = port- > > > > > > > > > > > > > > >mgr; > > > > > > > > > > > > > > > > > > > > > > > > > > > > dump_port_topology_history(port); > > > > > > > > > > > > > > + destroy_port_topology_history(port); > > > > > > > > > > > > > > > > > > > > > > > > > > > > /* There's nothing that needs locking to > > > > > > > > > > > > > > destroy > > > > > > > > > > > > > > an > > > > > > > > > > > > > > input port yet */ > > > > > > > > > > > > > > if (port->input) { > > > > > > > > > > > > > > @@ -2135,7 +2238,7 @@ drm_dp_port_set_pdt(struct > > > > > > > > > > > > > > drm_dp_mst_port *port, u8 new_pdt, > > > > > > > > > > > > > > ret = > > > > > > > > > > > > > > drm_dp_mst_register_i2c_bus(port); > > > > > > > > > > > > > > } else { > > > > > > > > > > > > > > lct = > > > > > > > > > > > > > > drm_dp_calculate_rad(port, > > > > > > > > > > > > > > rad); > > > > > > > > > > > > > > - mstb = > > > > > > > > > > > > > > drm_dp_add_mst_branch_device(lct, rad); > > > > > > > > > > > > > > + mstb = > > > > > > > > > > > > > > +drm_dp_add_mst_branch_device(mgr, lct, rad); > > > > > > > > > > > > > > if (!mstb) { > > > > > > > > > > > > > > ret = -ENOMEM; > > > > > > > > > > > > > > drm_err(mgr->dev, > > > > > > > > > > > > > > "Failed > > > > > > > > > > > > > > to > > > > > > > > > > > > > > create MSTB for port %p", port); @@ -2366,6 > > > > > > > > > > > > > > +2469,8 @@ > > > > > > > > > > > > > > drm_dp_mst_add_port(struct drm_device *dev, > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > drm_dp_mst_get_mstb_malloc(mstb); > > > > > > > > > > > > > > > > > > > > > > > > > > > > + init_port_topology_history(mgr, port); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > return port; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -3745,7 +3850,7 @@ int > > > > > > > > > > > > > > drm_dp_mst_topology_mgr_set_mst(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr, bool ms > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > /* add initial branch device at LCT > > > > > > > > > > > > > > 1 */ > > > > > > > > > > > > > > - mstb = > > > > > > > > > > > > > > drm_dp_add_mst_branch_device(1, > > > > > > > > > > > > > > NULL); > > > > > > > > > > > > > > + mstb = > > > > > > > > > > > > > > drm_dp_add_mst_branch_device(mgr, > > > > > > > > > > > > > > 1, > > > > > > > > > > > > > > +NULL); > > > > > > > > > > > > > > if (mstb == NULL) { > > > > > > > > > > > > > > ret = -ENOMEM; > > > > > > > > > > > > > > goto out_unlock; @@ -5512,14 > > > > > > > > > > > > > > +5617,16 > > > > > > > > > > > > > > @@ int drm_dp_mst_topology_mgr_init(struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr *mgr, > > > > > > > > > > > > > > mutex_init(&mgr->delayed_destroy_lock); > > > > > > > > > > > > > > mutex_init(&mgr->up_req_lock); > > > > > > > > > > > > > > mutex_init(&mgr->probe_lock); -#if > > > > > > > > > > > > > > IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) > > > > > > > > > > > > > > - mutex_init(&mgr->topology_ref_history_lock); > > > > > > > > > > > > > > -#endif > > > > > > > > > > > > > > INIT_LIST_HEAD(&mgr->tx_msg_downq); > > > > > > > > > > > > > > INIT_LIST_HEAD(&mgr->destroy_port_list); > > > > > > > > > > > > > > INIT_LIST_HEAD(&mgr- > > > > > > > > > > > > > > > destroy_branch_device_list); > > > > > > > > > > > > > > INIT_LIST_HEAD(&mgr->up_req_list); > > > > > > > > > > > > > > > > > > > > > > > > > > > > +#if > > > > > > > > > > > > > > IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) > > > > > > > > > > > > > > + mutex_init(&mgr->topology_ref_history_lock); > > > > > > > > > > > > > > + INIT_LIST_HEAD(&mgr- > > > > > > > > > > > > > > >topology_ref_history_list); > > > > > > > > > > > > > > +#endif > > > > > > > > > > > > > > + > > > > > > > > > > > > > > /* > > > > > > > > > > > > > > * delayed_destroy_work will be queued on a > > > > > > > > > > > > > > dedicated > > > > > > > > > > > > > > WQ, so that any > > > > > > > > > > > > > > * requeuing will be also flushed when > > > > > > > > > > > > > > deiniting > > > > > > > > > > > > > > the > > > > > > > > > > > > > > topology manager. > > > > > > > > > > > > > > diff --git > > > > > > > > > > > > > > a/drivers/gpu/drm/nouveau/nouveau_debugfs.c > > > > > > > > > > > > > > b/drivers/gpu/drm/nouveau/nouveau_debugfs.c > > > > > > > > > > > > > > index 1cbe01048b93..53ec7c1daada 100644 > > > > > > > > > > > > > > --- a/drivers/gpu/drm/nouveau/nouveau_debugfs.c > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/nouveau/nouveau_debugfs.c > > > > > > > > > > > > > > @@ -29,9 +29,13 @@ > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > > > > > > > > #include <linux/debugfs.h> > > > > > > > > > > > > > > +#include <drm/drm_dp_mst_helper.h> #include > > > > > > > > > > > > > > +<drm/drm_encoder.h> > > > > > > > > > > > > > > #include <nvif/class.h> > > > > > > > > > > > > > > #include <nvif/if0001.h> > > > > > > > > > > > > > > +#include <nouveau_encoder.h> > > > > > > > > > > > > > > #include "nouveau_debugfs.h" > > > > > > > > > > > > > > +#include "nouveau_display.h" > > > > > > > > > > > > > > #include "nouveau_drv.h" > > > > > > > > > > > > > > > > > > > > > > > > > > > > static int > > > > > > > > > > > > > > @@ -68,6 +72,34 @@ > > > > > > > > > > > > > > nouveau_debugfs_strap_peek(struct > > > > > > > > > > > > > > seq_file *m, void > > > > > > > > > > > > > > *data) > > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > +#ifdef CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > > > > > > > > > > > > +static int > > > > > > > > > > > > > > nouveau_debugfs_mst_topology_history(struct > > > > > > > > > > > > > > +seq_file *m, void *data) { > > > > > > > > > > > > > > + struct drm_info_node *node = m->private; > > > > > > > > > > > > > > + struct drm_device *dev = node->minor->dev; > > > > > > > > > > > > > > + struct drm_encoder *encoder; > > > > > > > > > > > > > > + struct nouveau_display *disp = > > > > > > > > > > > > > > nouveau_display(dev); > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + if (disp->disp.object.oclass < GF110_DISP) > > > > > > > > > > > > > > + return -EINVAL; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + drm_for_each_encoder(encoder, dev) { > > > > > > > > > > > > > > + struct nv50_mstm *mstm; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + /* We need the real encoder for each > > > > > > > > > > > > > > MST > > > > > > > > > > > > > > mgr > > > > > > > > > > > > > > +*/ > > > > > > > > > > > > > > + if (encoder->encoder_type != > > > > > > > > > > > > > > +DRM_MODE_ENCODER_TMDS) > > > > > > > > > > > > > > + continue; > > > > > > > > > > > > > > + mstm = nouveau_encoder(encoder)- > > > > > > > > > > > > > > > dp.mstm; > > > > > > > > > > > > > > + if (!mstm) > > > > > > > > > > > > > > + continue; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + seq_printf(m, "MSTM on %s:\n", > > > > > > > > > > > > > > encoder- > > > > > > > > > > > > > > > name); > > > > > > > > > > > > > > + drm_dp_mst_dump_topology_refs(m, > > > > > > > > > > > > > > &mstm- > > > > > > > > > > > > > > > mgr); > > > > > > > > > > > > > > + } > > > > > > > > > > > > > > + return 0; > > > > > > > > > > > > > > +} > > > > > > > > > > > > > > +#endif > > > > > > > > > > > > > > + > > > > > > > > > > > > > > static int > > > > > > > > > > > > > > nouveau_debugfs_pstate_get(struct seq_file *m, > > > > > > > > > > > > > > void > > > > > > > > > > > > > > *data) > > > > > > > > > > > > > > { @@ > > > > > > > > > > > > > > -213,6 > > > > > > > > > > > > > > +245,9 @@ static const struct file_operations > > > > > > > > > > > > > > nouveau_pstate_fops = { static struct > > > > > > > > > > > > > > drm_info_list > > > > > > > > > > > > > > nouveau_debugfs_list[] = { > > > > > > > > > > > > > > { "vbios.rom", nouveau_debugfs_vbios_image, > > > > > > > > > > > > > > 0, > > > > > > > > > > > > > > NULL > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > { "strap_peek", nouveau_debugfs_strap_peek, > > > > > > > > > > > > > > 0, > > > > > > > > > > > > > > NULL > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > +#ifdef CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > > > > > > > > > > > > + { "dp_mst_topology_refs", > > > > > > > > > > > > > > +nouveau_debugfs_mst_topology_history, 0, NULL }, > > > > > > > > > > > > > > #endif > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > #define NOUVEAU_DEBUGFS_ENTRIES > > > > > > > > > > > > > > ARRAY_SIZE(nouveau_debugfs_list) > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/include/drm/drm_dp_mst_helper.h > > > > > > > > > > > > > > b/include/drm/drm_dp_mst_helper.h index > > > > > > > > > > > > > > 78044ac5b59b..e3a73d8d97c0 > > > > > > > > > > > > > > 100644 > > > > > > > > > > > > > > --- a/include/drm/drm_dp_mst_helper.h > > > > > > > > > > > > > > +++ b/include/drm/drm_dp_mst_helper.h > > > > > > > > > > > > > > @@ -35,6 +35,11 @@ enum > > > > > > > > > > > > > > drm_dp_mst_topology_ref_type { > > > > > > > > > > > > > > DRM_DP_MST_TOPOLOGY_REF_PUT, > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > > > > > +enum drm_dp_mst_topology_history_type { > > > > > > > > > > > > > > + DRM_DP_MST_TOPOLOGY_HISTORY_PORT, > > > > > > > > > > > > > > + DRM_DP_MST_TOPOLOGY_HISTORY_MSTB, }; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > struct drm_dp_mst_topology_ref_history { > > > > > > > > > > > > > > struct drm_dp_mst_topology_ref_entry { > > > > > > > > > > > > > > enum drm_dp_mst_topology_ref_type > > > > > > > > > > > > > > type; > > > > > > > > > > > > > > @@ > > > > > > > > > > > > > > -43,6 > > > > > > > > > > > > > > +48,9 @@ struct drm_dp_mst_topology_ref_history { > > > > > > > > > > > > > > depot_stack_handle_t backtrace; > > > > > > > > > > > > > > } *entries; > > > > > > > > > > > > > > int len; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + enum drm_dp_mst_topology_history_type type; > > > > > > > > > > > > > > + struct list_head node; > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > #endif /* > > > > > > > > > > > > > > IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -769,6 +777,12 @@ struct > > > > > > > > > > > > > > drm_dp_mst_topology_mgr { > > > > > > > > > > > > > > * &drm_dp_mst_branch.topology_ref_history. > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > struct mutex topology_ref_history_lock; > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + /** > > > > > > > > > > > > > > + * @topology_ref_history_list: contains a > > > > > > > > > > > > > > list > > > > > > > > > > > > > > of > > > > > > > > > > > > > > +topology ref > > > > > > > > > > > > > > histories for any MSTBs or > > > > > > > > > > > > > > + * ports that are still in memory > > > > > > > > > > > > > > + */ > > > > > > > > > > > > > > + struct list_head topology_ref_history_list; > > > > > > > > > > > > > > #endif > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -873,6 +887,10 @@ void > > > > > > > > > > > > > > drm_dp_mst_put_port_malloc(struct > > > > > > > > > > > > > > drm_dp_mst_port *port); > > > > > > > > > > > > > > > > > > > > > > > > > > > > struct drm_dp_aux > > > > > > > > > > > > > > *drm_dp_mst_dsc_aux_for_port(struct > > > > > > > > > > > > > > drm_dp_mst_port *port); > > > > > > > > > > > > > > > > > > > > > > > > > > > > +#ifdef CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > > > > > > > > > > > > +void drm_dp_mst_dump_topology_refs(struct > > > > > > > > > > > > > > seq_file > > > > > > > > > > > > > > *m, > > > > > > > > > > > > > > +struct drm_dp_mst_topology_mgr *mgr); #endif > > > > > > > > > > > > > > + > > > > > > > > > > > > > > extern const struct drm_private_state_funcs > > > > > > > > > > > > > > drm_dp_mst_topology_state_funcs; > > > > > > > > > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Lyude Paul (she/her) > > > > > > > > > > > > Software Engineer at Red Hat > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Cheers, Lyude Paul (she/her) Software Engineer at Red Hat > > > > > > > -- > > > > > > > Regards, > > > > > > > Wayne Lin > > > > > > > > > > > > > > > > > > > -- > > > > > > Cheers, > > > > > > Lyude Paul (she/her) > > > > > > Software Engineer at Red Hat > > > > > > > > > > > > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat

2 4

AMD display drivers handling DRM CRTC color mgmt props
by Melissa Wen 05 May '22

05 May '22

Hi all, I'm examining how DRM color management properties (degamma, ctm, gamma) are applied to AMD display drivers. As far I could understand thanks Nicholas documentation on amdgpu_dm/amdgpu_dm_color, DC drivers have per-plane color correction features: * - Input gamma LUT (de-normalized) * - Input CSC (normalized) * - Surface degamma LUT (normalized) * - Surface CSC (normalized) * - Surface regamma LUT (normalized) * - Output CSC (normalized) so DM is "adapting" those DRM per-CRTC properties to fit into three of these color correction stages, which I guess are the surface stages: * - Surface degamma LUT (normalized) * - Surface CSC (normalized) * - Surface regamma LUT (normalized) I'm trying to understand what this mapping is doing. A comment mentions that is not possible to do these color corrections after blending, so, the same color correction pipe is performed on every plane before blending? (is the surface the plane?) Does this adaptation affect the expected output? Moreover, is there something that I misunderstood? :) That said, if the DRM color mgmt supports per-CRTC 3D LUT as the last step of color correction, I don't see how to accommodate it in the mapping above, but I see DC already supports programming 3D LUT on DPP. Once DRM has the 3D LUT interface and DM mapped it as a DPP property, the 3D LUT will be at the end of the color correction pipeline? Is there anything I need to worry about mapping DRM 3D LUT support? Or any advice? Thanks in advance, Melissa

2 6

[PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()
by Douglas Anderson 05 May '22

05 May '22

The drm_helper_probe_single_connector_modes() is a bit long. Let's break a chunk off to update and validate modes. This helps avoid one goto and also will allow us to more easily call the helper a second time in a future patch without adding looping or another goto. This change is intended to be a no-op change--just code movement. Signed-off-by: Douglas Anderson <dianders(a)chromium.org> --- drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++------------- 1 file changed, 59 insertions(+), 46 deletions(-) diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 682359512996..819225629010 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector, } EXPORT_SYMBOL(drm_helper_probe_detect); +static bool _drm_helper_update_and_validate(struct drm_connector *connector, + uint32_t maxX, uint32_t maxY, + struct drm_modeset_acquire_ctx *ctx) +{ + struct drm_device *dev = connector->dev; + struct drm_display_mode *mode; + int mode_flags = 0; + int ret; + + drm_connector_list_update(connector); + + if (connector->interlace_allowed) + mode_flags |= DRM_MODE_FLAG_INTERLACE; + if (connector->doublescan_allowed) + mode_flags |= DRM_MODE_FLAG_DBLSCAN; + if (connector->stereo_allowed) + mode_flags |= DRM_MODE_FLAG_3D_MASK; + + list_for_each_entry(mode, &connector->modes, head) { + if (mode->status != MODE_OK) + continue; + + mode->status = drm_mode_validate_driver(dev, mode); + if (mode->status != MODE_OK) + continue; + + mode->status = drm_mode_validate_size(mode, maxX, maxY); + if (mode->status != MODE_OK) + continue; + + mode->status = drm_mode_validate_flag(mode, mode_flags); + if (mode->status != MODE_OK) + continue; + + ret = drm_mode_validate_pipeline(mode, connector, ctx, + &mode->status); + if (ret) { + drm_dbg_kms(dev, + "drm_mode_validate_pipeline failed: %d\n", + ret); + + if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) + mode->status = MODE_ERROR; + else + return true; + } + + if (mode->status != MODE_OK) + continue; + mode->status = drm_mode_validate_ycbcr420(mode, connector); + } + + return false; +} + /** * drm_helper_probe_single_connector_modes - get complete set of display modes * @connector: connector to probe @@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private; int count = 0, ret; - int mode_flags = 0; bool verbose_prune = true; enum drm_connector_status old_status; struct drm_modeset_acquire_ctx ctx; @@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, connector->status == connector_status_unknown)) count = drm_add_modes_noedid(connector, 1024, 768); count += drm_helper_probe_add_cmdline_mode(connector); - if (count == 0) - goto prune; - - drm_connector_list_update(connector); - - if (connector->interlace_allowed) - mode_flags |= DRM_MODE_FLAG_INTERLACE; - if (connector->doublescan_allowed) - mode_flags |= DRM_MODE_FLAG_DBLSCAN; - if (connector->stereo_allowed) - mode_flags |= DRM_MODE_FLAG_3D_MASK; - - list_for_each_entry(mode, &connector->modes, head) { - if (mode->status != MODE_OK) - continue; - - mode->status = drm_mode_validate_driver(dev, mode); - if (mode->status != MODE_OK) - continue; - - mode->status = drm_mode_validate_size(mode, maxX, maxY); - if (mode->status != MODE_OK) - continue; - - mode->status = drm_mode_validate_flag(mode, mode_flags); - if (mode->status != MODE_OK) - continue; - - ret = drm_mode_validate_pipeline(mode, connector, &ctx, - &mode->status); - if (ret) { - drm_dbg_kms(dev, - "drm_mode_validate_pipeline failed: %d\n", - ret); - - if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) { - mode->status = MODE_ERROR; - } else { - drm_modeset_backoff(&ctx); - goto retry; - } + if (count != 0) { + if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) { + drm_modeset_backoff(&ctx); + goto retry; } - - if (mode->status != MODE_OK) - continue; - mode->status = drm_mode_validate_ycbcr420(mode, connector); } prune: -- 2.36.0.rc2.479.g8af0fa9b8e-goog

6 16

[PATCH 1/4] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb v2
by Christian König 05 May '22

05 May '22

drm_gem_plane_helper_prepare_fb() was using drm_atomic_set_fence_for_plane() which ignores all implicit fences when an explicit fence is already set. That's rather unfortunate when the fb still has a kernel fence we need to wait for to avoid presenting garbage on the screen. So instead update the fence in the plane state directly. While at it also take care of all potential GEM objects and not just the first one. Also remove the now unused drm_atomic_set_fence_for_plane() function, new drivers should probably use the atomic helpers directly. v2: improve kerneldoc, use local variable and num_planes, WARN_ON_ONCE on missing planes. Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> (v1) Acked-by: Thomas Zimmermann <tzimmermann(a)suse.de> --- drivers/gpu/drm/drm_atomic_uapi.c | 47 ++-------------- drivers/gpu/drm/drm_gem_atomic_helper.c | 73 +++++++++++++++++++------ include/drm/drm_atomic_uapi.h | 2 - include/drm/drm_plane.h | 4 +- 4 files changed, 62 insertions(+), 64 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index c6394ba13b24..434f3d4cb8a2 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, } EXPORT_SYMBOL(drm_atomic_set_fb_for_plane); -/** - * drm_atomic_set_fence_for_plane - set fence for plane - * @plane_state: atomic state object for the plane - * @fence: dma_fence to use for the plane - * - * Helper to setup the plane_state fence in case it is not set yet. - * By using this drivers doesn't need to worry if the user choose - * implicit or explicit fencing. - * - * This function will not set the fence to the state if it was set - * via explicit fencing interfaces on the atomic ioctl. In that case it will - * drop the reference to the fence as we are not storing it anywhere. - * Otherwise, if &drm_plane_state.fence is not set this function we just set it - * with the received implicit fence. In both cases this function consumes a - * reference for @fence. - * - * This way explicit fencing can be used to overrule implicit fencing, which is - * important to make explicit fencing use-cases work: One example is using one - * buffer for 2 screens with different refresh rates. Implicit fencing will - * clamp rendering to the refresh rate of the slower screen, whereas explicit - * fence allows 2 independent render and display loops on a single buffer. If a - * driver allows obeys both implicit and explicit fences for plane updates, then - * it will break all the benefits of explicit fencing. - */ -void -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state, - struct dma_fence *fence) -{ - if (plane_state->fence) { - dma_fence_put(fence); - return; - } - - plane_state->fence = fence; -} -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane); - /** * drm_atomic_set_crtc_for_connector - set CRTC for connector * @conn_state: atomic state object for the connector @@ -1077,10 +1040,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state, * * As a contrast, with implicit fencing the kernel keeps track of any * ongoing rendering, and automatically ensures that the atomic update waits - * for any pending rendering to complete. For shared buffers represented with - * a &struct dma_buf this is tracked in &struct dma_resv. - * Implicit syncing is how Linux traditionally worked (e.g. DRI2/3 on X.org), - * whereas explicit fencing is what Android wants. + * for any pending rendering to complete. This is usually tracked in &struct + * dma_resv which can also contain mandatory kernel fences. Implicit syncing + * is how Linux traditionally worked (e.g. DRI2/3 on X.org), whereas explicit + * fencing is what Android wants. * * "IN_FENCE_FD”: * Use this property to pass a fence that DRM should wait on before @@ -1095,7 +1058,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state, * * On the driver side the fence is stored on the @fence parameter of * &struct drm_plane_state. Drivers which also support implicit fencing - * should set the implicit fence using drm_atomic_set_fence_for_plane(), + * should extract the implicit fence using drm_gem_plane_helper_prepare_fb(), * to make sure there's consistent behaviour between drivers in precedence * of implicit vs. explicit fencing. * diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index a6d89aed0bda..a5026f617739 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include <linux/dma-resv.h> +#include <linux/dma-fence-chain.h> #include <drm/drm_atomic_state_helper.h> #include <drm/drm_atomic_uapi.h> @@ -137,29 +138,67 @@ * * This function is the default implementation for GEM drivers of * &drm_plane_helper_funcs.prepare_fb if no callback is provided. - * - * See drm_atomic_set_fence_for_plane() for a discussion of implicit and - * explicit fencing in atomic modeset updates. */ -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, + struct drm_plane_state *state) { - struct drm_gem_object *obj; - struct dma_fence *fence; + struct dma_fence *fence = dma_fence_get(state->fence); + enum dma_resv_usage usage; + size_t i; int ret; if (!state->fb) return 0; - obj = drm_gem_fb_get_obj(state->fb, 0); - ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence); - if (ret) - return ret; - - /* TODO: drm_atomic_set_fence_for_plane() should be changed to be able - * to handle more fences in general for multiple BOs per fb. + /* + * Only add the kernel fences here if there is already a fence set via + * explicit fencing interfaces on the atomic ioctl. + * + * This way explicit fencing can be used to overrule implicit fencing, + * which is important to make explicit fencing use-cases work: One + * example is using one buffer for 2 screens with different refresh + * rates. Implicit fencing will clamp rendering to the refresh rate of + * the slower screen, whereas explicit fence allows 2 independent + * render and display loops on a single buffer. If a driver allows + * obeys both implicit and explicit fences for plane updates, then it + * will break all the benefits of explicit fencing. */ - drm_atomic_set_fence_for_plane(state, fence); + usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE; + + for (i = 0; i < state->fb->format->num_planes; ++i) { + struct drm_gem_object *obj = drm_gem_fb_get_obj(state->fb, i); + struct dma_fence *new; + + if (WARN_ON_ONCE(!obj)) + continue; + + ret = dma_resv_get_singleton(obj->resv, usage, &new); + if (ret) + goto error; + + if (new && fence) { + struct dma_fence_chain *chain = dma_fence_chain_alloc(); + + if (!chain) { + ret = -ENOMEM; + goto error; + } + + dma_fence_chain_init(chain, fence, new, 1); + fence = &chain->base; + + } else if (new) { + fence = new; + } + } + + dma_fence_put(state->fence); + state->fence = fence; return 0; + +error: + dma_fence_put(fence); + return ret; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); @@ -168,13 +207,13 @@ EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); * @pipe: Simple display pipe * @plane_state: Plane state * - * This function uses drm_gem_plane_helper_prepare_fb() to extract the exclusive fence - * from &drm_gem_object.resv and attaches it to plane state for the atomic + * This function uses drm_gem_plane_helper_prepare_fb() to extract the fences + * from &drm_gem_object.resv and attaches them to the plane state for the atomic * helper to wait on. This is necessary to correctly implement implicit * synchronization for any buffers shared as a struct &dma_buf. Drivers can use * this as their &drm_simple_display_pipe_funcs.prepare_fb callback. * - * See drm_atomic_set_fence_for_plane() for a discussion of implicit and + * See drm_gem_plane_helper_prepare_fb() for a discussion of implicit and * explicit fencing in atomic modeset updates. */ int drm_gem_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe, diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h index 8cec52ad1277..4c6d39d7bdb2 100644 --- a/include/drm/drm_atomic_uapi.h +++ b/include/drm/drm_atomic_uapi.h @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, struct drm_framebuffer *fb); -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state, - struct dma_fence *fence); int __must_check drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, struct drm_crtc *crtc); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 2628c7cde2da..89ea54652e87 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -74,9 +74,7 @@ struct drm_plane_state { * * Optional fence to wait for before scanning out @fb. The core atomic * code will set this when userspace is using explicit fencing. Do not - * write this field directly for a driver's implicit fence, use - * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is - * preserved. + * write this field directly for a driver's implicit fence. * * Drivers should store any implicit fence in this from their * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb() -- 2.25.1

4 7

[PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS
by Jeffrey Hugo 05 May '22

05 May '22

DEFINE_DRM_GEM_FOPS() references drm functions from other headers. For example drm_open() is defined in drm_file.h and drm_ioctl() is defined in drm_ioctl.h. Since drm_gem.h doesn't include these headers, it relies on an implicit include from the .c file to have included these required headers before DEFINE_DRM_GEM_FOPS() gets used. Relying on these implicit includes can cause build failures for new code that doesn't know about these requirements, and can lead to future problems if the headers ever get restructured as there will be a need to update every downstream file that includes drm_gem.h. Lets fix this explicitly including the required headers in drm_gem.h so that code that includes drm_gem.h does not need to worry about these implicit dependencies. Signed-off-by: Jeffrey Hugo <quic_jhugo(a)quicinc.com> --- include/drm/drm_gem.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 9d7c61a..1cbe3d8 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -37,6 +37,8 @@ #include <linux/kref.h> #include <linux/dma-resv.h> +#include <drm/drm_file.h> +#include <drm/drm_ioctl.h> #include <drm/drm_vma_manager.h> struct iosys_map; -- 2.7.4

3 4

[PATCH] drm/nouveau/subdev/bus: Ratelimit logging for fault errors
by Lyude Paul 04 May '22

04 May '22

There's plenty of ways to fudge the GPU when developing on nouveau by mistake, some of which can result in nouveau seriously spamming dmesg with fault errors. This can be somewhat annoying, as it can quickly overrun the message buffer (or your terminal emulator's buffer) and get rid of actually useful feedback from the driver. While working on my new atomic only MST branch, I ran into this issue a couple of times. So, let's fix this by adding nvkm_error_ratelimited(), and using it to ratelimit errors from faults. This should be fine for developers, since it's nearly always only the first few faults that we care about seeing. Plus, you can turn off rate limiting in the kernel if you really need to. Signed-off-by: Lyude Paul <lyude(a)redhat.com> --- drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h | 2 ++ drivers/gpu/drm/nouveau/nvkm/subdev/bus/gf100.c | 14 +++++++------- drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv31.c | 6 +++--- drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv50.c | 6 +++--- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h b/drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h index 1665738948fb..96113c8bee8c 100644 --- a/drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h +++ b/drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h @@ -62,4 +62,6 @@ void nvkm_subdev_intr(struct nvkm_subdev *); #define nvkm_debug(s,f,a...) nvkm_printk((s), DEBUG, info, f, ##a) #define nvkm_trace(s,f,a...) nvkm_printk((s), TRACE, info, f, ##a) #define nvkm_spam(s,f,a...) nvkm_printk((s), SPAM, dbg, f, ##a) + +#define nvkm_error_ratelimited(s,f,a...) nvkm_printk((s), ERROR, err_ratelimited, f, ##a) #endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/gf100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/gf100.c index 53a6651ac225..80b5aaceeaad 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/gf100.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/gf100.c @@ -35,13 +35,13 @@ gf100_bus_intr(struct nvkm_bus *bus) u32 addr = nvkm_rd32(device, 0x009084); u32 data = nvkm_rd32(device, 0x009088); - nvkm_error(subdev, - "MMIO %s of %08x FAULT at %06x [ %s%s%s]\n", - (addr & 0x00000002) ? "write" : "read", data, - (addr & 0x00fffffc), - (stat & 0x00000002) ? "!ENGINE " : "", - (stat & 0x00000004) ? "PRIVRING " : "", - (stat & 0x00000008) ? "TIMEOUT " : ""); + nvkm_error_ratelimited(subdev, + "MMIO %s of %08x FAULT at %06x [ %s%s%s]\n", + (addr & 0x00000002) ? "write" : "read", data, + (addr & 0x00fffffc), + (stat & 0x00000002) ? "!ENGINE " : "", + (stat & 0x00000004) ? "PRIVRING " : "", + (stat & 0x00000008) ? "TIMEOUT " : ""); nvkm_wr32(device, 0x009084, 0x00000000); nvkm_wr32(device, 0x001100, (stat & 0x0000000e)); diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv31.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv31.c index ad8da523bb22..c75e463f3501 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv31.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv31.c @@ -45,9 +45,9 @@ nv31_bus_intr(struct nvkm_bus *bus) u32 addr = nvkm_rd32(device, 0x009084); u32 data = nvkm_rd32(device, 0x009088); - nvkm_error(subdev, "MMIO %s of %08x FAULT at %06x\n", - (addr & 0x00000002) ? "write" : "read", data, - (addr & 0x00fffffc)); + nvkm_error_ratelimited(subdev, "MMIO %s of %08x FAULT at %06x\n", + (addr & 0x00000002) ? "write" : "read", data, + (addr & 0x00fffffc)); stat &= ~0x00000008; nvkm_wr32(device, 0x001100, 0x00000008); diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv50.c index 3a1e45adeedc..2055d0b100d3 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv50.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bus/nv50.c @@ -60,9 +60,9 @@ nv50_bus_intr(struct nvkm_bus *bus) u32 addr = nvkm_rd32(device, 0x009084); u32 data = nvkm_rd32(device, 0x009088); - nvkm_error(subdev, "MMIO %s of %08x FAULT at %06x\n", - (addr & 0x00000002) ? "write" : "read", data, - (addr & 0x00fffffc)); + nvkm_error_ratelimited(subdev, "MMIO %s of %08x FAULT at %06x\n", + (addr & 0x00000002) ? "write" : "read", data, + (addr & 0x00fffffc)); stat &= ~0x00000008; nvkm_wr32(device, 0x001100, 0x00000008); -- 2.35.1

2 7

[PATCH v2] drm/i915: Fix race in __i915_vma_remove_closed
by Karol Herbst 04 May '22

04 May '22

i915_vma_reopen checked if the vma is closed before without taking the lock. So multiple threads could attempt removing the vma. Instead the lock needs to be taken before actually checking. v2: move struct declaration Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: intel-gfx(a)lists.freedesktop.org Cc: dri-devel(a)lists.freedesktop.org Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5732 Signed-off-by: Karol Herbst <kherbst(a)redhat.com> --- drivers/gpu/drm/i915/i915_vma.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 162e8d83691b..2efdad2b43fa 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1615,17 +1615,17 @@ void i915_vma_close(struct i915_vma *vma) static void __i915_vma_remove_closed(struct i915_vma *vma) { - struct intel_gt *gt = vma->vm->gt; - - spin_lock_irq(&gt->closed_lock); list_del_init(&vma->closed_link); - spin_unlock_irq(&gt->closed_lock); } void i915_vma_reopen(struct i915_vma *vma) { + struct intel_gt *gt = vma->vm->gt; + + spin_lock_irq(&gt->closed_lock); if (i915_vma_is_closed(vma)) __i915_vma_remove_closed(vma); + spin_unlock_irq(&gt->closed_lock); } static void force_unbind(struct i915_vma *vma) @@ -1641,6 +1641,7 @@ static void force_unbind(struct i915_vma *vma) static void release_references(struct i915_vma *vma, bool vm_ddestroy) { struct drm_i915_gem_object *obj = vma->obj; + struct intel_gt *gt = vma->vm->gt; GEM_BUG_ON(i915_vma_is_active(vma)); @@ -1651,7 +1652,9 @@ static void release_references(struct i915_vma *vma, bool vm_ddestroy) spin_unlock(&obj->vma.lock); + spin_lock_irq(&gt->closed_lock); __i915_vma_remove_closed(vma); + spin_unlock_irq(&gt->closed_lock); if (vm_ddestroy) i915_vm_resv_put(vma->vm); -- 2.35.1

2 3

[RESEND 1/2] Revert "drm: bridge: mcde_dsi: Drop explicit bridge remove"
by Jagan Teki 04 May '22

04 May '22

commit <3730bc6147b0> ("drm: bridge: mcde_dsi: Drop explicit bridge remove") has removed downstream bridge as it's prior commit <3d7039e1e649> ("drm: bridge: mcde_dsi: Switch to devm_drm_of_get_bridge") added devm_drm_of_get_bridge for looking up if child node has panel or bridge. However commit <b089c0a9b14c> ("Revert "drm: of: Lookup if child node has panel or bridge") has reverted panel or bridge child node lookup from devm_drm_of_get_bridge as it breaks the non-trivial cases the first child node might not be a panel or bridge. So, revert this commit to switch the previous behavior of looking up child panel or bridge. Signed-off-by: Jagan Teki <jagan(a)amarulasolutions.com> --- Notes: Nothing changed for RESEND. drivers/gpu/drm/mcde/mcde_dsi.c | 44 ++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 083a4728654d..5651734ce977 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -19,6 +19,7 @@ #include <drm/drm_mipi_dsi.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_of.h> +#include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> @@ -38,6 +39,7 @@ struct mcde_dsi { struct device *dev; struct mcde *mcde; struct drm_bridge bridge; + struct drm_panel *panel; drivers/gpu/drm/mcde/mcde_dsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 960b49ea2ee5..083a4728654d 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -1122,6 +1122,7 @@ static void mcde_dsi_unbind(struct device *dev, struct device *master, { struct mcde_dsi *d = dev_get_drvdata(dev); + drm_bridge_remove(d->bridge_out); regmap_update_bits(d->prcmu, PRCM_DSI_SW_RESET, PRCM_DSI_SW_RESET_DSI0_SW_RESETN, 0); } -- 2.25.1

3 5

← Newer
1
...
8
9
10
11
12
13
14
...
96
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

dri-devel April 2022