Noticed this while trying to respin my MST suspend/resume patch series.
It's not technically possible (at least until someone moves amdgpu
away from the deprecated drm_device->driver->{load,unload} hooks) for
amdgpu to properly register all of it's encoders before registering with
userspace. However, amdgpu also apparently adds and removes encoders
along with MST connectors - which is a much bigger issue as userspace
applications definitely do not expect this type of behavior.
So, let's fix it and add some WARNs() so new drivers don't accidentally
make this mistake in the future.
Lyude Paul (6):
drm/amdgpu/dm/mst: Don't create MST topology managers for eDP ports
drm/amdgpu/dm/mst: Remove unnecessary NULL check
drm/amdgpu/dm/mst: Use ->atomic_best_encoder
drm/amdgpu/dm/mst: Make MST encoders per-CRTC and fix encoder usage
drm/amdgpu/dm/mst: Report possible_crtcs incorrectly, for now
drm/encoder: WARN() when adding/removing encoders after device
registration
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 ++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 46 ++++++++++---------
.../display/amdgpu_dm/amdgpu_dm_mst_types.h | 3 ++
drivers/gpu/drm/drm_encoder.c | 31 ++++++++++---
6 files changed, 70 insertions(+), 29 deletions(-)
--
2.21.0
Signed-off-by: Lyude Paul <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 5ec14efd4d8c..185bf0e2bda2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -417,6 +417,10 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
&aconnector->base);
+
+ if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
+ return;
+
aconnector->mst_mgr.cbs = &dm_mst_cbs;
drm_dp_mst_topology_mgr_init(
&aconnector->mst_mgr,
--
2.21.0
kfree() checks this automatically.
Signed-off-by: Lyude Paul <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 185bf0e2bda2..a398ddd1f306 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -144,10 +144,8 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
struct amdgpu_encoder *amdgpu_encoder = amdgpu_dm_connector->mst_encoder;
- if (amdgpu_dm_connector->edid) {
- kfree(amdgpu_dm_connector->edid);
- amdgpu_dm_connector->edid = NULL;
- }
+ kfree(amdgpu_dm_connector->edid);
+ amdgpu_dm_connector->edid = NULL;
drm_encoder_cleanup(&amdgpu_encoder->base);
kfree(amdgpu_encoder);
--
2.21.0
We are supposed to be atomic after all. We'll need this in a moment for
the next commit.
Signed-off-by: Lyude Paul <[email protected]>
---
.../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index a398ddd1f306..d9113ce0be09 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -243,17 +243,17 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
return ret;
}
-static struct drm_encoder *dm_mst_best_encoder(struct drm_connector *connector)
+static struct drm_encoder *
+dm_mst_atomic_best_encoder(struct drm_connector *connector,
+ struct drm_connector_state *connector_state)
{
- struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
-
- return &amdgpu_dm_connector->mst_encoder->base;
+ return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
}
static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
.get_modes = dm_dp_mst_get_modes,
.mode_valid = amdgpu_dm_connector_mode_valid,
- .best_encoder = dm_mst_best_encoder,
+ .atomic_best_encoder = dm_mst_atomic_best_encoder,
};
static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
--
2.21.0
While this commit certainly will result in creating less fake MST
encoders, the main purpose of this is actually to fix amdgpu's incorrect
usage of the drm_encoder API in it's MST code. Currently we create one
encoder per-MST connector. However, MST connectors can and usually do
get created at basically any point before or after driver device
registration. Thus, this means we've been creating and destroying
drm_encoder structs every time we create or destroy an MST connector.
This behavior likely is just leftover from when we made amdgpu stop
reusing DRM connectors for MST. Since this will definitely break things,
let's fix it by being a bit more efficient with our MST encoders by
creating one per-CRTC, which allows us to have just the right number of
encoders and do so before we've called drm_dev_register().
Signed-off-by: Lyude Paul <[email protected]>
Fixes: 0e6613e46fed ("drm/amd/display: Drop reusing drm connector for MST")
Cc: Harry Wentland <[email protected]>
Cc: Lyude Paul <[email protected]>
Cc: Alex Deucher <[email protected]>
Cc: Nicholas Kazlauskas <[email protected]>
Cc: Leo Li <[email protected]>
Cc: David Francis <[email protected]>
Cc: "Jerry (Fangzhi) Zuo" <[email protected]>
Cc: Mario Kleiner <[email protected]>
Cc: Thomas Lim <[email protected]>
Cc: "Mathias Fröhlich" <[email protected]>
Cc: <[email protected]> # v4.20+
---
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 ++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 ++++++++++---------
.../display/amdgpu_dm/amdgpu_dm_mst_types.h | 3 ++
5 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index eb9975f4decb..b54a6f4e392e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -427,6 +427,9 @@ struct amdgpu_crtc {
int otg_inst;
struct drm_pending_vblank_event *event;
+
+ /* Fake encoder to use for MST */
+ struct amdgpu_encoder *mst_encoder;
};
struct amdgpu_encoder_atom_dig {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index f4e0f27a76de..b404f1ae6df7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4803,6 +4803,10 @@ static int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
true, MAX_COLOR_LUT_ENTRIES);
drm_mode_crtc_set_gamma_size(&acrtc->base, MAX_COLOR_LEGACY_LUT_ENTRIES);
+ acrtc->mst_encoder = amdgpu_dm_dp_create_fake_mst_encoder(acrtc);
+ if (!acrtc->mst_encoder)
+ goto fail;
+
return 0;
fail:
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index c8c525a2b505..bcd9115c4923 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -267,7 +267,6 @@ struct amdgpu_dm_connector {
struct amdgpu_dm_dp_aux dm_dp_aux;
struct drm_dp_mst_port *port;
struct amdgpu_dm_connector *mst_port;
- struct amdgpu_encoder *mst_encoder;
/* TODO see if we can merge with ddc_bus or make a dm_connector */
struct amdgpu_i2c_adapter *i2c;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index d9113ce0be09..d680f32cf625 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -141,14 +141,12 @@ dm_dp_mst_detect(struct drm_connector *connector, bool force)
static void
dm_dp_mst_connector_destroy(struct drm_connector *connector)
{
- struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
- struct amdgpu_encoder *amdgpu_encoder = amdgpu_dm_connector->mst_encoder;
+ struct amdgpu_dm_connector *amdgpu_dm_connector =
+ to_amdgpu_dm_connector(connector);
kfree(amdgpu_dm_connector->edid);
amdgpu_dm_connector->edid = NULL;
- drm_encoder_cleanup(&amdgpu_encoder->base);
- kfree(amdgpu_encoder);
drm_connector_cleanup(connector);
drm_dp_mst_put_port_malloc(amdgpu_dm_connector->port);
kfree(amdgpu_dm_connector);
@@ -247,7 +245,7 @@ static struct drm_encoder *
dm_mst_atomic_best_encoder(struct drm_connector *connector,
struct drm_connector_state *connector_state)
{
- return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
+ return &to_amdgpu_crtc(connector_state->crtc)->mst_encoder->base;
}
static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
@@ -266,11 +264,10 @@ static const struct drm_encoder_funcs amdgpu_dm_encoder_funcs = {
.destroy = amdgpu_dm_encoder_destroy,
};
-static struct amdgpu_encoder *
-dm_dp_create_fake_mst_encoder(struct amdgpu_dm_connector *connector)
+struct amdgpu_encoder *
+amdgpu_dm_dp_create_fake_mst_encoder(struct amdgpu_crtc *acrtc)
{
- struct drm_device *dev = connector->base.dev;
- struct amdgpu_device *adev = dev->dev_private;
+ struct drm_device *dev = acrtc->base.dev;
struct amdgpu_encoder *amdgpu_encoder;
struct drm_encoder *encoder;
@@ -279,7 +276,7 @@ dm_dp_create_fake_mst_encoder(struct amdgpu_dm_connector *connector)
return NULL;
encoder = &amdgpu_encoder->base;
- encoder->possible_crtcs = amdgpu_dm_get_encoder_crtc_mask(adev);
+ encoder->possible_crtcs = drm_crtc_mask(&acrtc->base);
drm_encoder_init(
dev,
@@ -302,6 +299,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
struct drm_device *dev = master->base.dev;
struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_dm_connector *aconnector;
+ struct drm_crtc *crtc;
struct drm_connector *connector;
aconnector = kzalloc(sizeof(*aconnector), GFP_KERNEL);
@@ -329,9 +327,13 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
master->dc_link,
master->connector_id);
- aconnector->mst_encoder = dm_dp_create_fake_mst_encoder(master);
- drm_connector_attach_encoder(&aconnector->base,
- &aconnector->mst_encoder->base);
+ /* Attach fake MST encoders */
+ drm_for_each_crtc(crtc, dev) {
+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+
+ drm_connector_attach_encoder(connector,
+ &acrtc->mst_encoder->base);
+ }
drm_object_attach_property(
&connector->base,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
index 2da851b40042..ce84d9db83e5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
@@ -28,8 +28,11 @@
struct amdgpu_display_manager;
struct amdgpu_dm_connector;
+struct amdgpu_crtc;
void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
struct amdgpu_dm_connector *aconnector);
+struct amdgpu_encoder *
+amdgpu_dm_dp_create_fake_mst_encoder(struct amdgpu_crtc *acrtc);
#endif
--
2.21.0
This commit is seperate from the previous one to make it easier to
revert in the future. Basically, there's multiple userspace applications
that interpret possible_crtcs very wrong:
https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
https://gitlab.gnome.org/GNOME/mutter/issues/759
While work is ongoing to fix these issues in userspace, we need to
report ->possible_crtcs incorrectly for now in order to avoid
introducing a regression in in userspace. Once these issues get fixed,
this commit should be reverted.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Ville Syrjälä <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b404f1ae6df7..fe8ac801d7a5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
if (!acrtc->mst_encoder)
goto fail;
+ /*
+ * FIXME: This is a hack to workaround the following issues:
+ *
+ * https://gitlab.gnome.org/GNOME/mutter/issues/759
+ * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
+ *
+ * One these issues are closed, this should be removed
+ */
+ acrtc->mst_encoder->base.possible_crtcs =
+ amdgpu_dm_get_encoder_crtc_mask(dm->adev);
+
return 0;
fail:
--
2.21.0
Turns out that we don't actually check this anywhere, and additionally
actually forget to even mention this in our documentation.
Since we've had one driver making this mistake already, let's clarify
this by mentioning this limitation in the kernel docs. Additionally, for
drivers not using the legacy ->load/->unload() hooks (which make it
impossible to create all encoders before registration): print a big
warning when drm_encoder_init() is called after device registration, or
when drm_encoder_cleanup() is called before device unregistration.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Ville Syrjälä <[email protected]>
---
drivers/gpu/drm/drm_encoder.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 80d88a55302e..5c5e40aafa4e 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -99,9 +99,12 @@ void drm_encoder_unregister_all(struct drm_device *dev)
* @encoder_type: user visible type of the encoder
* @name: printf style format string for the encoder name, or NULL for default name
*
- * Initialises a preallocated encoder. Encoder should be subclassed as part of
- * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
- * called from the driver's &drm_encoder_funcs.destroy hook.
+ * Initialises a preallocated encoder. The encoder should be subclassed as
+ * part of driver encoder objects. This function may not be called after the
+ * device is registered with drm_dev_register().
+ *
+ * At driver unload time drm_encoder_cleanup() must be called from the
+ * driver's &drm_encoder_funcs.destroy hook.
*
* Returns:
* Zero on success, error code on failure.
@@ -117,6 +120,14 @@ int drm_encoder_init(struct drm_device *dev,
if (WARN_ON(dev->mode_config.num_encoder >= 32))
return -EINVAL;
+ /*
+ * Encoders should never be added after device registration, with the
+ * exception of drivers using the legacy load/unload callbacks where
+ * it's impossible to create encoders beforehand. Such drivers should
+ * convert to using drm_dev_register() and friends.
+ */
+ WARN_ON(dev->registered && !dev->driver->load);
+
ret = drm_mode_object_add(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
if (ret)
return ret;
@@ -155,16 +166,22 @@ EXPORT_SYMBOL(drm_encoder_init);
* drm_encoder_cleanup - cleans up an initialised encoder
* @encoder: encoder to cleanup
*
- * Cleans up the encoder but doesn't free the object.
+ * Cleans up the encoder but doesn't free the object. This function may not be
+ * called until the respective &struct drm_device has been unregistered from
+ * userspace using drm_dev_unregister().
*/
void drm_encoder_cleanup(struct drm_encoder *encoder)
{
struct drm_device *dev = encoder->dev;
- /* Note that the encoder_list is considered to be static; should we
- * remove the drm_encoder at runtime we would have to decrement all
- * the indices on the drm_encoder after us in the encoder_list.
+ /*
+ * Encoders should never be removed after device registration, with
+ * the exception of drivers using the legacy load/unload callbacks
+ * where it's impossible to remove encoders until after
+ * deregistration. Such drivers should convert to using
+ * drm_dev_register() and friends.
*/
+ WARN_ON(dev->registered && !dev->driver->unload);
if (encoder->bridge) {
struct drm_bridge *bridge = encoder->bridge;
--
2.21.0
On Thu, Sep 26, 2019 at 6:52 PM Lyude Paul <[email protected]> wrote:
>
> kfree() checks this automatically.
>
> Signed-off-by: Lyude Paul <[email protected]>
Reviewed-by: Alex Deucher <[email protected]>
And applied. Thanks!
Alex
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 185bf0e2bda2..a398ddd1f306 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -144,10 +144,8 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
> struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
> struct amdgpu_encoder *amdgpu_encoder = amdgpu_dm_connector->mst_encoder;
>
> - if (amdgpu_dm_connector->edid) {
> - kfree(amdgpu_dm_connector->edid);
> - amdgpu_dm_connector->edid = NULL;
> - }
> + kfree(amdgpu_dm_connector->edid);
> + amdgpu_dm_connector->edid = NULL;
>
> drm_encoder_cleanup(&amdgpu_encoder->base);
> kfree(amdgpu_encoder);
> --
> 2.21.0
>
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Thu, Sep 26, 2019 at 6:52 PM Lyude Paul <[email protected]> wrote:
>
> We are supposed to be atomic after all. We'll need this in a moment for
> the next commit.
>
> Signed-off-by: Lyude Paul <[email protected]>
Acked-by: Alex Deucher <[email protected]>
> ---
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index a398ddd1f306..d9113ce0be09 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -243,17 +243,17 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> -static struct drm_encoder *dm_mst_best_encoder(struct drm_connector *connector)
> +static struct drm_encoder *
> +dm_mst_atomic_best_encoder(struct drm_connector *connector,
> + struct drm_connector_state *connector_state)
> {
> - struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
> -
> - return &amdgpu_dm_connector->mst_encoder->base;
> + return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
> }
>
> static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
> .get_modes = dm_dp_mst_get_modes,
> .mode_valid = amdgpu_dm_connector_mode_valid,
> - .best_encoder = dm_mst_best_encoder,
> + .atomic_best_encoder = dm_mst_atomic_best_encoder,
> };
>
> static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
> --
> 2.21.0
>
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote:
> This commit is seperate from the previous one to make it easier to
> revert in the future. Basically, there's multiple userspace applications
> that interpret possible_crtcs very wrong:
>
> https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> https://gitlab.gnome.org/GNOME/mutter/issues/759
>
> While work is ongoing to fix these issues in userspace, we need to
> report ->possible_crtcs incorrectly for now in order to avoid
> introducing a regression in in userspace. Once these issues get fixed,
> this commit should be reverted.
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b404f1ae6df7..fe8ac801d7a5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
> if (!acrtc->mst_encoder)
> goto fail;
>
> + /*
> + * FIXME: This is a hack to workaround the following issues:
> + *
> + * https://gitlab.gnome.org/GNOME/mutter/issues/759
> + * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> + *
> + * One these issues are closed, this should be removed
Even when these issues are closed, we'll still be introducing a regression if we
revert this change. Time for actually_possible_crtcs? :)
You also might want to briefly explain the u/s bug in case the links go sour.
> + */
> + acrtc->mst_encoder->base.possible_crtcs =
> + amdgpu_dm_get_encoder_crtc_mask(dm->adev);
Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()?
Sean
> +
> return 0;
>
> fail:
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On 2019-09-26 6:51 p.m., Lyude Paul wrote:
> Signed-off-by: Lyude Paul <[email protected]>
Reviewed-by: Harry Wentland <[email protected]>
Harry
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 5ec14efd4d8c..185bf0e2bda2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -417,6 +417,10 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> &aconnector->base);
> +
> + if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> + return;
> +
> aconnector->mst_mgr.cbs = &dm_mst_cbs;
> drm_dp_mst_topology_mgr_init(
> &aconnector->mst_mgr,
>
On 2019-09-26 6:51 p.m., Lyude Paul wrote:
> We are supposed to be atomic after all. We'll need this in a moment for
> the next commit.
>
> Signed-off-by: Lyude Paul <[email protected]>
Reviewed-by: Harry Wentland <[email protected]>
Harry
> ---
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index a398ddd1f306..d9113ce0be09 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -243,17 +243,17 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> -static struct drm_encoder *dm_mst_best_encoder(struct drm_connector *connector)
> +static struct drm_encoder *
> +dm_mst_atomic_best_encoder(struct drm_connector *connector,
> + struct drm_connector_state *connector_state)
> {
> - struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
> -
> - return &amdgpu_dm_connector->mst_encoder->base;
> + return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
> }
>
> static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
> .get_modes = dm_dp_mst_get_modes,
> .mode_valid = amdgpu_dm_connector_mode_valid,
> - .best_encoder = dm_mst_best_encoder,
> + .atomic_best_encoder = dm_mst_atomic_best_encoder,
> };
>
> static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
>
On Fri, Sep 27, 2019 at 1:48 PM Harry Wentland <[email protected]> wrote:
>
> On 2019-09-26 6:51 p.m., Lyude Paul wrote:
> > Signed-off-by: Lyude Paul <[email protected]>
>
> Reviewed-by: Harry Wentland <[email protected]>
>
Applied. Thanks!
Alex
> Harry
>
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 5ec14efd4d8c..185bf0e2bda2 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -417,6 +417,10 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> > drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > &aconnector->base);
> > +
> > + if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > + return;
> > +
> > aconnector->mst_mgr.cbs = &dm_mst_cbs;
> > drm_dp_mst_topology_mgr_init(
> > &aconnector->mst_mgr,
> >
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Fri, Sep 27, 2019 at 1:56 PM Harry Wentland <[email protected]> wrote:
>
> On 2019-09-26 6:51 p.m., Lyude Paul wrote:
> > We are supposed to be atomic after all. We'll need this in a moment for
> > the next commit.
> >
> > Signed-off-by: Lyude Paul <[email protected]>
>
> Reviewed-by: Harry Wentland <[email protected]>
>
Applied. Thanks!
Alex
> Harry
>
> > ---
> > .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index a398ddd1f306..d9113ce0be09 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -243,17 +243,17 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> > return ret;
> > }
> >
> > -static struct drm_encoder *dm_mst_best_encoder(struct drm_connector *connector)
> > +static struct drm_encoder *
> > +dm_mst_atomic_best_encoder(struct drm_connector *connector,
> > + struct drm_connector_state *connector_state)
> > {
> > - struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
> > -
> > - return &amdgpu_dm_connector->mst_encoder->base;
> > + return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
> > }
> >
> > static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
> > .get_modes = dm_dp_mst_get_modes,
> > .mode_valid = amdgpu_dm_connector_mode_valid,
> > - .best_encoder = dm_mst_best_encoder,
> > + .atomic_best_encoder = dm_mst_atomic_best_encoder,
> > };
> >
> > static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
> >
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Fri, Sep 27, 2019 at 11:27:41AM -0400, Sean Paul wrote:
> On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote:
> > This commit is seperate from the previous one to make it easier to
> > revert in the future. Basically, there's multiple userspace applications
> > that interpret possible_crtcs very wrong:
> >
> > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > https://gitlab.gnome.org/GNOME/mutter/issues/759
> >
> > While work is ongoing to fix these issues in userspace, we need to
> > report ->possible_crtcs incorrectly for now in order to avoid
> > introducing a regression in in userspace. Once these issues get fixed,
> > this commit should be reverted.
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Cc: Ville Syrj?l? <[email protected]>
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index b404f1ae6df7..fe8ac801d7a5 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
> > if (!acrtc->mst_encoder)
> > goto fail;
> >
> > + /*
> > + * FIXME: This is a hack to workaround the following issues:
> > + *
> > + * https://gitlab.gnome.org/GNOME/mutter/issues/759
> > + * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > + *
> > + * One these issues are closed, this should be removed
>
> Even when these issues are closed, we'll still be introducing a regression if we
> revert this change. Time for actually_possible_crtcs? :)
>
> You also might want to briefly explain the u/s bug in case the links go sour.
>
> > + */
> > + acrtc->mst_encoder->base.possible_crtcs =
> > + amdgpu_dm_get_encoder_crtc_mask(dm->adev);
>
> Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()?
If we don't have the same hack for i915 mst I think we shouldn't merge
this ... broken userspace is broken.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
a little late but: i915 does have this hack (or rather-possible_crtcs with MST
in i915 has been broken for a while and got fixed, but had to get reverted
because of this issue), it's where this originally came from.
On Wed, 2019-10-09 at 17:01 +0200, Daniel Vetter wrote:
> On Fri, Sep 27, 2019 at 11:27:41AM -0400, Sean Paul wrote:
> > On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote:
> > > This commit is seperate from the previous one to make it easier to
> > > revert in the future. Basically, there's multiple userspace applications
> > > that interpret possible_crtcs very wrong:
> > >
> > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > > https://gitlab.gnome.org/GNOME/mutter/issues/759
> > >
> > > While work is ongoing to fix these issues in userspace, we need to
> > > report ->possible_crtcs incorrectly for now in order to avoid
> > > introducing a regression in in userspace. Once these issues get fixed,
> > > this commit should be reverted.
> > >
> > > Signed-off-by: Lyude Paul <[email protected]>
> > > Cc: Ville Syrjälä <[email protected]>
> > > ---
> > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > index b404f1ae6df7..fe8ac801d7a5 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct
> > > amdgpu_display_manager *dm,
> > > if (!acrtc->mst_encoder)
> > > goto fail;
> > >
> > > + /*
> > > + * FIXME: This is a hack to workaround the following issues:
> > > + *
> > > + * https://gitlab.gnome.org/GNOME/mutter/issues/759
> > > + * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > > + *
> > > + * One these issues are closed, this should be removed
> >
> > Even when these issues are closed, we'll still be introducing a regression
> > if we
> > revert this change. Time for actually_possible_crtcs? :)
> >
> > You also might want to briefly explain the u/s bug in case the links go
> > sour.
> >
> > > + */
> > > + acrtc->mst_encoder->base.possible_crtcs =
> > > + amdgpu_dm_get_encoder_crtc_mask(dm->adev);
> >
> > Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()?
>
> If we don't have the same hack for i915 mst I think we shouldn't merge
> this ... broken userspace is broken.
> -Daniel
--
Cheers,
Lyude Paul
On Fri, Oct 11, 2019 at 04:51:13PM -0400, Lyude Paul wrote:
> a little late but: i915 does have this hack (or rather-possible_crtcs with MST
> in i915 has been broken for a while and got fixed, but had to get reverted
> because of this issue), it's where this originally came from.
Hm since this is widespread I think we should check for this when we
register connectors (either in drm_dev_register, or hotplugged ones). I
think just validating that all encoder->possible_crtc match and WARN_ON if
not would be really good.
2nd option would be to do that in the GETENCODERS ioctl. That would at
least keep the encoders useful for driver-internal stuff. We could then
un-revert the i915 patch again.
Either way I think we should have this hack + comment with links to the
offending userspace in common code, not duplicated over all drivers.
-Daniel
>
> On Wed, 2019-10-09 at 17:01 +0200, Daniel Vetter wrote:
> > On Fri, Sep 27, 2019 at 11:27:41AM -0400, Sean Paul wrote:
> > > On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote:
> > > > This commit is seperate from the previous one to make it easier to
> > > > revert in the future. Basically, there's multiple userspace applications
> > > > that interpret possible_crtcs very wrong:
> > > >
> > > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > > > https://gitlab.gnome.org/GNOME/mutter/issues/759
> > > >
> > > > While work is ongoing to fix these issues in userspace, we need to
> > > > report ->possible_crtcs incorrectly for now in order to avoid
> > > > introducing a regression in in userspace. Once these issues get fixed,
> > > > this commit should be reverted.
> > > >
> > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > Cc: Ville Syrj?l? <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > index b404f1ae6df7..fe8ac801d7a5 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct
> > > > amdgpu_display_manager *dm,
> > > > if (!acrtc->mst_encoder)
> > > > goto fail;
> > > >
> > > > + /*
> > > > + * FIXME: This is a hack to workaround the following issues:
> > > > + *
> > > > + * https://gitlab.gnome.org/GNOME/mutter/issues/759
> > > > + * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> > > > + *
> > > > + * One these issues are closed, this should be removed
> > >
> > > Even when these issues are closed, we'll still be introducing a regression
> > > if we
> > > revert this change. Time for actually_possible_crtcs? :)
> > >
> > > You also might want to briefly explain the u/s bug in case the links go
> > > sour.
> > >
> > > > + */
> > > > + acrtc->mst_encoder->base.possible_crtcs =
> > > > + amdgpu_dm_get_encoder_crtc_mask(dm->adev);
> > >
> > > Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()?
> >
> > If we don't have the same hack for i915 mst I think we shouldn't merge
> > this ... broken userspace is broken.
> > -Daniel
> --
> Cheers,
> Lyude Paul
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch