[3/5] drm/crtc: take references to connectors used in a modeset.
diff mbox

Message ID 1461722609-32474-3-git-send-email-airlied@gmail.com
State New
Headers show

Commit Message

Dave Airlie April 27, 2016, 2:03 a.m. UTC
From: Dave Airlie <airlied@redhat.com>

This just takes a reference on the connector when we set a mode
in the non-atomic paths.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Daniel Stone April 27, 2016, 7:07 a.m. UTC | #1
Hi,

On 27 April 2016 at 03:03, Dave Airlie <airlied@gmail.com> wrote:
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..29b7835 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>                          * between them is henceforth no longer available.
>                          */
>                         connector->dpms = DRM_MODE_DPMS_OFF;
> +
> +                       /* we keep a reference while the encoder is bound */
> +                       drm_connector_unreference(connector);
>                 }
>         }
>
> @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>                         mode_changed = true;
>                         /* If the encoder is reused for another connector, then
>                          * the appropriate crtc will be set later.
> +                        * take a reference only if we haven't had an encoder before.
>                          */
>                         if (connector->encoder)
>                                 connector->encoder->crtc = NULL;
> +                       else
> +                               drm_connector_reference(connector);
>                         connector->encoder = new_encoder;
>                 }
>         }

This new ref leaks in the if (fail) goto fail path below, and I can't
quite convince myself that it's correct in the case where they share
an encoder. How about this:
  - unconditionally ref every connector in set->connectors before
doing anything which can fail
  - on successful exit, unconditionally unref every connector in save_connectors
  - in the fail label, unconditionally unref every connector in set->connectors

Not quite as efficient as trying to do the only-ref-on-change dance,
but much easier to prove correct, as well as matching the atomic state
approach, if you imagine save_connectors as the current state, and
set->connectors as the new/req state. Plus, refcounting really isn't
the expensive part of this operation. ;)

Cheers,
Daniel
Daniel Vetter April 27, 2016, 7:34 a.m. UTC | #2
On Wed, Apr 27, 2016 at 12:03:27PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This just takes a reference on the connector when we set a mode
> in the non-atomic paths.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..29b7835 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
>  			 * between them is henceforth no longer available.
>  			 */
>  			connector->dpms = DRM_MODE_DPMS_OFF;
> +
> +			/* we keep a reference while the encoder is bound */
> +			drm_connector_unreference(connector);
>  		}
>  	}
>  
> @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
>  			mode_changed = true;
>  			/* If the encoder is reused for another connector, then
>  			 * the appropriate crtc will be set later.
> +			 * take a reference only if we haven't had an encoder before.
>  			 */
>  			if (connector->encoder)
>  				connector->encoder->crtc = NULL;
> +			else
> +				drm_connector_reference(connector);
>  			connector->encoder = new_encoder;
>  		}
>  	}
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter April 27, 2016, 7:48 a.m. UTC | #3
On Wed, Apr 27, 2016 at 09:34:34AM +0200, Daniel Vetter wrote:
> On Wed, Apr 27, 2016 at 12:03:27PM +1000, Dave Airlie wrote:
> > From: Dave Airlie <airlied@redhat.com>
> > 
> > This just takes a reference on the connector when we set a mode
> > in the non-atomic paths.
> > 
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok I feel real silly now reading Daniel Stone's reply ;-) r-b retracted
...

Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index 66ca313..29b7835 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
> >  			 * between them is henceforth no longer available.
> >  			 */
> >  			connector->dpms = DRM_MODE_DPMS_OFF;
> > +
> > +			/* we keep a reference while the encoder is bound */
> > +			drm_connector_unreference(connector);
> >  		}
> >  	}
> >  
> > @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> >  			mode_changed = true;
> >  			/* If the encoder is reused for another connector, then
> >  			 * the appropriate crtc will be set later.
> > +			 * take a reference only if we haven't had an encoder before.
> >  			 */
> >  			if (connector->encoder)
> >  				connector->encoder->crtc = NULL;
> > +			else
> > +				drm_connector_reference(connector);
> >  			connector->encoder = new_encoder;
> >  		}
> >  	}
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 66ca313..29b7835 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -456,6 +456,9 @@  drm_crtc_helper_disable(struct drm_crtc *crtc)
 			 * between them is henceforth no longer available.
 			 */
 			connector->dpms = DRM_MODE_DPMS_OFF;
+
+			/* we keep a reference while the encoder is bound */
+			drm_connector_unreference(connector);
 		}
 	}
 
@@ -635,9 +638,12 @@  int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			mode_changed = true;
 			/* If the encoder is reused for another connector, then
 			 * the appropriate crtc will be set later.
+			 * take a reference only if we haven't had an encoder before.
 			 */
 			if (connector->encoder)
 				connector->encoder->crtc = NULL;
+			else
+				drm_connector_reference(connector);
 			connector->encoder = new_encoder;
 		}
 	}