similar to: [PATCH 2/6] drm/nouveau: Unbreak nv50_mstc->best_encoder()

As mentioned in the previous commit, we currently prevent new modesets on recently-removed MST connectors by returning no encoder from our ->best_encoder() callback once the MST port has disappeared. This is wrong however, because it prevents legacy modesetting users from being able to disable CRTCs on MST connectors after the connector's respective topology has disappeared. So, fix this

As mentioned in the previous commit, we currently prevent new modesets on recently-removed MST connectors by returning no encoder from our ->best_encoder() callback once the MST port has disappeared. This is wrong however, because it prevents legacy modesetting users from being able to disable CRTCs on MST connectors after the connector's respective topology has disappeared. So, fix this

On Fri, Sep 13, 2019 at 6:05 PM Lyude Paul <lyude at redhat.com> wrote: > > When drm_connector_helper_funcs->atomic_best_encoder is defined, > ->best_encoder is ignored both by the atomic modesetting helpers. That By both the atomic modesetting helpers and ... (usually "both" implies 2 things) > being said, this hook is completely broken anyway - it always

When drm_connector_helper_funcs->atomic_best_encoder is defined, ->best_encoder is ignored both by the atomic modesetting helpers. That being said, this hook is completely broken anyway - it always returns the first msto for a given mstc, despite the fact it might already be in use. So, just get rid of it. We'll need this in a moment anyway, when we make mstos per-head as opposed to

Currently, nouveau uses the yolo method of setting up MST displays: it uses the old VCPI helpers (drm_dp_find_vcpi_slots()) for computing the display configuration. These helpers don't take care to make sure they take a reference to the mstb port that they're checking, and additionally don't actually check whether or not the topology still has enough bandwidth to provide the VCPI

Currently, nouveau uses the yolo method of setting up MST displays: it uses the old VCPI helpers (drm_dp_find_vcpi_slots()) for computing the display configuration. These helpers don't take care to make sure they take a reference to the mstb port that they're checking, and additionally don't actually check whether or not the topology still has enough bandwidth to provide the VCPI

Same as we did for i915, but for nouveau this time. Additionally, we grab a malloc reference to the port that lasts for the entire lifetime of nv50_mstc, which gives us the guarantee that mstc->port will always point to valid memory for as long as the mstc stays around. Signed-off-by: Lyude Paul <lyude at redhat.com> --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 18 +++++------------- 1

Currently, nouveau tries to go through the drm_dp_mst_port structures itself in order to retrieve the relevant payload and VCPI information that it needs to report to the GPU. This is wrong: mstc->port could be destroyed at any point, and additionally the payload could be changed at any point because it doesn't bother trying to grab the payload lock. So; remove nv50_msto_payload entirely

Atomic checks should never modify anything outside of the state that they're passed in. Unfortunately this appears to be exactly what we're doing in nv50_msto_atomic_check() where we update mstc->pbn every time the function is called. This hasn't caused any bugs yet, but it needs to be fixed in order to ensure that when committing an artificially duplicated state (like during system

There is no need to look at the port's VCPI allocation before calling drm_dp_mst_deallocate_vcpi(), as we already have msto->disabled to let us avoid cleaning up an msto more then once. The DP MST core will never call drm_dp_mst_deallocate_vcpi() on it's own, which is presumably what these checks are meant to protect against. More importantly though, we're about to stop clearing

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

Despite being an atomic driver, nouveau has a lot of leftover code that relies on retrieving information regarding the new atomic state from members of drm_encoder and drm_crtc. The first field being used, drm_encoder.crtc, is deprecated for atomic drivers. The second field being used is drm_crtc.state, which is only really sensible to use outside of an atomic modeset. So, add some helpers to

I have a strange dejavu feeling that I tried to submit a patch for this in the past, but that it was rejected. I can't remember though, but I'm further convinced this patch is the right thing to do anyway. We label the to-be-committed head state in nv50_msto_atomic_enable() as armh. Normally armh implies a state which is currently armed in hardware. nv50_msto_atomic_enable() is called

No functional changes, just change the function names to match the callbacks they provide. Signed-off-by: Lyude Paul <lyude at redhat.com> --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 33 ++++++++++++------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index

It turns out that I forgot to go through and make sure that I converted all encoder callbacks to use atomic_enable/atomic_disable(), so let's go and actually do that. Signed-off-by: Lyude Paul <lyude at redhat.com> Cc: Kirill A. Shutemov <kirill at shutemov.name> Fixes: 09838c4efe9a ("drm/nouveau/kms: Search for encoders' connectors properly") ---

While we currently grab a runtime PM ref in nouveau's normal connector detection code, we apparently don't do this for MST. This means if we're in a scenario where the GPU is suspended and userspace attempts to do a connector probe on an MSTC connector, the probe will fail entirely due to the DP aux channel and GPU not being woken up: [ 316.633489] nouveau 0000:01:00.0: i2c: aux

On Thu, Aug 17, 2017 at 6:03 PM, Colin King <colin.king at canonical.com> wrote: > From: Colin Ian King <colin.king at canonical.com> > > The null check on the array msto is incorrect since msto is never > null. The null check should be instead on msto[i] since this is > being dereferenced in the call to drm_mode_connector_attach_encoder. > > Thanks to Emil Velikov

On 08/18/2017 08:16 AM, Colin Ian King wrote: > On 17/08/17 23:07, Ilia Mirkin wrote: >> On Thu, Aug 17, 2017 at 6:03 PM, Colin King <colin.king at canonical.com> wrote: >>> From: Colin Ian King <colin.king at canonical.com> >>> >>> The null check on the array msto is incorrect since msto is never >>> null. The null check should be instead

Hi Sean, Thank you for the patch. On Thu, May 02, 2019 at 03:49:46PM -0400, Sean Paul wrote: > From: Sean Paul <seanpaul at chromium.org> > > Everyone who implements connector_helper_funcs->atomic_check reaches > into the connector state to get the atomic state. Instead of continuing > this pattern, change the callback signature to just give atomic state > and let the

From: Sean Paul <seanpaul at chromium.org> Everyone who implements connector_helper_funcs->atomic_check reaches into the connector state to get the atomic state. Instead of continuing this pattern, change the callback signature to just give atomic state and let the driver determine what it does and does not need from it. Eventually all atomic functions should do this, but that's