* [PATCH] drm/i915: Fix broken mst get_hw_state.
@ 2015-08-27 11:13 Maarten Lankhorst
2015-08-29 20:54 ` shuang.he
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Maarten Lankhorst @ 2015-08-27 11:13 UTC (permalink / raw)
To: Intel Graphics Development
connector->encoder is initialized as NULL. Fix this by setting it in
during pre enable. MST connectors are not read out during initial hw
readout, and have no fixed encoder mappings. So it's harmless to
return false when the connector has never been assigned to an encoder.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 738178a0ac96..e3922d973db0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
connector->base.name);
if (connector->get_hw_state(connector)) {
- struct drm_encoder *encoder = &connector->encoder->base;
+ struct intel_encoder *encoder = connector->encoder;
struct drm_connector_state *conn_state = connector->base.state;
I915_STATE_WARN(!crtc,
@@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
I915_STATE_WARN(!crtc->state->active,
"connector is active, but attached crtc isn't\n");
- if (!encoder)
+ if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
return;
- I915_STATE_WARN(conn_state->best_encoder != encoder,
+ I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
"atomic encoder doesn't match attached encoder\n");
- I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+ I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
"attached encoder crtc differs from connector crtc\n");
} else {
I915_STATE_WARN(crtc && crtc->state->active,
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index ebf205462631..d606721b1788 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
return;
}
+ /* MST encoders are bound to a crtc, not to a connector,
+ * force the mapping here for get_hw_state.
+ */
+ found->encoder = encoder;
+
DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
intel_mst->port = found->port;
@@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
{
- if (connector->encoder) {
+ if (connector->encoder && connector->base.state->crtc) {
enum pipe pipe;
if (!connector->encoder->get_hw_state(connector->encoder, &pipe))
return false;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-08-27 11:13 [PATCH] drm/i915: Fix broken mst get_hw_state Maarten Lankhorst
@ 2015-08-29 20:54 ` shuang.he
2015-09-02 14:07 ` Ander Conselvan De Oliveira
2015-09-04 7:48 ` Daniel Vetter
2 siblings, 0 replies; 8+ messages in thread
From: shuang.he @ 2015-08-29 20:54 UTC (permalink / raw)
To: shuang.he, julianx.dumez, christophe.sureau, lei.a.liu, intel-gfx,
maarten.lankhorst
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7270
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 253/253 253/253
SNB 248/248 248/248
IVB 281/281 281/281
BYT 234/234 234/234
HSW 317/317 317/317
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-08-27 11:13 [PATCH] drm/i915: Fix broken mst get_hw_state Maarten Lankhorst
2015-08-29 20:54 ` shuang.he
@ 2015-09-02 14:07 ` Ander Conselvan De Oliveira
2015-09-02 14:14 ` Maarten Lankhorst
2015-09-04 7:48 ` Daniel Vetter
2 siblings, 1 reply; 8+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-09-02 14:07 UTC (permalink / raw)
To: Maarten Lankhorst, Intel Graphics Development
On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
> connector->encoder is initialized as NULL. Fix this by setting it in
> during pre enable. MST connectors are not read out during initial hw
> readout, and have no fixed encoder mappings. So it's harmless to
> return false when the connector has never been assigned to an encoder.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 738178a0ac96..e3922d973db0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
> connector->base.name);
>
> if (connector->get_hw_state(connector)) {
> - struct drm_encoder *encoder = &connector->encoder->base;
> + struct intel_encoder *encoder = connector->encoder;
> struct drm_connector_state *conn_state = connector->base.state;
>
> I915_STATE_WARN(!crtc,
> @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
> I915_STATE_WARN(!crtc->state->active,
> "connector is active, but attached crtc isn't\n");
>
> - if (!encoder)
> + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> return;
>
> - I915_STATE_WARN(conn_state->best_encoder != encoder,
> + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> "atomic encoder doesn't match attached encoder\n");
>
> - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> "attached encoder crtc differs from connector crtc\n");
> } else {
> I915_STATE_WARN(crtc && crtc->state->active,
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index ebf205462631..d606721b1788 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> return;
> }
>
> + /* MST encoders are bound to a crtc, not to a connector,
> + * force the mapping here for get_hw_state.
> + */
> + found->encoder = encoder;
> +
> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> intel_mst->port = found->port;
>
> @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>
> static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> {
> - if (connector->encoder) {
> + if (connector->encoder && connector->base.state->crtc) {
Is this here to cover the case where we disable this connector and assign the encoder to a different
connector? I guess this should work since if we disable the connector than crtc will be null, and
we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to set
connector->encoder back to NULL in post_disable.
Ander
> enum pipe pipe;
> if (!connector->encoder->get_hw_state(connector->encoder, &pipe))
> return false;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-09-02 14:07 ` Ander Conselvan De Oliveira
@ 2015-09-02 14:14 ` Maarten Lankhorst
2015-09-02 14:50 ` Daniel Vetter
2015-09-03 9:15 ` Ander Conselvan De Oliveira
0 siblings, 2 replies; 8+ messages in thread
From: Maarten Lankhorst @ 2015-09-02 14:14 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, Intel Graphics Development
Op 02-09-15 om 16:07 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
>> connector->encoder is initialized as NULL. Fix this by setting it in
>> during pre enable. MST connectors are not read out during initial hw
>> readout, and have no fixed encoder mappings. So it's harmless to
>> return false when the connector has never been assigned to an encoder.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 738178a0ac96..e3922d973db0 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
>> connector->base.name);
>>
>> if (connector->get_hw_state(connector)) {
>> - struct drm_encoder *encoder = &connector->encoder->base;
>> + struct intel_encoder *encoder = connector->encoder;
>> struct drm_connector_state *conn_state = connector->base.state;
>>
>> I915_STATE_WARN(!crtc,
>> @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
>> I915_STATE_WARN(!crtc->state->active,
>> "connector is active, but attached crtc isn't\n");
>>
>> - if (!encoder)
>> + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
>> return;
>>
>> - I915_STATE_WARN(conn_state->best_encoder != encoder,
>> + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
>> "atomic encoder doesn't match attached encoder\n");
>>
>> - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
>> + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
>> "attached encoder crtc differs from connector crtc\n");
>> } else {
>> I915_STATE_WARN(crtc && crtc->state->active,
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index ebf205462631..d606721b1788 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>> return;
>> }
>>
>> + /* MST encoders are bound to a crtc, not to a connector,
>> + * force the mapping here for get_hw_state.
>> + */
>> + found->encoder = encoder;
>> +
>> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>> intel_mst->port = found->port;
>>
>> @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>>
>> static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
>> {
>> - if (connector->encoder) {
>> + if (connector->encoder && connector->base.state->crtc) {
> Is this here to cover the case where we disable this connector and assign the encoder to a different
> connector? I guess this should work since if we disable the connector than crtc will be null, and
> we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to set
> connector->encoder back to NULL in post_disable.
>
Yes, it's harmless to keep it non-null if conn_state->crtc is checked though. Saves another pointless loop.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-09-02 14:14 ` Maarten Lankhorst
@ 2015-09-02 14:50 ` Daniel Vetter
2015-09-03 9:15 ` Ander Conselvan De Oliveira
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-09-02 14:50 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Intel Graphics Development
On Wed, Sep 02, 2015 at 04:14:27PM +0200, Maarten Lankhorst wrote:
> Op 02-09-15 om 16:07 schreef Ander Conselvan De Oliveira:
> > On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
> >> connector->encoder is initialized as NULL. Fix this by setting it in
> >> during pre enable. MST connectors are not read out during initial hw
> >> readout, and have no fixed encoder mappings. So it's harmless to
> >> return false when the connector has never been assigned to an encoder.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 738178a0ac96..e3922d973db0 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
> >> connector->base.name);
> >>
> >> if (connector->get_hw_state(connector)) {
> >> - struct drm_encoder *encoder = &connector->encoder->base;
> >> + struct intel_encoder *encoder = connector->encoder;
> >> struct drm_connector_state *conn_state = connector->base.state;
> >>
> >> I915_STATE_WARN(!crtc,
> >> @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
> >> I915_STATE_WARN(!crtc->state->active,
> >> "connector is active, but attached crtc isn't\n");
> >>
> >> - if (!encoder)
> >> + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> >> return;
> >>
> >> - I915_STATE_WARN(conn_state->best_encoder != encoder,
> >> + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> >> "atomic encoder doesn't match attached encoder\n");
> >>
> >> - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> >> + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> >> "attached encoder crtc differs from connector crtc\n");
> >> } else {
> >> I915_STATE_WARN(crtc && crtc->state->active,
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> index ebf205462631..d606721b1788 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> >> return;
> >> }
> >>
> >> + /* MST encoders are bound to a crtc, not to a connector,
> >> + * force the mapping here for get_hw_state.
> >> + */
> >> + found->encoder = encoder;
> >> +
> >> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >> intel_mst->port = found->port;
> >>
> >> @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
> >>
> >> static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> >> {
> >> - if (connector->encoder) {
> >> + if (connector->encoder && connector->base.state->crtc) {
> > Is this here to cover the case where we disable this connector and assign the encoder to a different
> > connector? I guess this should work since if we disable the connector than crtc will be null, and
> > we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to set
> > connector->encoder back to NULL in post_disable.
> >
> Yes, it's harmless to keep it non-null if conn_state->crtc is checked though. Saves another pointless loop.
Hm in get_hw_state functions we really shouldn't look at state pointers
like connector->encoder or state->crtc at all. It /should/ be all
free-standing for fastboot to actually work.
But I guess that's a lot more than just one patch ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-09-02 14:14 ` Maarten Lankhorst
2015-09-02 14:50 ` Daniel Vetter
@ 2015-09-03 9:15 ` Ander Conselvan De Oliveira
2015-09-09 8:14 ` Jani Nikula
1 sibling, 1 reply; 8+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-09-03 9:15 UTC (permalink / raw)
To: Maarten Lankhorst, Intel Graphics Development
On Wed, 2015-09-02 at 16:14 +0200, Maarten Lankhorst wrote:
> Op 02-09-15 om 16:07 schreef Ander Conselvan De Oliveira:
> > On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
> > > connector->encoder is initialized as NULL. Fix this by setting it in
> > > during pre enable. MST connectors are not read out during initial hw
> > > readout, and have no fixed encoder mappings. So it's harmless to
> > > return false when the connector has never been assigned to an encoder.
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 738178a0ac96..e3922d973db0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector
> > > *connector)
> > > connector->base.name);
> > >
> > > if (connector->get_hw_state(connector)) {
> > > - struct drm_encoder *encoder = &connector->encoder->base;
> > > + struct intel_encoder *encoder = connector->encoder;
> > > struct drm_connector_state *conn_state = connector->base.state;
> > >
> > > I915_STATE_WARN(!crtc,
> > > @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector
> > > *connector)
> > > I915_STATE_WARN(!crtc->state->active,
> > > "connector is active, but attached crtc isn't\n");
> > >
> > > - if (!encoder)
> > > + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> > > return;
> > >
> > > - I915_STATE_WARN(conn_state->best_encoder != encoder,
> > > + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> > > "atomic encoder doesn't match attached encoder\n");
> > >
> > > - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> > > + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> > > "attached encoder crtc differs from connector crtc\n");
> > > } else {
> > > I915_STATE_WARN(crtc && crtc->state->active,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index ebf205462631..d606721b1788 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> > > return;
> > > }
> > >
> > > + /* MST encoders are bound to a crtc, not to a connector,
> > > + * force the mapping here for get_hw_state.
> > > + */
> > > + found->encoder = encoder;
> > > +
> > > DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > intel_mst->port = found->port;
> > >
> > > @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
> > >
> > > static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> > > {
> > > - if (connector->encoder) {
> > > + if (connector->encoder && connector->base.state->crtc) {
> > Is this here to cover the case where we disable this connector and assign the encoder to a
> > different
> > connector? I guess this should work since if we disable the connector than crtc will be null,
> > and
> > we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to
> > set
> > connector->encoder back to NULL in post_disable.
> >
> Yes, it's harmless to keep it non-null if conn_state->crtc is checked though. Saves another
> pointless loop.
Fair enough.
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-09-03 9:15 ` Ander Conselvan De Oliveira
@ 2015-09-09 8:14 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2015-09-09 8:14 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, Maarten Lankhorst,
Intel Graphics Development
On Thu, 03 Sep 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> On Wed, 2015-09-02 at 16:14 +0200, Maarten Lankhorst wrote:
>> Op 02-09-15 om 16:07 schreef Ander Conselvan De Oliveira:
>> > On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
>> > > connector->encoder is initialized as NULL. Fix this by setting it in
>> > > during pre enable. MST connectors are not read out during initial hw
>> > > readout, and have no fixed encoder mappings. So it's harmless to
>> > > return false when the connector has never been assigned to an encoder.
>> > >
>> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > > ---
>> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > > index 738178a0ac96..e3922d973db0 100644
>> > > --- a/drivers/gpu/drm/i915/intel_display.c
>> > > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > > @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector
>> > > *connector)
>> > > connector->base.name);
>> > >
>> > > if (connector->get_hw_state(connector)) {
>> > > - struct drm_encoder *encoder = &connector->encoder->base;
>> > > + struct intel_encoder *encoder = connector->encoder;
>> > > struct drm_connector_state *conn_state = connector->base.state;
>> > >
>> > > I915_STATE_WARN(!crtc,
>> > > @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector
>> > > *connector)
>> > > I915_STATE_WARN(!crtc->state->active,
>> > > "connector is active, but attached crtc isn't\n");
>> > >
>> > > - if (!encoder)
>> > > + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
>> > > return;
>> > >
>> > > - I915_STATE_WARN(conn_state->best_encoder != encoder,
>> > > + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
>> > > "atomic encoder doesn't match attached encoder\n");
>> > >
>> > > - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
>> > > + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
>> > > "attached encoder crtc differs from connector crtc\n");
>> > > } else {
>> > > I915_STATE_WARN(crtc && crtc->state->active,
>> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> > > index ebf205462631..d606721b1788 100644
>> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> > > @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>> > > return;
>> > > }
>> > >
>> > > + /* MST encoders are bound to a crtc, not to a connector,
>> > > + * force the mapping here for get_hw_state.
>> > > + */
>> > > + found->encoder = encoder;
>> > > +
>> > > DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>> > > intel_mst->port = found->port;
>> > >
>> > > @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>> > >
>> > > static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
>> > > {
>> > > - if (connector->encoder) {
>> > > + if (connector->encoder && connector->base.state->crtc) {
>> > Is this here to cover the case where we disable this connector and assign the encoder to a
>> > different
>> > connector? I guess this should work since if we disable the connector than crtc will be null,
>> > and
>> > we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to
>> > set
>> > connector->encoder back to NULL in post_disable.
>> >
>> Yes, it's harmless to keep it non-null if conn_state->crtc is checked though. Saves another
>> pointless loop.
>
> Fair enough.
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Pushed to drm-intel-next-fixes, thanks for the patch and review.
BR,
Jani.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Fix broken mst get_hw_state.
2015-08-27 11:13 [PATCH] drm/i915: Fix broken mst get_hw_state Maarten Lankhorst
2015-08-29 20:54 ` shuang.he
2015-09-02 14:07 ` Ander Conselvan De Oliveira
@ 2015-09-04 7:48 ` Daniel Vetter
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-09-04 7:48 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Intel Graphics Development
On Thu, Aug 27, 2015 at 01:13:31PM +0200, Maarten Lankhorst wrote:
> connector->encoder is initialized as NULL. Fix this by setting it in
> during pre enable. MST connectors are not read out during initial hw
> readout, and have no fixed encoder mappings. So it's harmless to
> return false when the connector has never been assigned to an encoder.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Commit message fails to explain what exactly goes boom here. And without
that I can't decided whether this is for dinq or -fixes.
Also I'd like a few FIXMEs sprinkled since get_hw_state really shouldn't
look at software state at all.
-Daniel
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 738178a0ac96..e3922d973db0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
> connector->base.name);
>
> if (connector->get_hw_state(connector)) {
> - struct drm_encoder *encoder = &connector->encoder->base;
> + struct intel_encoder *encoder = connector->encoder;
> struct drm_connector_state *conn_state = connector->base.state;
>
> I915_STATE_WARN(!crtc,
> @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
> I915_STATE_WARN(!crtc->state->active,
> "connector is active, but attached crtc isn't\n");
>
> - if (!encoder)
> + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> return;
>
> - I915_STATE_WARN(conn_state->best_encoder != encoder,
> + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> "atomic encoder doesn't match attached encoder\n");
>
> - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> "attached encoder crtc differs from connector crtc\n");
> } else {
> I915_STATE_WARN(crtc && crtc->state->active,
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index ebf205462631..d606721b1788 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> return;
> }
>
> + /* MST encoders are bound to a crtc, not to a connector,
> + * force the mapping here for get_hw_state.
> + */
> + found->encoder = encoder;
> +
> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> intel_mst->port = found->port;
>
> @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
>
> static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> {
> - if (connector->encoder) {
> + if (connector->encoder && connector->base.state->crtc) {
> enum pipe pipe;
> if (!connector->encoder->get_hw_state(connector->encoder, &pipe))
> return false;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-09 8:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-27 11:13 [PATCH] drm/i915: Fix broken mst get_hw_state Maarten Lankhorst
2015-08-29 20:54 ` shuang.he
2015-09-02 14:07 ` Ander Conselvan De Oliveira
2015-09-02 14:14 ` Maarten Lankhorst
2015-09-02 14:50 ` Daniel Vetter
2015-09-03 9:15 ` Ander Conselvan De Oliveira
2015-09-09 8:14 ` Jani Nikula
2015-09-04 7:48 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox