mirror of https://github.com/torvalds/linux.git
153 Commits
| Author | SHA1 | Message | Date |
|---|---|---|---|
Chenwandun
|
68acde7629 |
drm/dp_mst: fix gcc compile error
drivers/gpu/drm/drm_dp_mst_topology.c: In function __topology_ref_save:
drivers/gpu/drm/drm_dp_mst_topology.c:1424:6: error: implicit declaration of function stack_trace_save; did you mean stack_depot_save? [-Werror=implicit-function-declaration]
n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1);
^~~~~~~~~~~~~~~~
stack_depot_save
drivers/gpu/drm/drm_dp_mst_topology.c: In function __dump_topology_ref_history:
drivers/gpu/drm/drm_dp_mst_topology.c:1513:3: error: implicit declaration of function stack_trace_snprint; did you mean acpi_trace_point? [-Werror=implicit-function-declaration]
stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 4);
^~~~~~~~~~~~~~~~~~~
acpi_trace_point
stack_trace_save and stack_trace_snprint are declared in <linux/stacktrace.h>,
so there is need to include it, and <linux/stackdepot.h> is already included
by practices, so just replace <linux/stackdepot.h> by <linux/stacktrace.h>.
Signed-off-by: Chenwandun <chenwandun@huawei.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Link: https://patchwork.freedesktop.org/patch/msgid/1572515029-42087-1-git-send-email-chenwandun@huawei.com
|
|
Dave Airlie
|
57c2af791b |
Merge tag 'topic/mst-suspend-resume-reprobe-2019-10-29-2' of git://anongit.freedesktop.org/drm/drm-misc into drm-next
UAPI Changes: Cross-subsystem Changes: Core Changes: * Handle UP requests asynchronously in the DP MST helpers, fixing hotplug notifications and allowing us to implement suspend/resume reprobing * Add basic suspend/resume reprobing to the DP MST helpers * Improve locking for link address reprobing and connection status request handling in the DP MST helpers * Miscellaneous refactoring in the DP MST helpers * Add a Kconfig option to the DP MST helpers to enable tracking of gets/puts for topology references for debugging purposes Driver Changes: * nouveau: Resume hotplug interrupts earlier, so that sideband messages may be transmitted during resume and thus allow suspend/resume reprobing for DP MST to work * nouveau: Avoid grabbing runtime PM references when handling short DP pulses, so that handling sideband messages in resume codepaths with the DP MST helpers doesn't deadlock us * i915, nouveau, amdgpu, radeon: Use detect_ctx for probing MST connectors, so that we can grab the topology manager's atomic lock Note: there's some amdgpu patches that I didn't realize were pushed upstream already when creating this topic branch. When they fail to apply, you can just ignore and skip them. Signed-off-by: Dave Airlie <airlied@redhat.com> From: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/a74c6446bc960190d195a751cb6d8a00a98f3974.camel@redhat.com |
|
Lyude Paul
|
12a280c728 |
drm/dp_mst: Add topology ref history tracking for debugging
For very subtle mistakes with topology refs, it can be rather difficult to trace them down with the debugging info that we already have. I had one such issue recently while trying to implement suspend/resume reprobing for MST, and ended up coming up with this. Inspired by Chris Wilson's wakeref tracking for i915, this adds a very similar feature to the DP MST helpers, which allows for partial tracking of topology refs for both ports and branch devices. This is a lot less advanced then wakeref tracking: we merely keep a count of all of the spots where a topology ref has been grabbed or dropped, then dump out that history in chronological order when a port or branch device's topology refcount reaches 0. So far, I've found this incredibly useful for debugging topology refcount errors. Since this has the potential to be somewhat slow and loud, we add an expert kernel config option to enable or disable this feature, CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS. Changes since v1: * Don't forget to destroy topology_ref_history_lock Changes since v4: * Correct order of kref_put()/topology_ref_history_unlock - we can't unlock the history after kref_put() since the memory might have been freed by that point * Don't print message on allocation error failures, the kernel already does this for us Changes since v5: * Get rid of some leftover usages of %px * Remove a leftover empty return; statement Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-15-lyude@redhat.com |
|
|
Lyude Paul
|
6f85f73821 |
drm/dp_mst: Add basic topology reprobing when resuming
Finally! For a very long time, our MST helpers have had one very annoying issue: They don't know how to reprobe the topology state when coming out of suspend. This means that if a user has a machine connected to an MST topology and decides to suspend their machine, we lose all topology changes that happened during that period. That can be a big problem if the machine was connected to a different topology on the same port before resuming, as we won't bother reprobing any of the ports and likely cause the user's monitors not to come back up as expected. So, we start fixing this by teaching our MST helpers how to reprobe the link addresses of each connected topology when resuming. As it turns out, the behavior that we want here is identical to the behavior we want when initially probing a newly connected MST topology, with a couple of important differences: - We need to be more careful about handling the potential races between events from the MST hub that could change the topology state as we're performing the link address reprobe - We need to be more careful about handling unlikely state changes on ports - such as an input port turning into an output port, something that would be far more likely to happen in situations like the MST hub we're connected to being changed while we're suspend Both of which have been solved by previous commits. That leaves one requirement: - We need to prune any MST ports in our in-memory topology state that were present when suspending, but have not appeared in the post-resume link address response from their parent branch device Which we can now handle in this commit by modifying drm_dp_send_link_address(). We then introduce suspend/resume reprobing by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology state to indicate that each mstb needs it's link address resent and PBN resources reprobed. On resume, we start back up &mgr->work and have it reprobe the topology in the same way we would on a hotplug, removing any leftover ports that no longer appear in the topology state. Changes since v4: * Split indenting changes in drm_dp_mst_topology_mgr_resume() into a separate patch * Only fire hotplugs when something has actually changed after a link address probe * Don't try to change port->connector at all on ports, just throw out ports that need their connectors removed to make things easier. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-14-lyude@redhat.com |
|
|
Lyude Paul
|
79413ed4a1 |
drm/dp_mst: Lessen indenting in drm_dp_mst_topology_mgr_resume()
Does what it says on the tin. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-9-lyude@redhat.com |
|
|
Lyude Paul
|
dad7d84f88 |
drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
This probably hasn't caused any problems up until now since it's probably nearly impossible to encounter this in the wild, however if we were to receive a connection status notification from the MST hub after resume while we're in the middle of reprobing the link addresses for a topology then there's a much larger chance that a port could have changed from being an output port to input port (or vice versa). If we forget to update this bit of information, we'll potentially ignore a valid PDT change on a downstream port because we think it's an input port. So, make sure we read the input_port field in connection status notifications in drm_dp_mst_handle_conn_stat() to prevent this from happening once we've implemented suspend/resume reprobing. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-8-lyude@redhat.com |
|
|
Lyude Paul
|
3f9b3f02dd |
drm/dp_mst: Protect drm_dp_mst_port members with locking
This is a complicated one. Essentially, there's currently a problem in the MST
core that hasn't really caused any issues that we're aware of (emphasis on "that
we're aware of"): locking.
When we go through and probe the link addresses and path resources in a
topology, we hold no locks when updating ports with said information. The
members I'm referring to in particular are:
- ldps
- ddps
- mcs
- pdt
- dpcd_rev
- num_sdp_streams
- num_sdp_stream_sinks
- available_pbn
- input
- connector
Now that we're handling UP requests asynchronously and will be using some of
the struct members mentioned above in atomic modesetting in the future for
features such as PBN validation, this is going to become a lot more important.
As well, the next few commits that prepare us for and introduce suspend/resume
reprobing will also need clear locking in order to prevent from additional
racing hilarities that we never could have hit in the past.
So, let's solve this issue by using &mgr->base.lock, the modesetting
lock which currently only protects &mgr->base.state. This works
perfectly because it allows us to avoid blocking connection_mutex
unnecessarily, and we can grab this in connector detection paths since
it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this
when updating ports. For drm_dp_mst_handle_link_address_port() things
are a bit more complicated. As I've learned the hard way, we can grab
&mgr->lock.base for everything except for port->connector. See, our
normal driver probing paths end up generating this rather obvious
lockdep chain:
&drm->mode_config.mutex
-> crtc_ww_class_mutex/crtc_ww_class_acquire
-> &connector->mutex
However, sysfs grabs &drm->mode_config.mutex in order to protect itself
from connector state changing under it. Because this entails grabbing
kn->count, e.g. the lock that the kernel provides for protecting sysfs
contexts, we end up grabbing kn->count followed by
&drm->mode_config.mutex. This ends up creating an extremely rude chain:
&kn->count
-> &drm->mode_config.mutex
-> crtc_ww_class_mutex/crtc_ww_class_acquire
-> &connector->mutex
I mean, look at that thing! It's just evil!!! This gross thing ends up
making any calls to drm_connector_register()/drm_connector_unregister()
impossible when holding any kind of modesetting lock. This is annoying
because ideally, we always want to ensure that
drm_dp_mst_port->connector never changes when doing an atomic commit or
check that would affect the atomic topology state so that it can
reliably and easily be used from future DRM DP MST helpers to assist
with tasks such as scanning through the current VCPI allocations and
adding connectors which need to have their allocations updated in
response to a bandwidth change or the like.
Being able to hold &mgr->base.lock throughout the entire link probe
process would have been _great_, since we could prevent userspace from
ever seeing any states in-between individual port changes and as a
result likely end up with a much faster probe and more consistent
results from said probes. But without some rework of how we handle
connector probing in sysfs it's not at all currently possible. In the
future, maybe we can try using the sysfs locks to protect updates to
connector probing state and fix this mess.
So for now, to protect everything other than port->connector under
&mgr->base.lock and ensure that we still have the guarantee that atomic
check/commit contexts will never see port->connector change we use a
silly trick. See: port->connector only needs to change in order to
ensure that input ports (see the MST spec) never have a ghost connector
associated with them. But, there's nothing stopping us from simply
throwing the entire port out and creating a new one in order to maintain
that requirement while still keeping port->connector consistent across
the lifetime of the port in atomic check/commit contexts. For all
intended purposes this works fine, as we validate ports in any contexts
we care about before using them and as such will end up reporting the
connector as disconnected until it's port's destruction finalizes. So,
we just do that in cases where we detect port->input has transitioned
from true->false. We don't need to worry about the other direction,
since a port without a connector isn't visible to userspace and as such
doesn't need to be protected by &mgr->base.lock until we finish
registering a connector for it.
For updating members of drm_dp_mst_port other than port->connector, we
simply grab &mgr->base.lock in drm_dp_mst_link_probe_work() for already
registered ports, update said members and drop the lock before
potentially registering a connector and probing the link address of it's
children.
Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock
acquisition context in order to acquire &mgr->base.lock under
&connection_mutex and convert all it's users over to using the
.detect_ctx probe hooks.
With that, we finally have well defined locking.
Changes since v4:
* Get rid of port->mutex, stop using connection_mutex and just use our own
modesetting lock - mgr->base.lock. Also, add a probe_lock that comes
before this patch.
* Just throw out ports that get changed from an output to an input, and
replace them with new ports. This lets us ensure that modesetting
contexts never see port->connector go from having a connector to being
NULL.
* Write an extremely detailed explanation of what problems this is
trying to fix, since there's a _lot_ of context here and I honestly
forgot some of it myself a couple times.
* Don't grab mgr->lock when reading port->mstb in
drm_dp_mst_handle_link_address_port(). It's not needed.
Cc: Juston Li <juston.li@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Sean Paul <sean@poorly.run>
Signed-off-by: Lyude Paul <lyude@redhat.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-7-lyude@redhat.com
|
|
|
Lyude Paul
|
14692a3637 |
drm/dp_mst: Add probe_lock
Currently, MST lacks locking in a lot of places that really should have some sort of locking. Hotplugging and link address code paths are some of the offenders here, as there is actually nothing preventing us from running a link address probe while at the same time handling a connection status update request - something that's likely always been possible but never seen in the wild because hotplugging has been broken for ages now (with the exception of amdgpu, for reasons I don't think are worth digging into very far). Note: I'm going to start using the term "in-memory topology layout" here to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports. Locking in these places is a little tougher then it looks though. Generally we protect anything having to do with the in-memory topology layout under &mgr->lock. But this becomes nearly impossible to do from the context of link address probes due to the fact that &mgr->lock is usually grabbed under random various modesetting locks, meaning that there's no way we can just invert the &mgr->lock order and keep it locked throughout the whole process of updating the topology. Luckily there are only two workers which can modify the in-memory topology layout: drm_dp_mst_up_req_work() and drm_dp_mst_link_probe_work(), meaning as long as we prevent these two workers from traveling the topology layout in parallel with the intent of updating it we don't need to worry about grabbing &mgr->lock in these workers for reads. We only need to grab &mgr->lock in these workers for writes, so that readers outside these two workers are still protected from the topology layout changing beneath them. So, add the new &mgr->probe_lock and use it in both drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally, add some more detailed explanations for how this locking is intended to work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports. Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Sean Paul <sean@poorly.run> Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-6-lyude@redhat.com |
|
|
Lyude Paul
|
9408cc94eb |
drm/dp_mst: Handle UP requests asynchronously
Once upon a time, hotplugging devices on MST branches actually worked in DRM. Now, it only works in amdgpu (likely because of how it's hotplug handlers are implemented). On both i915 and nouveau, hotplug notifications from MST branches are noticed - but trying to respond to them causes messaging timeouts and causes the whole topology state to go out of sync with reality, usually resulting in the user needing to replug the entire topology in hopes that it actually fixes things. The reason for this is because the way we currently handle UP requests in MST is completely bogus. drm_dp_mst_handle_up_req() is called from drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug handler. Because we handle sending the hotplug event from this function, we actually cause the driver's hotplug handler (and in turn, all sideband transactions) to block on drm_device->mode_config.connection_mutex. This makes it impossible to send any sideband messages from the driver's connector probing functions, resulting in the aforementioned sideband message timeout. There's even more problems with this beyond breaking hotplugging on MST branch devices. It also makes it almost impossible to protect drm_dp_mst_port struct members under a lock because we then have to worry about dealing with all of the lock dependency issues that ensue. So, let's finally actually fix this issue by handling the processing of up requests asyncronously. This way we can send sideband messages from most contexts without having to deal with getting blocked if we hold connection_mutex. This also fixes MST branch device hotplugging on i915, finally! Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-5-lyude@redhat.com |
|
|
Lyude Paul
|
c485e2c97d |
drm/dp_mst: Refactor pdt setup/teardown, add more locking
Since we're going to be implementing suspend/resume reprobing very soon, we need to make sure we are extra careful to ensure that our locking actually protects the topology state where we expect it to. Turns out this isn't the case with drm_dp_port_setup_pdt() and drm_dp_port_teardown_pdt(), both of which change port->mstb without grabbing &mgr->lock. Additionally, since most callers of these functions are just using it to teardown the port's previous PDT and setup a new one we can simplify things a bit and combine drm_dp_port_setup_pdt() and drm_dp_port_teardown_pdt() into a single function: drm_dp_port_set_pdt(). This function also handles actually ensuring that we grab the correct locks when we need to modify port->mstb. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-4-lyude@redhat.com |
|
|
Lyude Paul
|
d29333cf5c |
drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor
This will allow us to add some locking for port->* members, in particular the PDT and ->connector, which can't be done from drm_dp_destroy_port() since we don't know what locks the caller might be holding. Note that we already do this in delayed_destroy_work (renamed from destroy_connector_work in this patch) for ports, we're just making it so mstbs are also destroyed in this worker. Changes since v2: * Clarify commit message Changes since v4: * Clarify commit message more Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-3-lyude@redhat.com |
|
|
Lyude Paul
|
7cb12d4831 |
drm/dp_mst: Destroy MSTBs asynchronously
When reprobing an MST topology during resume, we have to account for the fact that while we were suspended it's possible that mstbs may have been removed from any ports in the topology. Since iterating downwards in the topology requires that we hold &mgr->lock, destroying MSTBs from this context would result in attempting to lock &mgr->lock a second time and deadlocking. So, fix this by first moving destruction of MSTBs into destroy_connector_work, then rename destroy_connector_work and friends to reflect that they now destroy both ports and mstbs. Note that even though this means that MSTBs will still be accessible for a short period of time between their removal from the topology and delayed destruction, we are still protected against referencing a MSTB with a refcount of 0 since we use kref_get_unless_zero() in most places. Changes since v1: * s/destroy_connector_list/destroy_port_list/ s/connector_destroy_lock/delayed_destroy_lock/ s/connector_destroy_work/delayed_destroy_work/ s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/ s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/ - danvet * Use two loops in drm_dp_delayed_destroy_work() - danvet * Better explain why we need to do this - danvet * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't account for work requeing Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel@ffwll.ch> Reviewed-by: Sean Paul <sean@poorly.run> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-2-lyude@redhat.com |
|
Wambui Karuga
|
ddd9b54dee |
drm: remove unnecessary return variable
Remove unnecessary variable `ret` in drm_dp_atomic_find_vcpi_slots()
only used to hold the function return value and have the function
return the value directly.
Issue found by coccinelle:
@@
local idexpression ret;
expression e;
@@
-ret =
+return
e;
-return ret;
Signed-off-by: Wambui Karuga <wambui.karugax@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20191019071840.16877-1-wambui@karuga.xyz
|
|
Lucas De Marchi
|
67c698fc5e |
drm/dp-mst: fix warning on unused var
Fixes:
|
|
Daniel Vetter
|
83fa9842af |
drm/dp-mst: Drop connection_mutex check
Private atomic objects have grown their own locking with
commit
|
|
Jani Nikula
|
f0a8f533ad |
drm/print: add drm_debug_enabled()
Add helper to check if a drm debug category is enabled. Convert drm core to use it. No functional changes. v2: Move unlikely() to drm_debug_enabled() (Eric) v3: Keep unlikely() when combined with other conditions (Eric) Cc: Eric Engestrom <eric@engestrom.ch> Acked-by: Alex Deucher <alexander.deucher@amd.com> Reviewed-by: Eric Engestrom <eric@engestrom.ch> Signed-off-by: Jani Nikula <jani.nikula@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191001140614.26909-1-jani.nikula@intel.com |
|
Ville Syrjälä
|
ed20b7d5c6 |
drm/dp/mst: Replace the fixed point thing with straight calculation
Get rid of the drm_fixp_from_fraction() usage and just do the straightforward calculation directly. Cc: Lyude Paul <lyude@redhat.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190925141442.23236-3-ville.syrjala@linux.intel.com Reviewed-by: Lyude Paul <lyude@redhat.com> |
|
|
Ville Syrjälä
|
0c3bb15cfc |
drm/dp/mst: Handle arbitrary DP_LINK_BW values
Make drm_dp_get_vc_payload() tolerate arbitrary DP_LINK_BW_*
values, just like drm_dp_bw_code_to_link_rate() does since commit
|
|
|
Ville Syrjälä
|
db0cc143b6 |
drm/dp/mst: Reduce nested ifs
Replace the nested ifs with a single if and a logical AND. Cc: Lyude Paul <lyude@redhat.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190925141442.23236-1-ville.syrjala@linux.intel.com Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Lyude Paul <lyude@redhat.com> |
|
Matt Roper
|
caf81ec6cd |
drm: Destroy the correct mutex name in drm_dp_mst_topology_mgr_destroy
It looks like one of the topology manager mutexes may have been renamed
during a rebase, but the destruction function wasn't updated with the
new name:
error: ‘struct drm_dp_mst_topology_mgr’ has no member named
‘delayed_destroy_lock’
Fixes:
|
|
|
Lyude Paul
|
e2839ff692 |
drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port
The names for these functions are rather confusing. drm_dp_add_port() sounds like a function that would simply create a port and add it to a topology, and do nothing more. Similarly, drm_dp_update_port() would be assumed to be the function that should be used to update port information after initial creation. While those assumptions are currently correct in how these functions are used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port() can also update the information on a port, and seems explicitly designed to do so. This can be explained pretty simply by the fact that there's more situations that would involve updating the port information based on a link address response as opposed to a connection status notification than the driver's initial topology probe. Case in point: reprobing link addresses after suspend/resume. Since we're about to start using drm_dp_add_port() differently for suspend/resume reprobing, let's rename both functions to clarify what they actually do. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Sean Paul <sean@poorly.run> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-18-lyude@redhat.com |
|
|
Lyude Paul
|
50094b5dcd |
drm/dp_mst: Destroy topology_mgr mutexes
Turns out we've been forgetting for a while now to actually destroy any of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do that. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Sean Paul <sean@poorly.run> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-15-lyude@redhat.com |
|
|
Lyude Paul
|
37dfdc55ff |
drm/dp_mst: Cleanup drm_dp_send_link_address() a bit
Declare local pointer to the drm_dp_link_address_ack_reply struct instead of constantly dereferencing it through the union in txmsg->reply. Then, invert the order of conditionals so we don't have to do the bulk of the work inside them, and can wrap lines even less. Then finally, rearrange variable declarations a bit. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Dave Airlie <airlied@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-16-lyude@redhat.com |
|
|
Lyude Paul
|
8b1e589d13 |
drm/dp_mst: Refactor drm_dp_mst_handle_down_rep()
* Remove the big ugly have_eomt conditional * Store &mgr->down_rep_recv.initial_hdr in a var to make line wrapping easier * Remove duplicate memset() calls * Actually wrap lines Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-14-lyude@redhat.com |
|
|
Lyude Paul
|
a29d881875 |
drm/dp_mst: Refactor drm_dp_mst_handle_up_req()
There's a couple of changes here, so to summarize: * Remove the big ugly mgr->up_req_recv.have_eomt conditional to save on indenting * Store &mgr->up_req_recv.initial_hdr in a variable so we don't keep going over 80 character long lines * De-duplicate code for calling drm_dp_send_up_ack_reply() and getting the MSTB via it's GUID * Remove all of the duplicate calls to memset() and just use a goto instead * Actually do line wrapping * Remove the unnecessary if (mstb) check before calling drm_dp_mst_topology_put_mstb() - we are guaranteed to always have mstb != NULL at that point in the function Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-13-lyude@redhat.com |
|
|
Lyude Paul
|
fde61a7a7d |
drm/dp_mst: Constify guid in drm_dp_get_mst_branch_by_guid()
And it's helper, we'll be using this in just a moment. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Dave Airlie <airlied@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-12-lyude@redhat.com |
|
|
Lyude Paul
|
60f9ae9d0d |
drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req()
Which reduces indentation and makes this function more legible. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-11-lyude@redhat.com |
|
|
Lyude Paul
|
95b0013d20 |
drm/dp_mst: Refactor drm_dp_send_enum_path_resources
Use more pointers so we don't have to write out txmsg->reply.u.path_resources each time. Also, fix line wrapping + rearrange local variables. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-10-lyude@redhat.com |
|
|
Lyude Paul
|
2f015ec6ea |
drm/dp_mst: Add sideband down request tracing + selftests
Unfortunately the DP MST helpers do not have much in the way of debugging utilities. So, let's add some! This adds basic debugging output for down sideband requests that we send from the driver, so that we can actually discern what's happening when sideband requests timeout. Since there wasn't really a good way of testing that any of this worked, I ended up writing simple selftests that lightly test sideband message encoding and decoding as well. Enjoy! Changes since v1: * Clean up DO_TEST() and sideband_msg_req_encode_decode() - danvet * Get rid of pr_fmt(), just define a prefix string instead and use drm_printf() * Check highest bit of VCPI in drm_dp_decode_sideband_req() - danvet * Make the switch case order between drm_dp_decode_sideband_req() and drm_dp_encode_sideband_req() the same - danvet * Only check DRM_UT_DP - danvet * Clean up sideband_msg_req_equal() from selftests a bit, and add comments explaining why we can't just use memcmp - danvet Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-8-lyude@redhat.com |
|
|
Lyude Paul
|
aa2a2fe138 |
drm/dp_mst: Combine redundant cases in drm_dp_encode_sideband_req()
Noticed this while working on adding a drm_dp_decode_sideband_req(). DP_POWER_DOWN_PHY/DP_POWER_UP_PHY both use the same struct as DP_ENUM_PATH_RESOURCES, so we can just combine their cases. Changes since v2: * Fix commit message Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Dave Airlie <airlied@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903215702.16984-1-lyude@redhat.com |
|
|
Lyude Paul
|
7cbce45d62 |
drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest
Yes, apparently we've been testing this for every single driver load for quite a long time now. At least that means our PBN calculation is solid! Anyway, introduce self tests for MST and move this into there. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-5-lyude@redhat.com |
|
|
Lyude Paul
|
3ba64aa36c |
drm/dp_mst: Get rid of list clear in destroy_connector_work
This seems to be some leftover detritus from before the port/mstb kref cleanup and doesn't do anything anymore, so get rid of it. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-3-lyude@redhat.com |
|
|
Lyude Paul
|
5950f0b797 |
drm/dp_mst: Move link address dumping into a function
Makes things easier to read. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Reviewed-by: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190903204645.25487-2-lyude@redhat.com |
|
Sean Paul
|
63b87c310a |
drm/mst: Fix sphinx warnings in drm_dp_msg_connector register functions
Fixes the following warnings:
../drivers/gpu/drm/drm_dp_mst_topology.c:1593: warning: Excess function parameter 'drm_connector' description in 'drm_dp_mst_connector_late_register'
../drivers/gpu/drm/drm_dp_mst_topology.c:1613: warning: Excess function parameter 'drm_connector' description in 'drm_dp_mst_connector_early_unregister'
../drivers/gpu/drm/drm_dp_mst_topology.c:1594: warning: Function parameter or member 'connector' not described in 'drm_dp_mst_connector_late_register'
../drivers/gpu/drm/drm_dp_mst_topology.c:1614: warning: Function parameter or member 'connector' not described in 'drm_dp_mst_connector_early_unregister'
Fixes:
|
|
|
Ville Syrjälä
|
562836a269 |
drm/dp_mst: Enable registration of AUX devices for MST ports
All available downstream ports - physical and logical - are exposed for
each MST device. They are listed in /dev/, following the same naming
scheme as SST devices by appending an incremental ID.
Although all downstream ports are exposed, only some will work as
expected. Consider the following topology:
+---------+
| ASIC |
+---------+
Conn-0|
|
+----v----+
+----| MST HUB |----+
| +---------+ |
| |
|Port-1 Port-2|
+-----v-----+ +-----v-----+
| MST | | SST |
| Display | | Display |
+-----------+ +-----------+
|Port-1
x
MST Path | MST Device
----------+----------------------------------
sst:0 | MST Hub
mst:0-1 | MST Display
mst:0-1-1 | MST Display's disconnected DP out
mst:0-1-8 | MST Display's internal sink
mst:0-2 | SST Display
On certain MST displays, the upstream physical port will ACK DPCD reads.
However, reads on the local logical port to the internal sink will
*NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
There may also be duplicates. Some displays will return the same GUID
when reading DPCD from both mst:0-1 and mst:0-1-8.
There are some device-dependent behavior as well. The MST hub used
during testing will actually *ACK* read requests on a disconnected
physical port, whereas the MST displays will NAK.
In light of these discrepancies, it's simpler to expose all downstream
ports - both physical and logical - and let the user decide what to use.
v3 changes:
* Change WARN_ON_ONCE -> DRM_ERROR on dpcd read errors
* Docstring and cosmetic fixes
v2 changes:
Moved remote aux device (un)registration to new mst connector late
register and early unregister helpers. Drivers should call these from
their own mst connector function hooks.
This is to solve an issue during driver unload, where mst connector
devices are unregistered before the remote aux devices are. In a setup
where aux devices are created as children of connector devices, the aux
device would be removed too early, and uncleanly. Doing so in
early_unregister solves this issue, as that is called before connector
unregistration.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Leo Li <sunpeng.li@amd.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190723232808.28128-3-sunpeng.li@amd.com
|
|
Imre Deak
|
d8fd372220 |
drm/mst: Fix MST sideband up-reply failure handling
Fix the breakage resulting in the stacktrace below, due to tx queue being full when trying to send an up-reply. txmsg->seqno is -1 in this case leading to a corruption of the mstb object by txmsg->dst->tx_slots[txmsg->seqno] = NULL; in process_single_up_tx_qlock(). [ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]] set_hdr_from_dst_qlock: failed to find slot [ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19 [drm_kms_helper]] failed to send msg in q -11 [ +0,000939] BUG: kernel NULL pointer dereference, address: 00000000000005a0 [ +0,006982] #PF: supervisor write access in kernel mode [ +0,005223] #PF: error_code(0x0002) - not-present page [ +0,005135] PGD 0 P4D 0 [ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI [ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted: G U 5.2.0-rc1+ #410 [ +0,008433] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3175.A00.1904261428 04/26/2019 [ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915] [ +0,005676] RIP: 0010:queue_work_on+0x19/0x70 [ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 41 56 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00 <f0> 48 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f [ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006 [ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX: 0000000000000001 [ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI: ffffffff82121972 [ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09: 0000000000000001 [ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88847bfa5096 [ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15: 0000000000000000 [ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000) knlGS:0000000000000000 [ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4: 0000000000760ee0 [ +0,007128] PKRU: 55555554 [ +0,002722] Call Trace: [ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper] [ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper] [ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper] [ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915] [ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915] [ +0,004836] i915_digport_work_func+0xbb/0x140 [i915] [ +0,005108] process_one_work+0x245/0x610 [ +0,004027] worker_thread+0x37/0x380 [ +0,003684] ? process_one_work+0x610/0x610 [ +0,004184] kthread+0x119/0x130 [ +0,003240] ? kthread_park+0x80/0x80 [ +0,003668] ret_from_fork+0x24/0x50 Cc: Lyude Paul <lyude@redhat.com> Cc: Dave Airlie <airlied@redhat.com> Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190523212433.9058-1-imre.deak@intel.com |
|
|
Jani Nikula
|
580fc13f3e |
drm/dp: drmP.h include removal
Continue to get rid of drmP.h. Add minimal includes to build. Sort includes while at it. Reviewed-by: Noralf Trønnes <noralf@tronnes.org> Signed-off-by: Jani Nikula <jani.nikula@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190506095248.20874-1-jani.nikula@intel.com |
|
Manasi Navare
|
2de3a07849 |
drm/dp: Set the connector's TILE property even for DP SST connectors
Current driver sets the tile property only for DP MST connectors. However there are some tiled displays where each SST connector carries a single tile. So we need to attach this property object for every connector and set it for every connector (DP SST and MST). Plus since the tile information is obtained as a result of EDID parsing, the best place to update tile property is where we update edid property. Also now we dont need to explicitly set this now for MST connectors. This has been tested with xrandr --props and modetest and verified that TILE property is exposed correctly. Cc: Dave Airlie <airlied@redhat.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190313021722.10068-1-manasi.d.navare@intel.com |
|
|
Lyude Paul
|
123cbb6c70 |
drm/dp_mst: Remove rebase-detritus in VCPI helper kernel-docs
Looks like when making the final revision of: commit |
|
|
Lyude Paul
|
022debad06 |
drm/atomic: Add drm_atomic_state->duplicated
Since commit |
|
|
Lyude Paul
|
a3d15c4b0e |
drm/dp_mst: Remove port validation in drm_dp_atomic_find_vcpi_slots()
Since we now have an easy way of refcounting drm_dp_mst_port structs and
safely accessing their contents, there isn't any good reason to keep
validating ports here. It doesn't prevent us from performing modesets on
branch devices that have been removed either, and we already disallow
enabling new displays on unregistered connectors in
update_connector_routing() in drm_atomic_check_modeset(). All it does is
cause us to have to make weird special exceptions in our atomic
modesetting code. So, get rid of it entirely.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes:
|
|
|
Lyude Paul
|
3a8844c298 |
drm/dp_mst: Fix unbalanced malloc ref in drm_dp_mst_deallocate_vcpi()
In drm_dp_mst_deallocate_vcpi(), we currently unconditionally call
drm_dp_mst_put_port_malloc() on the port that's passed to us, even if we
never successfully allocated VCPI to it. This is contrary to what we do
in drm_dp_mst_allocate_vcpi(), where we only call
drm_dp_mst_get_port_malloc() on the passed port if we successfully
allocated VCPI to it.
As a result, if drm_dp_mst_allocate_vcpi() fails during a modeset and
another successive modeset calls drm_dp_mst_deallocate_vcpi() we will
end up dropping someone else's malloc reference to the port. Example:
[ 962.309260] ==================================================================
[ 962.309290] BUG: KASAN: use-after-free in drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309296] Read of size 4 at addr ffff888416c30004 by task kworker/0:1H/500
[ 962.309308] CPU: 0 PID: 500 Comm: kworker/0:1H Tainted: G W O 5.0.0-rc2Lyude-Test+ #1
[ 962.309313] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018
[ 962.309428] Workqueue: events_highpri intel_atomic_cleanup_work [i915]
[ 962.309434] Call Trace:
[ 962.309452] dump_stack+0xad/0x150
[ 962.309462] ? dump_stack_print_info.cold.0+0x1b/0x1b
[ 962.309472] ? kmsg_dump_rewind_nolock+0xd9/0xd9
[ 962.309504] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309515] print_address_description+0x6c/0x23c
[ 962.309542] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309568] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309577] kasan_report.cold.3+0x1a/0x32
[ 962.309605] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309631] drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
[ 962.309658] ? drm_dp_mst_put_mstb_malloc+0x180/0x180 [drm_kms_helper]
[ 962.309687] drm_dp_mst_destroy_state+0xcd/0x120 [drm_kms_helper]
[ 962.309745] drm_atomic_state_default_clear+0x6ee/0xcc0 [drm]
[ 962.309864] intel_atomic_state_clear+0xe/0x80 [i915]
[ 962.309928] __drm_atomic_state_free+0x35/0xd0 [drm]
[ 962.310044] intel_atomic_cleanup_work+0x56/0x70 [i915]
[ 962.310057] process_one_work+0x884/0x1400
[ 962.310067] ? drain_workqueue+0x5a0/0x5a0
[ 962.310075] ? __schedule+0x87f/0x1e80
[ 962.310086] ? __sched_text_start+0x8/0x8
[ 962.310095] ? run_rebalance_domains+0x400/0x400
[ 962.310110] ? deref_stack_reg+0xb4/0x120
[ 962.310117] ? __read_once_size_nocheck.constprop.7+0x10/0x10
[ 962.310124] ? worker_enter_idle+0x47f/0x6a0
[ 962.310134] ? schedule+0xd7/0x2e0
[ 962.310141] ? __schedule+0x1e80/0x1e80
[ 962.310148] ? _raw_spin_lock_irq+0x9f/0x130
[ 962.310155] ? _raw_write_unlock_irqrestore+0x110/0x110
[ 962.310164] worker_thread+0x196/0x11e0
[ 962.310175] ? set_load_weight+0x2e0/0x2e0
[ 962.310181] ? __switch_to_asm+0x34/0x70
[ 962.310187] ? __switch_to_asm+0x40/0x70
[ 962.310194] ? process_one_work+0x1400/0x1400
[ 962.310199] ? __switch_to_asm+0x40/0x70
[ 962.310205] ? __switch_to_asm+0x34/0x70
[ 962.310211] ? __switch_to_asm+0x34/0x70
[ 962.310216] ? __switch_to_asm+0x40/0x70
[ 962.310221] ? __switch_to_asm+0x34/0x70
[ 962.310226] ? __switch_to_asm+0x40/0x70
[ 962.310231] ? __switch_to_asm+0x34/0x70
[ 962.310236] ? __switch_to_asm+0x40/0x70
[ 962.310242] ? syscall_return_via_sysret+0xf/0x7f
[ 962.310248] ? __switch_to_asm+0x34/0x70
[ 962.310253] ? __switch_to_asm+0x40/0x70
[ 962.310258] ? __switch_to_asm+0x34/0x70
[ 962.310263] ? __switch_to_asm+0x40/0x70
[ 962.310268] ? __switch_to_asm+0x34/0x70
[ 962.310273] ? __switch_to_asm+0x40/0x70
[ 962.310281] ? __schedule+0x87f/0x1e80
[ 962.310292] ? __sched_text_start+0x8/0x8
[ 962.310300] ? save_stack+0x8c/0xb0
[ 962.310308] ? __kasan_kmalloc.constprop.6+0xc6/0xd0
[ 962.310313] ? kthread+0x98/0x3a0
[ 962.310318] ? ret_from_fork+0x35/0x40
[ 962.310334] ? __wake_up_common+0x178/0x6f0
[ 962.310343] ? _raw_spin_lock_irqsave+0xa4/0x140
[ 962.310349] ? __lock_text_start+0x8/0x8
[ 962.310355] ? _raw_write_lock_irqsave+0x70/0x130
[ 962.310360] ? __lock_text_start+0x8/0x8
[ 962.310371] ? process_one_work+0x1400/0x1400
[ 962.310376] kthread+0x2e2/0x3a0
[ 962.310383] ? kthread_create_on_node+0xc0/0xc0
[ 962.310389] ret_from_fork+0x35/0x40
[ 962.310401] Allocated by task 1462:
[ 962.310410] __kasan_kmalloc.constprop.6+0xc6/0xd0
[ 962.310437] drm_dp_add_port+0xd60/0x1960 [drm_kms_helper]
[ 962.310464] drm_dp_send_link_address+0x4b0/0x770 [drm_kms_helper]
[ 962.310491] drm_dp_check_and_send_link_address+0x197/0x1f0 [drm_kms_helper]
[ 962.310515] drm_dp_mst_link_probe_work+0x2b6/0x330 [drm_kms_helper]
[ 962.310522] process_one_work+0x884/0x1400
[ 962.310529] worker_thread+0x196/0x11e0
[ 962.310533] kthread+0x2e2/0x3a0
[ 962.310538] ret_from_fork+0x35/0x40
[ 962.310543] Freed by task 500:
[ 962.310550] __kasan_slab_free+0x133/0x180
[ 962.310555] kfree+0x92/0x1a0
[ 962.310581] drm_dp_mst_put_port_malloc+0x14d/0x180 [drm_kms_helper]
[ 962.310693] intel_connector_destroy+0xb2/0xe0 [i915]
[ 962.310747] drm_mode_object_put.part.0+0x12b/0x1a0 [drm]
[ 962.310802] drm_atomic_state_default_clear+0x1f2/0xcc0 [drm]
[ 962.310916] intel_atomic_state_clear+0xe/0x80 [i915]
[ 962.310972] __drm_atomic_state_free+0x35/0xd0 [drm]
[ 962.311083] intel_atomic_cleanup_work+0x56/0x70 [i915]
[ 962.311092] process_one_work+0x884/0x1400
[ 962.311098] worker_thread+0x196/0x11e0
[ 962.311103] kthread+0x2e2/0x3a0
[ 962.311108] ret_from_fork+0x35/0x40
[ 962.311116] The buggy address belongs to the object at ffff888416c30000
which belongs to the cache kmalloc-2k of size 2048
[ 962.311122] The buggy address is located 4 bytes inside of
2048-byte region [ffff888416c30000, ffff888416c30800)
[ 962.311124] The buggy address belongs to the page:
[ 962.311132] page:ffffea00105b0c00 count:1 mapcount:0 mapping:ffff88841d003040 index:0x0 compound_mapcount: 0
[ 962.311142] flags: 0x8000000000010200(slab|head)
[ 962.311152] raw: 8000000000010200 dead000000000100 dead000000000200 ffff88841d003040
[ 962.311159] raw: 0000000000000000 00000000000f000f 00000001ffffffff 0000000000000000
[ 962.311162] page dumped because: kasan: bad access detected
So, bail early if drm_dp_mst_deallocate_vcpi() is called on a port with
no VCPI allocation. Additionally, clean up the surrounding kerneldoc
while we're at it since the port is assumed to be kept around because
the DRM driver is expected to hold a malloc reference to it, not just
us.
Changes since v1:
* Doc changes - danvet
Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes:
|
|
|
Matt Roper
|
1e55a53a28 |
drm: Trivial comment grammar cleanups
Most of these are just cases where code comments used contractions (it's, who's) where they actually mean to use a possessive pronoun (its, whose) or vice-versa. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20190202012326.20096-1-matthew.d.roper@intel.com |
|
|
Ville Syrjälä
|
3dadbd2957 |
drm/dp/mst: Provide better debugs for NAK replies
Decode the NAK reply fields to make it easier to parse the logs.
v2: s/STR/DP_STR/ to avoid conflict with some header stuff (0day)
Use drm_dp_mst_req_type_str() more (DK)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190122200301.18633-2-ville.syrjala@linux.intel.com
|
|
|
Ville Syrjälä
|
45bbda1e35 |
drm/dp/mst: Provide defines for ACK vs. NAK reply type
Make the code a bit easier to read by providing symbolic names
for the reply_type (ACK vs. NAK). Also clean up some brace stuff
while at it.
v2: s/DP_REPLY/DP_SIDEBAND_REPLY/ (DK)
Fix some checkpatch issues
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190122200301.18633-1-ville.syrjala@linux.intel.com
|
|
|
Daniel Vetter
|
fcd70cd36b |
drm: Split out drm_probe_helper.h
Having the probe helper stuff (which pretty much everyone needs) in the drm_crtc_helper.h file (which atomic drivers should never need) is confusing. Split them out. To make sure I actually achieved the goal here I went through all drivers. And indeed, all atomic drivers are now free of drm_crtc_helper.h includes. v2: Make it compile. There was so much compile fail on arm drivers that I figured I'll better not include any of the acks on v1. v3: Massive rebase because i915 has lost a lot of drmP.h includes, but not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h there was still one, which this patch largely removes. Which means rolling out lots more includes all over. This will also conflict with ongoing drmP.h cleanup by others I expect. v3: Rebase on top of atomic bochs. v4: Review from Laurent for bridge/rcar/omap/shmob/core bits: - (re)move some of the added includes, use the better include files in other places (all suggested from Laurent adopted unchanged). - sort alphabetically v5: Actually try to sort them, and while at it, sort all the ones I touch. v6: Rebase onto i915 changes. v7: Rebase once more. Acked-by: Harry Wentland <harry.wentland@amd.com> Acked-by: Sam Ravnborg <sam@ravnborg.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> Acked-by: Jani Nikula <jani.nikula@intel.com> Acked-by: Neil Armstrong <narmstrong@baylibre.com> Acked-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> Acked-by: CK Hu <ck.hu@mediatek.com> Acked-by: Alex Deucher <alexander.deucher@amd.com> Acked-by: Sam Ravnborg <sam@ravnborg.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Liviu Dudau <liviu.dudau@arm.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: linux-arm-kernel@lists.infradead.org Cc: virtualization@lists.linux-foundation.org Cc: etnaviv@lists.freedesktop.org Cc: linux-samsung-soc@vger.kernel.org Cc: intel-gfx@lists.freedesktop.org Cc: linux-mediatek@lists.infradead.org Cc: linux-amlogic@lists.infradead.org Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: nouveau@lists.freedesktop.org Cc: spice-devel@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org Cc: linux-renesas-soc@vger.kernel.org Cc: linux-rockchip@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com Cc: linux-tegra@vger.kernel.org Cc: xen-devel@lists.xen.org Link: https://patchwork.freedesktop.org/patch/msgid/20190117210334.13234-1-daniel.vetter@ffwll.ch |
|
|
Lyude Paul
|
5e187a0142 |
drm/dp_mst: Check payload count in drm_dp_mst_atomic_check()
It occurred to me that we never actually check this! So let's start doing that. Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@redhat.com> Cc: Jerry Zuo <Jerry.Zuo@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Juston Li <juston.li@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-20-lyude@redhat.com |
|
|
Lyude Paul
|
eceae14724 |
drm/dp_mst: Start tracking per-port VCPI allocations
There has been a TODO waiting for quite a long time in
drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release.
*/
That's not the only reason we should fix this: forcing the driver to
track the VCPI allocations throughout a state's atomic check is
error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and
drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
idempotency. Currently the only driver actually using these helpers,
i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would
over-allocate VCPI slots, something I learned trying to implement
fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which
must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these
/must/ be called by /all/ atomic drivers supporting MST.
Changes since v9:
* Add some missing changes that were requested by danvet that I forgot
about after I redid all of the kref stuff:
* Remove unnecessary state changes in intel_dp_mst_atomic_check
* Cleanup atomic check logic for VCPI allocations - all we need to check in
compute_config is whether or not this state disables a CRTC, then free
VCPI based off that
Changes since v8:
* Fix compile errors, whoops!
Changes since v7:
- Don't check for mixed stale/valid VCPI allocations, just rely on
connector registration to stop such erroneous modesets
Changes since v6:
- Keep a kref to all of the ports we have allocations on. This required
a good bit of changing to when we call drm_dp_find_vcpi_slots(),
mainly that we need to ensure that we only redo VCPI allocations on
actual mode or CRTC changes, not crtc_state->active changes.
Additionally, we no longer take the registration of the DRM connector
for each port into account because so long as we have a kref to the
port in the new or previous atomic state, the connector will stay
registered.
- Use the small changes to drm_dp_put_port() to add even more error
checking to make misusage of the helpers more obvious. I added this
after having to chase down various use-after-free conditions that
started popping up from the new helpers so no one else has to
troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not be
called on the same port in a single atomic check phase (but multiple
calls to one or the other is OK)
Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI
allocations happen in a state. This makes the next change I'm about
to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that
when ports are destroyed and still have VCPI allocations in the
topology state, the only state changes allowed are releasing said
ports' VCPI. This prevents a state with a mix of VCPI allocations
from destroyed ports, and allocations from valid ports.
Changes since v3:
- Don't release VCPI allocations in the topology state immediately in
drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip
over them in drm_dp_mst_duplicate_state(). This makes it so
drm_dp_atomic_release_vcpi_slots() is still idempotent while also
throwing warnings if the driver messes up it's book keeping and tries
to release VCPI slots on a port that doesn't have any pre-existing
VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"
Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in
drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's
own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth
requirements. This is needed because originally we relied on the
state duplication functions to prune any stale ports from the new
state, which would prevent us from incorrectly considering their
bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet
Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook,
just give drivers a function to call themselves
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-19-lyude@redhat.com
|
|
|
Lyude Paul
|
bea5c38f1e |
drm/dp_mst: Add some atomic state iterator macros
Changes since v6: - Move EXPORT_SYMBOL() for drm_dp_mst_topology_state_funcs to this commit - Document __drm_dp_mst_state_iter_get() and note that it shouldn't be called directly Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@redhat.com> Cc: Jerry Zuo <Jerry.Zuo@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Juston Li <juston.li@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-18-lyude@redhat.com |
|
|
Lyude Paul
|
cfe9f90358 |
drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs
Up until now, freeing payloads on remote MST hubs that just had ports removed has almost never worked because we've been relying on port validation in order to stop us from accessing ports that have already been freed from memory, but ports which need their payloads released due to being removed will never be a valid part of the topology after they've been removed. Since we've introduced malloc refs, we can replace all of the validation logic in payload helpers which are used for deallocation with some well-placed malloc krefs. This ensures that regardless of whether or not the ports are still valid and in the topology, any port which has an allocated payload will remain allocated in memory until it's payloads have been removed - finally allowing us to actually release said payloads correctly. Signed-off-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Cc: David Airlie <airlied@redhat.com> Cc: Jerry Zuo <Jerry.Zuo@amd.com> Cc: Juston Li <juston.li@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-10-lyude@redhat.com |