| Message ID | 20180615164925.28971-12-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Each fake MST encoder is tied to a specific pipe. Fix the encoder's > crtc_mask to reflect that fact. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > b/drivers/gpu/drm/i915/intel_dp_mst.c > index 5890500a3a8b..8e30765402b4 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -565,7 +565,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 = 0x7; > + intel_encoder->crtc_mask = BIT(pipe); How did this not cause any problems? Does this mean this field was/is unused? Disclaimer: I didn't look at the whole series. > intel_encoder->cloneable = 0; > > intel_encoder->compute_config = intel_dp_mst_compute_config;
On Fri, Jun 15, 2018 at 11:33:01AM -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Each fake MST encoder is tied to a specific pipe. Fix the encoder's > > crtc_mask to reflect that fact. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 5890500a3a8b..8e30765402b4 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -565,7 +565,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 = 0x7; > > + intel_encoder->crtc_mask = BIT(pipe); > > How did this not cause any problems? Does this mean this field was/is > unused? This is a hint to userspace. So userspace would pick the connector and crtc based on the hints, and then the kernel gets to pick the actual encoder. In this case the bogus hint was good enough to tell userspace that it can pick any crtc for any MST connector. Hmm. Why on earth do we have .atomic_best_encoder() and .best_encoder() for MST? The fb_helper appears to want to use the non-atomic one for some reason... On boy, I guess I'll need to do something about that. As is this patch would probably break it :(
On Fri, 2018-06-15 at 21:43 +0300, Ville Syrjälä wrote: > On Fri, Jun 15, 2018 at 11:33:01AM -0700, Dhinakaran Pandiyan wrote: > > > > On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Each fake MST encoder is tied to a specific pipe. Fix the > > > encoder's > > > crtc_mask to reflect that fact. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > > index 5890500a3a8b..8e30765402b4 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -565,7 +565,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 = 0x7; > > > + intel_encoder->crtc_mask = BIT(pipe); > > How did this not cause any problems? Does this mean this field > > was/is > > unused? > This is a hint to userspace. So userspace would pick the connector > and crtc based on the hints, and then the kernel gets to pick the > actual encoder. In this case the bogus hint was good enough to tell > userspace that it can pick any crtc for any MST connector. > > Hmm. Why on earth do we have .atomic_best_encoder() and > .best_encoder() > for MST? The fb_helper appears to want to use the non-atomic one for > some reason... On boy, I guess I'll need to do something about that. > As is this patch would probably break it :( > So we end up picking crtc-0 for all MST connectors with the same primary. Since this patch does the right thing and assuming https://patchwork.freedesktop.org/patch/229905/ gets merged before this Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 5890500a3a8b..8e30765402b4 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -565,7 +565,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 = 0x7; + intel_encoder->crtc_mask = BIT(pipe); intel_encoder->cloneable = 0; intel_encoder->compute_config = intel_dp_mst_compute_config;