| Message ID | 1436252911-5703-10-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 07, 2015 at 09:08:20AM +0200, Maarten Lankhorst wrote: > Perform a full readout of the state by making sure the mode is set > up correctly atomically. > > Also there was a small memory leak by doing the memset, fix this > by calling __drm_atomic_helper_crtc_destroy_state first. > > Changes since v1: > - Moved after reworking primary plane and updating mode members. > - Force a modeset calculation by changing mode->type from what the > final mode will be. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> tbh I don't really like mode_from_pipe_config since it goes in reverse. With the adjust mode of config_compare we can compare different final hw states and make a call whether they match enough for modeset avoidance or not. mode_from_pipe_config otoh is a lie on panels where we can do upscaling, hence I'd like to remove it to avoid confusion. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++------------------- > drivers/gpu/drm/i915/intel_fbdev.c | 9 ++------- > 2 files changed, 18 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ee543e1515f1..dc4bdb91ad4d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15191,6 +15191,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); > struct intel_encoder *encoder; > u32 reg; > bool enable; > @@ -15202,6 +15203,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > /* restore vblank interrupts to correct state */ > drm_crtc_vblank_reset(&crtc->base); > if (crtc->active) { > + drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); > update_scanline_offset(crtc); > drm_crtc_vblank_on(&crtc->base); > } > @@ -15254,9 +15256,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > crtc->base.state->enable ? "enabled" : "disabled", > crtc->active ? "enabled" : "disabled"); > > - crtc->base.state->enable = crtc->active; > - crtc->base.state->active = crtc->active; > - crtc->base.enabled = crtc->active; > + crtc->base.enabled = crtc->base.state->active = crtc->active; > + WARN_ON(drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL) < 0); > > /* Because we only establish the connector -> encoder -> > * crtc links if something is active, this means the > @@ -15412,6 +15413,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > int i; > > for_each_intel_crtc(dev, crtc) { > + __drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state); > memset(crtc->config, 0, sizeof(*crtc->config)); > crtc->config->base.crtc = &crtc->base; > > @@ -15420,11 +15422,21 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > crtc->active = dev_priv->display.get_pipe_config(crtc, > crtc->config); > > - crtc->base.state->enable = crtc->active; > crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > crtc->base.hwmode = crtc->config->base.adjusted_mode; > > + if (crtc->base.enabled) { > + memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > + intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); > + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode) < 0); > + > + /* make sure a initial modeset happens by making sure > + * mode->type will be different from the final mode > + */ > + crtc->base.state->mode.type = 0; > + } > + > readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); > > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > @@ -15501,21 +15513,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > > intel_modeset_readout_hw_state(dev); > > - /* > - * Now that we have the config, copy it to each CRTC struct > - * Note that this could go away if we move to using crtc_config > - * checking everywhere. > - */ > - for_each_intel_crtc(dev, crtc) { > - if (crtc->active && i915.fastboot) { > - intel_mode_from_pipe_config(&crtc->base.mode, > - crtc->config); > - DRM_DEBUG_KMS("[CRTC:%d] found active mode: ", > - crtc->base.base.id); > - drm_mode_debug_printmodeline(&crtc->base.mode); > - } > - } > - > /* HW state is read out, now we need to sanitize this mess. */ > for_each_intel_encoder(dev, encoder) { > intel_sanitize_encoder(encoder); > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 3d5bb56477ab..52d20cea182c 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -486,16 +486,11 @@ retry: > * config, not the input mode, which is what crtc->mode > * usually contains. But since our current fastboot > * code puts a mode derived from the post-pfit timings > - * into crtc->mode this works out correctly. We don't > - * use hwmode anywhere right now, so use it for this > - * since the fb helper layer wants a pointer to > - * something we own. > + * into crtc->mode this works out correctly. > */ > DRM_DEBUG_KMS("looking for current mode on connector %s\n", > connector->name); > - intel_mode_from_pipe_config(&encoder->crtc->hwmode, > - to_intel_crtc(encoder->crtc)->config); > - modes[i] = &encoder->crtc->hwmode; > + modes[i] = &encoder->crtc->mode; > } > crtcs[i] = new_crtc; > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 07-07-15 om 12:28 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 09:08:20AM +0200, Maarten Lankhorst wrote: >> Perform a full readout of the state by making sure the mode is set >> up correctly atomically. >> >> Also there was a small memory leak by doing the memset, fix this >> by calling __drm_atomic_helper_crtc_destroy_state first. >> >> Changes since v1: >> - Moved after reworking primary plane and updating mode members. >> - Force a modeset calculation by changing mode->type from what the >> final mode will be. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > tbh I don't really like mode_from_pipe_config since it goes in reverse. > With the adjust mode of config_compare we can compare different final hw > states and make a call whether they match enough for modeset avoidance or > not. mode_from_pipe_config otoh is a lie on panels where we can do > upscaling, hence I'd like to remove it to avoid confusion. What do you want the initial mode to be then? You need to fill in some mode to satisfy drm_atomic_crtc_check. ~Maarten
On Mon, Jul 13, 2015 at 11:32:09AM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 12:28 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 09:08:20AM +0200, Maarten Lankhorst wrote: > >> Perform a full readout of the state by making sure the mode is set > >> up correctly atomically. > >> > >> Also there was a small memory leak by doing the memset, fix this > >> by calling __drm_atomic_helper_crtc_destroy_state first. > >> > >> Changes since v1: > >> - Moved after reworking primary plane and updating mode members. > >> - Force a modeset calculation by changing mode->type from what the > >> final mode will be. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > tbh I don't really like mode_from_pipe_config since it goes in reverse. > > With the adjust mode of config_compare we can compare different final hw > > states and make a call whether they match enough for modeset avoidance or > > not. mode_from_pipe_config otoh is a lie on panels where we can do > > upscaling, hence I'd like to remove it to avoid confusion. > What do you want the initial mode to be then? > > You need to fill in some mode to satisfy drm_atomic_crtc_check. All zeros? That would make it clear that we have a mode, and that we also have no idea really what it is ... Otoh I think you've convinced me now that we indeed do need a real mode object here to avoid other troubles (i.e. the entire discussion around initial fbcon modesets). Given that I'd guess even the slightly wrong fill_in_modes here with the change to set DRIVER_MODE would be ok. As long as we can guarantee that we'll get a full modeset eventually we should be fine I hope. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ee543e1515f1..dc4bdb91ad4d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15191,6 +15191,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct intel_encoder *encoder; u32 reg; bool enable; @@ -15202,6 +15203,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* restore vblank interrupts to correct state */ drm_crtc_vblank_reset(&crtc->base); if (crtc->active) { + drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); update_scanline_offset(crtc); drm_crtc_vblank_on(&crtc->base); } @@ -15254,9 +15256,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) crtc->base.state->enable ? "enabled" : "disabled", crtc->active ? "enabled" : "disabled"); - crtc->base.state->enable = crtc->active; - crtc->base.state->active = crtc->active; - crtc->base.enabled = crtc->active; + crtc->base.enabled = crtc->base.state->active = crtc->active; + WARN_ON(drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL) < 0); /* Because we only establish the connector -> encoder -> * crtc links if something is active, this means the @@ -15412,6 +15413,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) int i; for_each_intel_crtc(dev, crtc) { + __drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state); memset(crtc->config, 0, sizeof(*crtc->config)); crtc->config->base.crtc = &crtc->base; @@ -15420,11 +15422,21 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->active = dev_priv->display.get_pipe_config(crtc, crtc->config); - crtc->base.state->enable = crtc->active; crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; crtc->base.hwmode = crtc->config->base.adjusted_mode; + if (crtc->base.enabled) { + memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); + intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode) < 0); + + /* make sure a initial modeset happens by making sure + * mode->type will be different from the final mode + */ + crtc->base.state->mode.type = 0; + } + readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", @@ -15501,21 +15513,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, intel_modeset_readout_hw_state(dev); - /* - * Now that we have the config, copy it to each CRTC struct - * Note that this could go away if we move to using crtc_config - * checking everywhere. - */ - for_each_intel_crtc(dev, crtc) { - if (crtc->active && i915.fastboot) { - intel_mode_from_pipe_config(&crtc->base.mode, - crtc->config); - DRM_DEBUG_KMS("[CRTC:%d] found active mode: ", - crtc->base.base.id); - drm_mode_debug_printmodeline(&crtc->base.mode); - } - } - /* HW state is read out, now we need to sanitize this mess. */ for_each_intel_encoder(dev, encoder) { intel_sanitize_encoder(encoder); diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 3d5bb56477ab..52d20cea182c 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -486,16 +486,11 @@ retry: * config, not the input mode, which is what crtc->mode * usually contains. But since our current fastboot * code puts a mode derived from the post-pfit timings - * into crtc->mode this works out correctly. We don't - * use hwmode anywhere right now, so use it for this - * since the fb helper layer wants a pointer to - * something we own. + * into crtc->mode this works out correctly. */ DRM_DEBUG_KMS("looking for current mode on connector %s\n", connector->name); - intel_mode_from_pipe_config(&encoder->crtc->hwmode, - to_intel_crtc(encoder->crtc)->config); - modes[i] = &encoder->crtc->hwmode; + modes[i] = &encoder->crtc->mode; } crtcs[i] = new_crtc;
Perform a full readout of the state by making sure the mode is set up correctly atomically. Also there was a small memory leak by doing the memset, fix this by calling __drm_atomic_helper_crtc_destroy_state first. Changes since v1: - Moved after reworking primary plane and updating mode members. - Force a modeset calculation by changing mode->type from what the final mode will be. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++------------------- drivers/gpu/drm/i915/intel_fbdev.c | 9 ++------- 2 files changed, 18 insertions(+), 26 deletions(-)