Re: [Intel-gfx] [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> Instead of restoring dpms and a flag for whether a temp fb is allocated
> duplicate
> the old plane_state and crtc_state, and restore the members we potentially
> touched.
>
> Signed-off-by: Maarten Lankhorst
Do we have the load-detect igt testcase now in the BAT set? We've broken
this way too many times to not do that. Imo that should be done before
merging this fix.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 128
> ---
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> 2 files changed, 76 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
> if (obj->base.size < mode->vdisplay * fb->pitches[0])
> return NULL;
>
> + drm_framebuffer_reference(fb);
> return fb;
> #else
> return NULL;
> @@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
> encoder->base.id, encoder->name);
>
> retry:
> + old->old_pipe_config = NULL;
> + old->old_plane_state = NULL;
> +
> ret = drm_modeset_lock(&config->connection_mutex, ctx);
> if (ret)
> goto fail;
> @@ -10441,24 +10445,15 @@ retry:
>*/
>
> /* See if we already have a CRTC for this connector */
> - if (encoder->crtc) {
> - crtc = encoder->crtc;
> + if (connector->state->crtc) {
> + crtc = connector->state->crtc;
>
> ret = drm_modeset_lock(&crtc->mutex, ctx);
> if (ret)
> goto fail;
> - ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> - if (ret)
> - goto fail;
> -
> - old->dpms_mode = connector->dpms;
> - old->load_detect_temp = false;
>
> /* Make sure the crtc and connector are running */
> - if (connector->dpms != DRM_MODE_DPMS_ON)
> - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> -
> - return true;
> + goto found;
> }
>
> /* Find an unused one (if possible) */
> @@ -10466,8 +10461,15 @@ retry:
> i++;
> if (!(encoder->possible_crtcs & (1 << i)))
> continue;
> - if (possible_crtc->state->enable)
> +
> + ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> + if (ret)
> + goto fail;
> +
> + if (possible_crtc->state->enable) {
> + drm_modeset_unlock(&possible_crtc->mutex);
> continue;
> + }
>
> crtc = possible_crtc;
> break;
> @@ -10481,17 +10483,19 @@ retry:
> goto fail;
> }
>
> - ret = drm_modeset_lock(&crtc->mutex, ctx);
> - if (ret)
> - goto fail;
> +found:
> + intel_crtc = to_intel_crtc(crtc);
> +
> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> if (ret)
> goto fail;
>
> - intel_crtc = to_intel_crtc(crtc);
> - old->dpms_mode = connector->dpms;
> - old->load_detect_temp = true;
> - old->release_fb = NULL;
> + old->old_pipe_config = intel_crtc_duplicate_state(crtc);
> + old->old_plane_state = intel_plane_duplicate_state(crtc->primary);
> + if (!old->old_pipe_config || !old->old_plane_state) {
> + ret = -ENOMEM;
> + goto fail;
> + }
>
> state = drm_atomic_state_alloc(dev);
> if (!state)
> @@ -10505,7 +10509,9 @@ retry:
> goto fail;
> }
>
> - connector_state->crtc = crtc;
> + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
> + if (ret)
> + goto fail;
>
> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> if (IS_ERR(crtc_state)) {
> @@ -10529,7 +10535,6 @@ retry:
> if (fb == NULL) {
> DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
> fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
> - old->release_fb = fb;
> } else
> DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
> if (IS_ERR(fb)) {
> @@ -10541,15 +10546,16 @@ retry:
> if (ret)
> goto fail;
>
> - drm_mode_copy(&crtc_state->base.mode, mode);
> + drm_framebuffer_unreference(fb);
> +
> + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
> + if (ret)
> + goto fail;
>
> if (drm_atomic_commit(state)) {
> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
> - if (old->release_fb)
> - old
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
Op 01-02-16 om 18:09 schreef Ville Syrjälä:
> On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
>> Instead of restoring dpms and a flag for whether a temp fb is allocated
>> duplicate
>> the old plane_state and crtc_state, and restore the members we potentially
>> touched.
>>
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 128
>> ---
>> drivers/gpu/drm/i915/intel_drv.h | 4 +-
>> 2 files changed, 76 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4d8c9f7857db..0702ce8ec36a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
>> if (obj->base.size < mode->vdisplay * fb->pitches[0])
>> return NULL;
>>
>> +drm_framebuffer_reference(fb);
>> return fb;
>> #else
>> return NULL;
>> @@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>>encoder->base.id, encoder->name);
>>
>> retry:
>> +old->old_pipe_config = NULL;
>> +old->old_plane_state = NULL;
>> +
>> ret = drm_modeset_lock(&config->connection_mutex, ctx);
>> if (ret)
>> goto fail;
>> @@ -10441,24 +10445,15 @@ retry:
>> */
>>
>> /* See if we already have a CRTC for this connector */
>> -if (encoder->crtc) {
>> -crtc = encoder->crtc;
>> +if (connector->state->crtc) {
> All these connector->state accesses made me a bit uneasy, but we did
> indeed grab connection_mutex already so it should be fine. It's even
> more troubling seeing connector->state accessed outside
> intel_get_load_detect_pipe() in the later patches, but it seems it's
> only done if intel_get_load_detect_pipe() succeeded which means we
> should be holding the right lock.
>
> Dunno, maybe there should be some comments explaining this stuff.
> Or maybe maybe we should have a helper to return the current
> connector state that also asserts that the right lock is held?
I'm not sure how to proceed on that, I do think all accesses should be cleaned
up,
but I'm not sure yet what the path forward is, and so I want to leave it for a
different series.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
Op 01-02-16 om 17:08 schreef Ville Syrjälä:
> On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
>> Instead of restoring dpms and a flag for whether a temp fb is allocated
>> duplicate
>> the old plane_state and crtc_state, and restore the members we potentially
>> touched.
>>
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 128
>> ---
>> drivers/gpu/drm/i915/intel_drv.h | 4 +-
>> 2 files changed, 76 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4d8c9f7857db..0702ce8ec36a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
>> if (obj->base.size < mode->vdisplay * fb->pitches[0])
>> return NULL;
>>
>> +drm_framebuffer_reference(fb);
>> return fb;
>> #else
>> return NULL;
>> @@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>>encoder->base.id, encoder->name);
>>
>> retry:
>> +old->old_pipe_config = NULL;
>> +old->old_plane_state = NULL;
>> +
>> ret = drm_modeset_lock(&config->connection_mutex, ctx);
>> if (ret)
>> goto fail;
>> @@ -10441,24 +10445,15 @@ retry:
>> */
>>
>> /* See if we already have a CRTC for this connector */
>> -if (encoder->crtc) {
>> -crtc = encoder->crtc;
>> +if (connector->state->crtc) {
>> +crtc = connector->state->crtc;
>>
>> ret = drm_modeset_lock(&crtc->mutex, ctx);
>> if (ret)
>> goto fail;
>> -ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>> -if (ret)
>> -goto fail;
>> -
>> -old->dpms_mode = connector->dpms;
>> -old->load_detect_temp = false;
>>
>> /* Make sure the crtc and connector are running */
>> -if (connector->dpms != DRM_MODE_DPMS_ON)
>> -connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
>> -
>> -return true;
>> +goto found;
>> }
>>
>> /* Find an unused one (if possible) */
>> @@ -10466,8 +10461,15 @@ retry:
>> i++;
>> if (!(encoder->possible_crtcs & (1 << i)))
>> continue;
>> -if (possible_crtc->state->enable)
>> +
>> +ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
>> +if (ret)
>> +goto fail;
>> +
>> +if (possible_crtc->state->enable) {
>> +drm_modeset_unlock(&possible_crtc->mutex);
>> continue;
>> +}
>>
>> crtc = possible_crtc;
>> break;
>> @@ -10481,17 +10483,19 @@ retry:
>> goto fail;
>> }
>>
>> -ret = drm_modeset_lock(&crtc->mutex, ctx);
>> -if (ret)
>> -goto fail;
>> +found:
>> +intel_crtc = to_intel_crtc(crtc);
>> +
>> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
>> if (ret)
>> goto fail;
>>
>> -intel_crtc = to_intel_crtc(crtc);
>> -old->dpms_mode = connector->dpms;
>> -old->load_detect_temp = true;
>> -old->release_fb = NULL;
>> +old->old_pipe_config = intel_crtc_duplicate_state(crtc);
>> +old->old_plane_state = intel_plane_duplicate_state(crtc->primary);
>> +if (!old->old_pipe_config || !old->old_plane_state) {
>> +ret = -ENOMEM;
>> +goto fail;
>> +}
>>
>> state = drm_atomic_state_alloc(dev);
>> if (!state)
>> @@ -10505,7 +10509,9 @@ retry:
>> goto fail;
>> }
>>
>> -connector_state->crtc = crtc;
>> +ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
>> +if (ret)
>> +goto fail;
>>
>> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> if (IS_ERR(crtc_state)) {
>> @@ -10529,7 +10535,6 @@ retry:
>> if (fb == NULL) {
>> DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
>> fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
>> -old->release_fb = fb;
>> } else
>> DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
>> if (IS_ERR(fb)) {
>> @@ -10541,15 +10546,16 @@ retry:
>> if (ret)
>> goto fail;
>>
>> -drm_mode_copy(&crtc_state->base.mode, mode);
>> +drm_framebuffer_unreference(fb);
>> +
>> +ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
>> +if (ret)
>> +goto fail;
>>
>> if (drm_atomic_commit(state)) {
>> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>> -if (old->release_fb)
>> -old->release_fb->funcs->destroy(old->release_fb);
>> goto fail;
>>
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> Instead of restoring dpms and a flag for whether a temp fb is allocated
> duplicate
> the old plane_state and crtc_state, and restore the members we potentially
> touched.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_display.c | 128
> ---
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> 2 files changed, 76 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
> if (obj->base.size < mode->vdisplay * fb->pitches[0])
> return NULL;
>
> + drm_framebuffer_reference(fb);
> return fb;
> #else
> return NULL;
> @@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
> encoder->base.id, encoder->name);
>
> retry:
> + old->old_pipe_config = NULL;
> + old->old_plane_state = NULL;
> +
> ret = drm_modeset_lock(&config->connection_mutex, ctx);
> if (ret)
> goto fail;
> @@ -10441,24 +10445,15 @@ retry:
>*/
>
> /* See if we already have a CRTC for this connector */
> - if (encoder->crtc) {
> - crtc = encoder->crtc;
> + if (connector->state->crtc) {
All these connector->state accesses made me a bit uneasy, but we did
indeed grab connection_mutex already so it should be fine. It's even
more troubling seeing connector->state accessed outside
intel_get_load_detect_pipe() in the later patches, but it seems it's
only done if intel_get_load_detect_pipe() succeeded which means we
should be holding the right lock.
Dunno, maybe there should be some comments explaining this stuff.
Or maybe maybe we should have a helper to return the current
connector state that also asserts that the right lock is held?
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> Instead of restoring dpms and a flag for whether a temp fb is allocated
> duplicate
> the old plane_state and crtc_state, and restore the members we potentially
> touched.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_display.c | 128
> ---
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> 2 files changed, 76 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
> if (obj->base.size < mode->vdisplay * fb->pitches[0])
> return NULL;
>
> + drm_framebuffer_reference(fb);
> return fb;
> #else
> return NULL;
> @@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
> encoder->base.id, encoder->name);
>
> retry:
> + old->old_pipe_config = NULL;
> + old->old_plane_state = NULL;
> +
> ret = drm_modeset_lock(&config->connection_mutex, ctx);
> if (ret)
> goto fail;
> @@ -10441,24 +10445,15 @@ retry:
>*/
>
> /* See if we already have a CRTC for this connector */
> - if (encoder->crtc) {
> - crtc = encoder->crtc;
> + if (connector->state->crtc) {
> + crtc = connector->state->crtc;
>
> ret = drm_modeset_lock(&crtc->mutex, ctx);
> if (ret)
> goto fail;
> - ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> - if (ret)
> - goto fail;
> -
> - old->dpms_mode = connector->dpms;
> - old->load_detect_temp = false;
>
> /* Make sure the crtc and connector are running */
> - if (connector->dpms != DRM_MODE_DPMS_ON)
> - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> -
> - return true;
> + goto found;
> }
>
> /* Find an unused one (if possible) */
> @@ -10466,8 +10461,15 @@ retry:
> i++;
> if (!(encoder->possible_crtcs & (1 << i)))
> continue;
> - if (possible_crtc->state->enable)
> +
> + ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> + if (ret)
> + goto fail;
> +
> + if (possible_crtc->state->enable) {
> + drm_modeset_unlock(&possible_crtc->mutex);
> continue;
> + }
>
> crtc = possible_crtc;
> break;
> @@ -10481,17 +10483,19 @@ retry:
> goto fail;
> }
>
> - ret = drm_modeset_lock(&crtc->mutex, ctx);
> - if (ret)
> - goto fail;
> +found:
> + intel_crtc = to_intel_crtc(crtc);
> +
> ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> if (ret)
> goto fail;
>
> - intel_crtc = to_intel_crtc(crtc);
> - old->dpms_mode = connector->dpms;
> - old->load_detect_temp = true;
> - old->release_fb = NULL;
> + old->old_pipe_config = intel_crtc_duplicate_state(crtc);
> + old->old_plane_state = intel_plane_duplicate_state(crtc->primary);
> + if (!old->old_pipe_config || !old->old_plane_state) {
> + ret = -ENOMEM;
> + goto fail;
> + }
>
> state = drm_atomic_state_alloc(dev);
> if (!state)
> @@ -10505,7 +10509,9 @@ retry:
> goto fail;
> }
>
> - connector_state->crtc = crtc;
> + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
> + if (ret)
> + goto fail;
>
> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> if (IS_ERR(crtc_state)) {
> @@ -10529,7 +10535,6 @@ retry:
> if (fb == NULL) {
> DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
> fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
> - old->release_fb = fb;
> } else
> DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
> if (IS_ERR(fb)) {
> @@ -10541,15 +10546,16 @@ retry:
> if (ret)
> goto fail;
>
> - drm_mode_copy(&crtc_state->base.mode, mode);
> + drm_framebuffer_unreference(fb);
> +
> + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
> + if (ret)
> + goto fail;
>
> if (drm_atomic_commit(state)) {
> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
> - if (old->release_fb)
> - old->release_fb->funcs->destroy(old->release_fb);
> goto fail;
> }
> - crtc->primary->crtc = crtc;
>
> /* let the connector get through one ful
