Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.
Date: Tue, 2 Feb 2016 13:46:03 +0100	[thread overview]
Message-ID: <56B0A50B.1040509@linux.intel.com> (raw)
In-Reply-To: <20160201170929.GS23290@intel.com>

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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  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