Revert "drm/i915: Fix DP-MST crtc_mask"
diff mbox series

Message ID 20190903154018.26357-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • Revert "drm/i915: Fix DP-MST crtc_mask"
Related show

Commit Message

Ville Syrjälä Sept. 3, 2019, 3:40 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit 4eaceea3a00f8e936a7f48dcd0c975a57f88930f.

Several userspace clients (modesetting ddx and mutter+wayland at least)
handle encoder.possible_crtcs incorrectly. What they essentially do is
the following:

possible_crtcs = ~0;
for_each_possible_encoder(connector)
	possible_crtcs &= encoder->possible_crtcs;

Ie. they calculate the intersection of the possible_crtcs
for the connector when they really should be calculating the
union instead.

In our case each MST encoder now has just one unique bit set,
and so the intersection is always zero. The end result is that
MST connectors can't be lit up because no crtc can be found to
drive them.

I've submitted a fix for the modesetting ddx [1], and complained
on #wayland about mutter, so hopefully the situation will improve
in the future. In the meantime we have regression, and so must go
back to the old way of misconfiguring possible_crtcs in the kernel.

[1] https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277

Cc: Jonas Ådahl <jadahl@gmail.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111507
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

José Roberto de Souza Sept. 3, 2019, 8:10 p.m. UTC | #1
On Tue, 2019-09-03 at 18:40 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit 4eaceea3a00f8e936a7f48dcd0c975a57f88930f.
> 
> Several userspace clients (modesetting ddx and mutter+wayland at
> least)
> handle encoder.possible_crtcs incorrectly. What they essentially do
> is
> the following:
> 
> possible_crtcs = ~0;
> for_each_possible_encoder(connector)
> 	possible_crtcs &= encoder->possible_crtcs;
> 
> Ie. they calculate the intersection of the possible_crtcs
> for the connector when they really should be calculating the
> union instead.
> 
> In our case each MST encoder now has just one unique bit set,
> and so the intersection is always zero. The end result is that
> MST connectors can't be lit up because no crtc can be found to
> drive them.
> 
> I've submitted a fix for the modesetting ddx [1], and complained
> on #wayland about mutter, so hopefully the situation will improve
> in the future. In the meantime we have regression, and so must go
> back to the old way of misconfiguring possible_crtcs in the kernel.

In the meantime are you planing to send a patch doing:

for_each_pipe(dev_priv, pipe)
		intel_encoder->crtc_mask |= BIT(pipe);

We had a patch doing that in one of the TGL enabling series but it was
dropped because of your patch looked better, I can bring it back if you
are not planning.

> 
> [1] https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277

Just looked to the merge request above, not to the other userspace
clients

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Cc: Jonas Ådahl <jadahl@gmail.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111507
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 6df240a01b8c..37366f81255b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -615,7 +615,7 @@ intel_dp_create_fake_mst_encoder(struct
> intel_digital_port *intel_dig_port, enum
>  	intel_encoder->type = INTEL_OUTPUT_DP_MST;
>  	intel_encoder->power_domain = intel_dig_port-
> >base.power_domain;
>  	intel_encoder->port = intel_dig_port->base.port;
> -	intel_encoder->crtc_mask = BIT(pipe);
> +	intel_encoder->crtc_mask = 0x7;
>  	intel_encoder->cloneable = 0;
>  
>  	intel_encoder->compute_config = intel_dp_mst_compute_config;
Ville Syrjälä Sept. 4, 2019, 2:02 p.m. UTC | #2
On Tue, Sep 03, 2019 at 08:10:18PM +0000, Souza, Jose wrote:
> On Tue, 2019-09-03 at 18:40 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > This reverts commit 4eaceea3a00f8e936a7f48dcd0c975a57f88930f.
> > 
> > Several userspace clients (modesetting ddx and mutter+wayland at
> > least)
> > handle encoder.possible_crtcs incorrectly. What they essentially do
> > is
> > the following:
> > 
> > possible_crtcs = ~0;
> > for_each_possible_encoder(connector)
> > 	possible_crtcs &= encoder->possible_crtcs;
> > 
> > Ie. they calculate the intersection of the possible_crtcs
> > for the connector when they really should be calculating the
> > union instead.
> > 
> > In our case each MST encoder now has just one unique bit set,
> > and so the intersection is always zero. The end result is that
> > MST connectors can't be lit up because no crtc can be found to
> > drive them.
> > 
> > I've submitted a fix for the modesetting ddx [1], and complained
> > on #wayland about mutter, so hopefully the situation will improve
> > in the future. In the meantime we have regression, and so must go
> > back to the old way of misconfiguring possible_crtcs in the kernel.
> 
> In the meantime are you planing to send a patch doing:
> 
> for_each_pipe(dev_priv, pipe)
> 		intel_encoder->crtc_mask |= BIT(pipe);
> 
> We had a patch doing that in one of the TGL enabling series but it was
> dropped because of your patch looked better, I can bring it back if you
> are not planning.

Yeah, please resend your version.

> 
> > 
> > [1] https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277
> 
> Just looked to the merge request above, not to the other userspace
> clients
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

Ta.

> 
> > 
> > Cc: Jonas Ådahl <jadahl@gmail.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111507
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 6df240a01b8c..37366f81255b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -615,7 +615,7 @@ intel_dp_create_fake_mst_encoder(struct
> > intel_digital_port *intel_dig_port, enum
> >  	intel_encoder->type = INTEL_OUTPUT_DP_MST;
> >  	intel_encoder->power_domain = intel_dig_port-
> > >base.power_domain;
> >  	intel_encoder->port = intel_dig_port->base.port;
> > -	intel_encoder->crtc_mask = BIT(pipe);
> > +	intel_encoder->crtc_mask = 0x7;
> >  	intel_encoder->cloneable = 0;
> >  
> >  	intel_encoder->compute_config = intel_dp_mst_compute_config;
Ville Syrjälä Sept. 4, 2019, 3:57 p.m. UTC | #3
On Tue, Sep 03, 2019 at 09:29:35PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: Revert "drm/i915: Fix DP-MST crtc_mask"
> URL   : https://patchwork.freedesktop.org/series/66173/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_6828_full -> Patchwork_14266_full
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_14266_full absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_14266_full, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_14266_full:
> 
> ### Piglit changes ###
> 
> #### Possible regressions ####
> 
>   * spec@ext_framebuffer_multisample@alpha-to-coverage-no-draw-buffer-zero 4 (NEW):
>     - pig-hsw-4770r:      NOTRUN -> [CRASH][1] +52 similar issues
>    [1]: None

Can't think why a display related patch would affect that. Pushed to dinq.

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 6df240a01b8c..37366f81255b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -615,7 +615,7 @@  intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 	intel_encoder->type = INTEL_OUTPUT_DP_MST;
 	intel_encoder->power_domain = intel_dig_port->base.power_domain;
 	intel_encoder->port = intel_dig_port->base.port;
-	intel_encoder->crtc_mask = BIT(pipe);
+	intel_encoder->crtc_mask = 0x7;
 	intel_encoder->cloneable = 0;
 
 	intel_encoder->compute_config = intel_dp_mst_compute_config;