Re: [Intel-gfx] [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.
On Tue, Jul 07, 2015 at 03:14:57PM +0200, Daniel Vetter wrote: > On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote: > > Op 07-07-15 om 11:57 schreef Daniel Vetter: > > > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote: > > > Since we've only badly bruised escape this trap I think this deserves a > > > comment: > > > > > > /* > > >* WARNING: We can't do a full atomic modeset including > > >* compute/check phase here since especially encoder > > >* compute_config functions depend upon output detection state. > > >* And that's just not yet available at driver load. Therefore we > > >* must read out the entire relevant hw state (including any > > >* driver internal state) faithfully here and only apply the > > >* commit side. > > >*/ > > > > > > Hm, makes me think ... should we end up calling just > > > dev->atomic_commit(state) here > > > once atomic modeset is fully working? > > Not for initial hw readout unless you want to call detect in this function > > for all encoders.. resume's fine probably. > > I meant calling dev->mode_config.funcs->atomic_commit(state) directly, > without calling ->atomic_check at all. That should avoid any state > recomputation (otherwise our check/commit split is botched) and hence be > exactly what we need here. I didn't check how close intel_set_mode is > compared our ->atomic_commit implementation after this series (didn't > apply them all). But I think from a semantic pov those two should match. Ah I mixed up intel_set_mode with intel_crtc_set_config, which does a more elaborate compute_config. I guess this is something we still need to untangle when we replace all the existing legacy entry points with the legacy2atomic helpers. Probably needs some shuffling of responsibilities betwen atomic_check and atomic_commit. -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
Re: [Intel-gfx] [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.
On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 11:57 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote:
> >> @@ -10421,10 +10414,6 @@ void intel_release_load_detect_pipe(struct
> >> drm_connector *connector,
> >>if (IS_ERR(crtc_state))
> >>goto fail;
> >>
> >> - to_intel_connector(connector)->new_encoder = NULL;
> >> - intel_encoder->new_crtc = NULL;
> >> - intel_crtc->new_enabled = false;
> > load_detect changes should be a separate patch. Or the commit message
> > needs to explain why this needs to be one.
> These members no longer exist. ;-) all the new_ stuff was to restore things
> pre-atomic, the atomic updates are good enough here.
>
> Making it a separate patch's probably ok.
Yeah I think splitting out the new_* removal would address most of my
concerns here.
[snip]
> >> + struct intel_connector *conn;
> >> + struct intel_plane *plane;
> >> + struct drm_crtc *crtc;
> >> + int ret;
> >>
> >> - /*
> >> - * We need to use raw interfaces for restoring state to avoid
> >> - * checking (bogus) intermediate states.
> >> - */
> >> - for_each_pipe(dev_priv, pipe) {
> >> - struct drm_crtc *crtc =
> >> - dev_priv->pipe_to_crtc_mapping[pipe];
> >> + if (!state)
> > debug output missing that the state alloc failed. Perhaps just goto fail;
> > since state_free can cope with a NULL state.
> It can only fail because of kmalloc, which prints its own warnings.
Might still be useful just to have unified error reporting - you need to
guess the caller otherwise which would make debug (if this ever happesn)
harder. But really just a bikeshed.
>
> >> + return;
> >>
> >> - intel_crtc_restore_mode(crtc);
> >> - }
> >> - } else {
> >> - intel_modeset_update_staged_output_state(dev);
> >> + state->acquire_ctx = dev->mode_config.acquire_ctx;
> >> +
> >> + /* preserve complete old state, including dpll */
> >> + intel_atomic_get_shared_dpll_state(state);
> >> +
> >> + for_each_crtc(dev, crtc) {
> >> + struct drm_crtc_state *crtc_state =
> >> + drm_atomic_get_crtc_state(state, crtc);
> >> +
> >> + ret = PTR_ERR_OR_ZERO(crtc_state);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + /* force a restore */
> >> + crtc_state->mode_changed = true;
> >> + }
> >> +
> >> + for_each_intel_plane(dev, plane) {
> >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state,
> >> &plane->base));
> >> + if (ret)
> >> + goto err;
> >> + }
> >> +
> >> + for_each_intel_connector(dev, conn) {
> >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state,
> >> &conn->base));
> >> + if (ret)
> >> + goto err;
> >>}
> >>
> >> - intel_modeset_check_state(dev);
> >> + intel_modeset_setup_hw_state(dev);
> >> +
> >> + i915_redisable_vga(dev);
> > Since we've only badly bruised escape this trap I think this deserves a
> > comment:
> >
> > /*
> > * WARNING: We can't do a full atomic modeset including
> > * compute/check phase here since especially encoder
> > * compute_config functions depend upon output detection state.
> > * And that's just not yet available at driver load. Therefore we
> > * must read out the entire relevant hw state (including any
> > * driver internal state) faithfully here and only apply the
> > * commit side.
> > */
> >
> > Hm, makes me think ... should we end up calling just
> > dev->atomic_commit(state) here
> > once atomic modeset is fully working?
> Not for initial hw readout unless you want to call detect in this function
> for all encoders.. resume's fine probably.
I meant calling dev->mode_config.funcs->atomic_commit(state) directly,
without calling ->atomic_check at all. That should avoid any state
recomputation (otherwise our check/commit split is botched) and hence be
exactly what we need here. I didn't check how close intel_set_mode is
compared our ->atomic_commit implementation after this series (didn't
apply them all). But I think from a semantic pov those two should match.
-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
Re: [Intel-gfx] [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.
Op 07-07-15 om 11:57 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote:
>> Instead of all the ad-hoc updating, duplicate the old state first before
>> reading out the hw state, then restore it.
>>
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 2 +-
>> drivers/gpu/drm/i915/i915_drv.h | 3 +-
>> drivers/gpu/drm/i915/intel_display.c | 153
>> +--
>> drivers/gpu/drm/i915/intel_drv.h | 12 ---
>> drivers/gpu/drm/i915/intel_lvds.c| 2 +-
>> 5 files changed, 76 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index e44dc0d6656f..db48aee7f140 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev)
>> spin_unlock_irq(&dev_priv->irq_lock);
>>
>> drm_modeset_lock_all(dev);
>> -intel_modeset_setup_hw_state(dev, true);
>> +intel_display_resume(dev);
>> drm_modeset_unlock_all(dev);
>>
>> intel_dp_mst_resume(dev);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 093d6421dddf..2a78a0ee0f97 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3278,8 +3278,7 @@ extern void intel_modeset_gem_init(struct drm_device
>> *dev);
>> extern void intel_modeset_cleanup(struct drm_device *dev);
>> extern void intel_connector_unregister(struct intel_connector *);
>> extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
>> -extern void intel_modeset_setup_hw_state(struct drm_device *dev,
>> - bool force_restore);
>> +extern void intel_display_resume(struct drm_device *dev);
>> extern void i915_redisable_vga(struct drm_device *dev);
>> extern void i915_redisable_vga_power_on(struct drm_device *dev);
>> extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index dc4bdb91ad4d..222d587ed4ea 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev,
>> struct intel_crtc *intel_cr
>> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>> int num_connectors);
>> static void intel_pre_disable_primary(struct drm_crtc *crtc);
>> +static void intel_modeset_setup_hw_state(struct drm_device *dev);
>>
>> static struct intel_encoder *intel_find_encoder(struct intel_connector
>> *connector, int pipe)
>> {
>> @@ -3247,7 +3248,7 @@ void intel_finish_reset(struct drm_device *dev)
>> dev_priv->display.hpd_irq_setup(dev);
>> spin_unlock_irq(&dev_priv->irq_lock);
>>
>> -intel_modeset_setup_hw_state(dev, true);
>> +intel_display_resume(dev);
>>
>> intel_hpd_init(dev_priv);
>>
>> @@ -10239,7 +10240,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>> retry:
>> ret = drm_modeset_lock(&config->connection_mutex, ctx);
>> if (ret)
>> -goto fail_unlock;
>> +goto fail;
>>
>> /*
>> * Algorithm gets a little messy:
>> @@ -10257,10 +10258,10 @@ retry:
>>
>> ret = drm_modeset_lock(&crtc->mutex, ctx);
>> if (ret)
>> -goto fail_unlock;
>> +goto fail;
>> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>> if (ret)
>> -goto fail_unlock;
>> +goto fail;
>>
>> old->dpms_mode = connector->dpms;
>> old->load_detect_temp = false;
>> @@ -10279,9 +10280,6 @@ retry:
>> continue;
>> if (possible_crtc->state->enable)
>> continue;
>> -/* This can occur when applying the pipe A quirk on resume. */
>> -if (to_intel_crtc(possible_crtc)->new_enabled)
>> -continue;
>>
>> crtc = possible_crtc;
>> break;
>> @@ -10292,20 +10290,17 @@ retry:
>> */
>> if (!crtc) {
>> DRM_DEBUG_KMS("no pipe available for load-detect\n");
>> -goto fail_unlock;
>> +goto fail;
>> }
>>
>> ret = drm_modeset_lock(&crtc->mutex, ctx);
>> if (ret)
>> -goto fail_unlock;
>> +goto fail;
>> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>> if (ret)
>> -goto fail_unlock;
>> -intel_encoder->new_crtc = to_intel_crtc(crtc);
>> -to_intel_connector(connector)->new_encoder = intel_encoder;
>> +goto fail;
>>
>> intel_crtc = to_intel_crtc(crtc);
>> -intel_crtc->new_enabled = true;
>> old->dpms_mode = connector->dpms;
>> old->l
Re: [Intel-gfx] [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.
On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote:
> Instead of all the ad-hoc updating, duplicate the old state first before
> reading out the hw state, then restore it.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_display.c | 153
> +--
> drivers/gpu/drm/i915/intel_drv.h | 12 ---
> drivers/gpu/drm/i915/intel_lvds.c| 2 +-
> 5 files changed, 76 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e44dc0d6656f..db48aee7f140 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev)
> spin_unlock_irq(&dev_priv->irq_lock);
>
> drm_modeset_lock_all(dev);
> - intel_modeset_setup_hw_state(dev, true);
> + intel_display_resume(dev);
> drm_modeset_unlock_all(dev);
>
> intel_dp_mst_resume(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 093d6421dddf..2a78a0ee0f97 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3278,8 +3278,7 @@ extern void intel_modeset_gem_init(struct drm_device
> *dev);
> extern void intel_modeset_cleanup(struct drm_device *dev);
> extern void intel_connector_unregister(struct intel_connector *);
> extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
> -extern void intel_modeset_setup_hw_state(struct drm_device *dev,
> - bool force_restore);
> +extern void intel_display_resume(struct drm_device *dev);
> extern void i915_redisable_vga(struct drm_device *dev);
> extern void i915_redisable_vga_power_on(struct drm_device *dev);
> extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index dc4bdb91ad4d..222d587ed4ea 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -110,6 +110,7 @@ static void skl_init_scalers(struct drm_device *dev,
> struct intel_crtc *intel_cr
> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> int num_connectors);
> static void intel_pre_disable_primary(struct drm_crtc *crtc);
> +static void intel_modeset_setup_hw_state(struct drm_device *dev);
>
> static struct intel_encoder *intel_find_encoder(struct intel_connector
> *connector, int pipe)
> {
> @@ -3247,7 +3248,7 @@ void intel_finish_reset(struct drm_device *dev)
> dev_priv->display.hpd_irq_setup(dev);
> spin_unlock_irq(&dev_priv->irq_lock);
>
> - intel_modeset_setup_hw_state(dev, true);
> + intel_display_resume(dev);
>
> intel_hpd_init(dev_priv);
>
> @@ -10239,7 +10240,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
> retry:
> ret = drm_modeset_lock(&config->connection_mutex, ctx);
> if (ret)
> - goto fail_unlock;
> + goto fail;
>
> /*
>* Algorithm gets a little messy:
> @@ -10257,10 +10258,10 @@ retry:
>
> ret = drm_modeset_lock(&crtc->mutex, ctx);
> if (ret)
> - goto fail_unlock;
> + goto fail;
> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> if (ret)
> - goto fail_unlock;
> + goto fail;
>
> old->dpms_mode = connector->dpms;
> old->load_detect_temp = false;
> @@ -10279,9 +10280,6 @@ retry:
> continue;
> if (possible_crtc->state->enable)
> continue;
> - /* This can occur when applying the pipe A quirk on resume. */
> - if (to_intel_crtc(possible_crtc)->new_enabled)
> - continue;
>
> crtc = possible_crtc;
> break;
> @@ -10292,20 +10290,17 @@ retry:
>*/
> if (!crtc) {
> DRM_DEBUG_KMS("no pipe available for load-detect\n");
> - goto fail_unlock;
> + goto fail;
> }
>
> ret = drm_modeset_lock(&crtc->mutex, ctx);
> if (ret)
> - goto fail_unlock;
> + goto fail;
> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> if (ret)
> - goto fail_unlock;
> - intel_encoder->new_crtc = to_intel_crtc(crtc);
> - to_intel_connector(connector)->new_encoder = intel_encoder;
> + goto fail;
>
> intel_crtc = to_intel_crtc(crtc);
> - intel_crtc->new_enabled = true;
> old->dpms_mode = connector->dpms;
> old->load_detect_temp = true;
> old->release_fb = NULL;
> @@ -10373,9 +10368,7 @@ retry:
> intel_wait_for_vbla
