| Message ID | 1451908400-25147-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1e42309ec40a..b76778d76035 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); > crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > + crtc->base.state->connector_mask = 0; > > /* Because we only establish the connector -> encoder -> > * crtc links if something is active, this means the > @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) > { > struct intel_connector *connector; > struct drm_device *dev = encoder->base.dev; > + struct drm_crtc *crtc = encoder->base.crtc; > bool active = false; > > /* We need to check both for a crtc link (meaning that the > * encoder is active and trying to read from a pipe) and the > * pipe itself being active. */ > - bool has_active_crtc = encoder->base.crtc && > - to_intel_crtc(encoder->base.crtc)->active; > + bool has_active_crtc = crtc && crtc->state->active; > > for_each_intel_connector(dev, connector) { > if (connector->base.encoder != &encoder->base) > continue; > > active = true; > - break; > + if (!has_active_crtc) > + break; > + > + crtc->state->connector_mask |= > + 1 << drm_connector_index(&connector->base); I still think this is the wrong place. Imo this should be done in intel_modeset_update_connector_atomic_state. It'd be great if we could somehow share the logic with drm_atomic_set_crtc_for_connector even, but that's probably over the top. -Daniel > } > > if (active && !has_active_crtc) { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 05-01-16 om 09:35 schreef Daniel Vetter: > On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote: >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 1e42309ec40a..b76778d76035 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) >> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); >> crtc->base.state->active = crtc->active; >> crtc->base.enabled = crtc->active; >> + crtc->base.state->connector_mask = 0; >> >> /* Because we only establish the connector -> encoder -> >> * crtc links if something is active, this means the >> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) >> { >> struct intel_connector *connector; >> struct drm_device *dev = encoder->base.dev; >> + struct drm_crtc *crtc = encoder->base.crtc; >> bool active = false; >> >> /* We need to check both for a crtc link (meaning that the >> * encoder is active and trying to read from a pipe) and the >> * pipe itself being active. */ >> - bool has_active_crtc = encoder->base.crtc && >> - to_intel_crtc(encoder->base.crtc)->active; >> + bool has_active_crtc = crtc && crtc->state->active; >> >> for_each_intel_connector(dev, connector) { >> if (connector->base.encoder != &encoder->base) >> continue; >> >> active = true; >> - break; >> + if (!has_active_crtc) >> + break; >> + >> + crtc->state->connector_mask |= >> + 1 << drm_connector_index(&connector->base); > I still think this is the wrong place. Imo this should be done in > intel_modeset_update_connector_atomic_state. It'd be great if we could > somehow share the logic with drm_atomic_set_crtc_for_connector even, but > that's probably over the top. > No it should be done sooner. I want to be able use it anywhere in the .crtc_disable calls without worrying about it.. ~Maarten
On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote: > Op 05-01-16 om 09:35 schreef Daniel Vetter: > > On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote: > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 1e42309ec40a..b76778d76035 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > >> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); > >> crtc->base.state->active = crtc->active; > >> crtc->base.enabled = crtc->active; > >> + crtc->base.state->connector_mask = 0; > >> > >> /* Because we only establish the connector -> encoder -> > >> * crtc links if something is active, this means the > >> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) > >> { > >> struct intel_connector *connector; > >> struct drm_device *dev = encoder->base.dev; > >> + struct drm_crtc *crtc = encoder->base.crtc; > >> bool active = false; > >> > >> /* We need to check both for a crtc link (meaning that the > >> * encoder is active and trying to read from a pipe) and the > >> * pipe itself being active. */ > >> - bool has_active_crtc = encoder->base.crtc && > >> - to_intel_crtc(encoder->base.crtc)->active; > >> + bool has_active_crtc = crtc && crtc->state->active; > >> > >> for_each_intel_connector(dev, connector) { > >> if (connector->base.encoder != &encoder->base) > >> continue; > >> > >> active = true; > >> - break; > >> + if (!has_active_crtc) > >> + break; > >> + > >> + crtc->state->connector_mask |= > >> + 1 << drm_connector_index(&connector->base); > > I still think this is the wrong place. Imo this should be done in > > intel_modeset_update_connector_atomic_state. It'd be great if we could > > somehow share the logic with drm_atomic_set_crtc_for_connector even, but > > that's probably over the top. > > > No it should be done sooner. I want to be able use it anywhere in the > .crtc_disable calls without worrying about it.. Well I don't want to split things up all over. Atm our state recover is a complete mess, and we need to start recovering some order in it. Updating related things at completely different places without even a comment stating why that's required is imo a no-go. -Daniel
Op 05-01-16 om 10:10 schreef Daniel Vetter: > On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote: >> Op 05-01-16 om 09:35 schreef Daniel Vetter: >>> On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote: >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 1e42309ec40a..b76778d76035 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) >>>> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); >>>> crtc->base.state->active = crtc->active; >>>> crtc->base.enabled = crtc->active; >>>> + crtc->base.state->connector_mask = 0; >>>> >>>> /* Because we only establish the connector -> encoder -> >>>> * crtc links if something is active, this means the >>>> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) >>>> { >>>> struct intel_connector *connector; >>>> struct drm_device *dev = encoder->base.dev; >>>> + struct drm_crtc *crtc = encoder->base.crtc; >>>> bool active = false; >>>> >>>> /* We need to check both for a crtc link (meaning that the >>>> * encoder is active and trying to read from a pipe) and the >>>> * pipe itself being active. */ >>>> - bool has_active_crtc = encoder->base.crtc && >>>> - to_intel_crtc(encoder->base.crtc)->active; >>>> + bool has_active_crtc = crtc && crtc->state->active; >>>> >>>> for_each_intel_connector(dev, connector) { >>>> if (connector->base.encoder != &encoder->base) >>>> continue; >>>> >>>> active = true; >>>> - break; >>>> + if (!has_active_crtc) >>>> + break; >>>> + >>>> + crtc->state->connector_mask |= >>>> + 1 << drm_connector_index(&connector->base); >>> I still think this is the wrong place. Imo this should be done in >>> intel_modeset_update_connector_atomic_state. It'd be great if we could >>> somehow share the logic with drm_atomic_set_crtc_for_connector even, but >>> that's probably over the top. >>> >> No it should be done sooner. I want to be able use it anywhere in the >> .crtc_disable calls without worrying about it.. > Well I don't want to split things up all over. Atm our state recover is a > complete mess, and we need to start recovering some order in it. Updating > related things at completely different places without even a comment > stating why that's required is imo a no-go. Ok so if I resubmit with a comment in this hunk it will be acceptable? ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; + crtc->base.state->connector_mask = 0; /* Because we only establish the connector -> encoder -> * crtc links if something is active, this means the @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev; + struct drm_crtc *crtc = encoder->base.crtc; bool active = false; /* We need to check both for a crtc link (meaning that the * encoder is active and trying to read from a pipe) and the * pipe itself being active. */ - bool has_active_crtc = encoder->base.crtc && - to_intel_crtc(encoder->base.crtc)->active; + bool has_active_crtc = crtc && crtc->state->active; for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue; active = true; - break; + if (!has_active_crtc) + break; + + crtc->state->connector_mask |= + 1 << drm_connector_index(&connector->base); } if (active && !has_active_crtc) {
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)