[6/6] drm/i915/mst: Document the userspace fail with possible_crtcs
diff mbox series

Message ID 20191002162505.30716-6-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [1/6] drm/i915: Polish possible_clones setup
Related show

Commit Message

Ville Syrjälä Oct. 2, 2019, 4:25 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To avoid accidentally breaking things in the future add a
comment explaining why we misconfigure the pipe_mask.

Also toss in a TODO for investigating a single encoder
approach as opposed to the encoder-per-pipe approach.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Ville Syrjälä Oct. 3, 2019, 3:28 p.m. UTC | #1
On Wed, Oct 02, 2019 at 07:25:05PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To avoid accidentally breaking things in the future add a
> comment explaining why we misconfigure the pipe_mask.
> 
> Also toss in a TODO for investigating a single encoder
> approach as opposed to the encoder-per-pipe approach.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7be82cf926ca..cb3047fe2d02 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -615,6 +615,18 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
>  	intel_encoder->power_domain = intel_dig_port->base.power_domain;
>  	intel_encoder->port = intel_dig_port->base.port;
>  	intel_encoder->cloneable = 0;
> +	/*
> +	 * This is wrong, but broken userspace uses the intersection
> +	 * of possible_crtcs of all the encoders of a given connector
> +	 * to figure out which crtcs can drive said connector. What
> +	 * should be used instead is the union of possible_crtcs.
> +	 * To keep such userspace functioning we must misconfigure
> +	 * this to make sure the intersection is not empty :(
> +	 *
> +	 * TODO: figure out if we could eliminate the per-pipe
> +	 * encoders here and just have a single encoder for each
> +	 * MST connector. That would sidestep the userspace bug.

That of course won't work since we can't register encoders dynamically.
I guess we'll just have to live with this slight discrepancy with the
possible_crtcs. We could make the encoder<->pipe assignment totally
flexible but that wouldn't actually change anything. We still get to
pick one based on whatever reason we can think of, and using the pipe
for that seems as good a reason as any.

What we could try to remove is having separate MST encoders for each
DDI port. But that would make encoder->port not work for MST again. So
defintiely has downsides, and doens't do anything for the possible_crtcs
thing.

Bah. I'll just drop the TODO.

> +	 */
>  	intel_encoder->pipe_mask = ~0;
>  
>  	intel_encoder->compute_config = intel_dp_mst_compute_config;
> -- 
> 2.21.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 7be82cf926ca..cb3047fe2d02 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -615,6 +615,18 @@  intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 	intel_encoder->power_domain = intel_dig_port->base.power_domain;
 	intel_encoder->port = intel_dig_port->base.port;
 	intel_encoder->cloneable = 0;
+	/*
+	 * This is wrong, but broken userspace uses the intersection
+	 * of possible_crtcs of all the encoders of a given connector
+	 * to figure out which crtcs can drive said connector. What
+	 * should be used instead is the union of possible_crtcs.
+	 * To keep such userspace functioning we must misconfigure
+	 * this to make sure the intersection is not empty :(
+	 *
+	 * TODO: figure out if we could eliminate the per-pipe
+	 * encoders here and just have a single encoder for each
+	 * MST connector. That would sidestep the userspace bug.
+	 */
 	intel_encoder->pipe_mask = ~0;
 
 	intel_encoder->compute_config = intel_dp_mst_compute_config;