[PATCH 1/4] drm/fb-helper: Fix connector ref leak on error

[PATCH 1/4] drm/fb-helper: Fix connector ref leak on error

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We need to drop the connector references already taken when we
abort in the middle of drm_fb_helper_single_add_all_connectors()

Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com>
Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 83dbae0fabcf..db469d12d195 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -131,7 +131,12 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	return 0;
 fail:
 	for (i = 0; i < fb_helper->connector_count; i++) {
-		kfree(fb_helper->connector_info[i]);
+		struct drm_fb_helper_connector *fb_helper_connector =
+			fb_helper->connector_info[i];
+
+		drm_connector_unreference(fb_helper_connector->connector);
+
+		kfree(fb_helper_connector);
 		fb_helper->connector_info[i] = NULL;
 	}
 	fb_helper->connector_count = 0;
-- 
2.7.4

Re: [PATCH 1/4] drm/fb-helper: Fix connector ref leak on error

[Chris Wilson]

On Wed, Oct 26, 2016 at 12:05:52PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> We need to drop the connector references already taken when we
> abort in the middle of drm_fb_helper_single_add_all_connectors()
> 
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/4] drm/fb-helper: Keep references for the current set of used connectors

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The fbdev helper code keeps around two lists of connectors. One is the
list of all connectors it could use, and that list already holds
references for all the connectors. However the other list, or rather
lists, is the one actively being used. That list is tracked per-crtc
and currently doesn't hold any extra references. Let's grab those
extra references to avoid oopsing when the connector vanishes. The
list of all possible connectors should get updated when the hpd happens,
but the list of actively used connectors would not get updated until
the next time the fb-helper picks through the set of possible connectors.
And so we need to hang on to the connectors until that time.

Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com>
Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index db469d12d195..9ee1dacf97b8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2090,7 +2090,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	struct drm_fb_helper_crtc **crtcs;
 	struct drm_display_mode **modes;
 	struct drm_fb_offset *offsets;
-	struct drm_mode_set *modeset;
 	bool *enabled;
 	int width, height;
 	int i;
@@ -2139,7 +2138,14 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
 	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
+		struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set;
+		int j;
+
+		for (j = 0; j < modeset->num_connectors; j++) {
+			drm_connector_unreference(modeset->connectors[j]);
+			modeset->connectors[j] = NULL;
+		}
+
 		modeset->num_connectors = 0;
 		modeset->fb = NULL;
 	}
@@ -2148,19 +2154,23 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
 		struct drm_fb_offset *offset = &offsets[i];
-		modeset = &fb_crtc->mode_set;
+		struct drm_mode_set *modeset = &fb_crtc->mode_set;
 
 		if (mode && fb_crtc) {
+			struct drm_connector *connector =
+				fb_helper->connector_info[i]->connector;
+
 			DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n",
 				      mode->name, fb_crtc->mode_set.crtc->base.id, offset->x, offset->y);
+
 			fb_crtc->desired_mode = mode;
 			fb_crtc->x = offset->x;
 			fb_crtc->y = offset->y;
 			if (modeset->mode)
 				drm_mode_destroy(dev, modeset->mode);
-			modeset->mode = drm_mode_duplicate(dev,
-							   fb_crtc->desired_mode);
-			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			modeset->mode = drm_mode_duplicate(dev, fb_crtc->desired_mode);
+			drm_connector_reference(connector);
+			modeset->connectors[modeset->num_connectors++] = connector;
 			modeset->fb = fb_helper->fb;
 			modeset->x = offset->x;
 			modeset->y = offset->y;
@@ -2169,7 +2179,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 
 	/* Clear out any old modes if there are no more connected outputs. */
 	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
+		struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set;
+
 		if (modeset->num_connectors == 0) {
 			BUG_ON(modeset->fb);
 			if (modeset->mode)
-- 
2.7.4

Re: [PATCH 2/4] drm/fb-helper: Keep references for the current set of used connectors

[Chris Wilson]

On Wed, Oct 26, 2016 at 12:05:53PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> The fbdev helper code keeps around two lists of connectors. One is the
> list of all connectors it could use, and that list already holds
> references for all the connectors. However the other list, or rather
> lists, is the one actively being used. That list is tracked per-crtc
> and currently doesn't hold any extra references. Let's grab those
> extra references to avoid oopsing when the connector vanishes. The
> list of all possible connectors should get updated when the hpd happens,
> but the list of actively used connectors would not get updated until
> the next time the fb-helper picks through the set of possible connectors.
> And so we need to hang on to the connectors until that time.
> 
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index db469d12d195..9ee1dacf97b8 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2090,7 +2090,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  	struct drm_fb_helper_crtc **crtcs;
>  	struct drm_display_mode **modes;
>  	struct drm_fb_offset *offsets;
> -	struct drm_mode_set *modeset;
>  	bool *enabled;
>  	int width, height;
>  	int i;
> @@ -2139,7 +2138,14 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  	/* need to set the modesets up here for use later */
>  	/* fill out the connector<->crtc mappings into the modesets */
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
> -		modeset = &fb_helper->crtc_info[i].mode_set;
> +		struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set;
> +		int j;
> +
> +		for (j = 0; j < modeset->num_connectors; j++) {
> +			drm_connector_unreference(modeset->connectors[j]);
> +			modeset->connectors[j] = NULL;
> +		}
> +

Don't we also need a similar cleanup loop in drm_fb_helper_crtc_free()?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/fb-helper: Keep references for the current set of used connectors

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 10:52:24AM +0100, Chris Wilson wrote:
> On Wed, Oct 26, 2016 at 12:05:53PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > The fbdev helper code keeps around two lists of connectors. One is the
> > list of all connectors it could use, and that list already holds
> > references for all the connectors. However the other list, or rather
> > lists, is the one actively being used. That list is tracked per-crtc
> > and currently doesn't hold any extra references. Let's grab those
> > extra references to avoid oopsing when the connector vanishes. The
> > list of all possible connectors should get updated when the hpd happens,
> > but the list of actively used connectors would not get updated until
> > the next time the fb-helper picks through the set of possible connectors.
> > And so we need to hang on to the connectors until that time.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com>
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index db469d12d195..9ee1dacf97b8 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2090,7 +2090,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> >  	struct drm_fb_helper_crtc **crtcs;
> >  	struct drm_display_mode **modes;
> >  	struct drm_fb_offset *offsets;
> > -	struct drm_mode_set *modeset;
> >  	bool *enabled;
> >  	int width, height;
> >  	int i;
> > @@ -2139,7 +2138,14 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> >  	/* need to set the modesets up here for use later */
> >  	/* fill out the connector<->crtc mappings into the modesets */
> >  	for (i = 0; i < fb_helper->crtc_count; i++) {
> > -		modeset = &fb_helper->crtc_info[i].mode_set;
> > +		struct drm_mode_set *modeset = &fb_helper->crtc_info[i].mode_set;
> > +		int j;
> > +
> > +		for (j = 0; j < modeset->num_connectors; j++) {
> > +			drm_connector_unreference(modeset->connectors[j]);
> > +			modeset->connectors[j] = NULL;
> > +		}
> > +
> 
> Don't we also need a similar cleanup loop in drm_fb_helper_crtc_free()?

Sounds likely. I'll go have a look.

-- 
Ville Syrj�l�
Intel OTC

[PATCH v2 2/4] drm/fb-helper: Keep references for the current set of used connectors

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The fbdev helper code keeps around two lists of connectors. One is the
list of all connectors it could use, and that list already holds
references for all the connectors. However the other list, or rather
lists, is the one actively being used. That list is tracked per-crtc
and currently doesn't hold any extra references. Let's grab those
extra references to avoid oopsing when the connector vanishes. The
list of all possible connectors should get updated when the hpd happens,
but the list of actively used connectors would not get updated until
the next time the fb-helper picks through the set of possible connectors.
And so we need to hang on to the connectors until that time.

Since we need to clean up in drm_fb_helper_crtc_free() as well,
let's pull the code to a common place. And while at it let's
pull in up the modeset->mode cleanup in there as well. The case
of modeset->fb is a bit less clear. I'm thinking we should probably
hold a reference to it, but for now I just slapped on a FIXME.

v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 58 +++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index db469d12d195..83961f1a97d2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_blank);
 
+static void drm_fb_helper_modeset_free(struct drm_fb_helper *helper,
+				       struct drm_mode_set *modeset)
+{
+	int i;
+
+	for (i = 0; i < modeset->num_connectors; i++) {
+		drm_connector_unreference(modeset->connectors[i]);
+		modeset->connectors[i] = NULL;
+	}
+	modeset->num_connectors = 0;
+
+	drm_mode_destroy(helper->dev, modeset->mode);
+	modeset->mode = NULL;
+
+	/* FIXME should hold a ref? */
+	modeset->fb = NULL;
+}
+
 static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 {
 	int i;
@@ -614,11 +632,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 		kfree(helper->connector_info[i]);
 	}
 	kfree(helper->connector_info);
-	for (i = 0; i < helper->crtc_count; i++) {
-		kfree(helper->crtc_info[i].mode_set.connectors);
-		if (helper->crtc_info[i].mode_set.mode)
-			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
-	}
+
+	for (i = 0; i < helper->crtc_count; i++)
+		drm_fb_helper_modeset_free(helper,
+					   &helper->crtc_info[i].mode_set);
 	kfree(helper->crtc_info);
 }
 
@@ -2090,7 +2107,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	struct drm_fb_helper_crtc **crtcs;
 	struct drm_display_mode **modes;
 	struct drm_fb_offset *offsets;
-	struct drm_mode_set *modeset;
 	bool *enabled;
 	int width, height;
 	int i;
@@ -2138,45 +2154,35 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
-		modeset->num_connectors = 0;
-		modeset->fb = NULL;
-	}
+	for (i = 0; i < fb_helper->crtc_count; i++)
+		drm_fb_helper_modeset_free(fb_helper,
+					   &fb_helper->crtc_info[i].mode_set);
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
 		struct drm_fb_offset *offset = &offsets[i];
-		modeset = &fb_crtc->mode_set;
+		struct drm_mode_set *modeset = &fb_crtc->mode_set;
 
 		if (mode && fb_crtc) {
+			struct drm_connector *connector =
+				fb_helper->connector_info[i]->connector;
+
 			DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n",
 				      mode->name, fb_crtc->mode_set.crtc->base.id, offset->x, offset->y);
+
 			fb_crtc->desired_mode = mode;
 			fb_crtc->x = offset->x;
 			fb_crtc->y = offset->y;
-			if (modeset->mode)
-				drm_mode_destroy(dev, modeset->mode);
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
-			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			drm_connector_reference(connector);
+			modeset->connectors[modeset->num_connectors++] = connector;
 			modeset->fb = fb_helper->fb;
 			modeset->x = offset->x;
 			modeset->y = offset->y;
 		}
 	}
-
-	/* Clear out any old modes if there are no more connected outputs. */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
-		if (modeset->num_connectors == 0) {
-			BUG_ON(modeset->fb);
-			if (modeset->mode)
-				drm_mode_destroy(dev, modeset->mode);
-			modeset->mode = NULL;
-		}
-	}
 out:
 	kfree(crtcs);
 	kfree(modes);
-- 
2.7.4

Re: [PATCH v2 2/4] drm/fb-helper: Keep references for the current set of used connectors

[Chris Wilson]

On Wed, Oct 26, 2016 at 04:31:20PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> The fbdev helper code keeps around two lists of connectors. One is the
> list of all connectors it could use, and that list already holds
> references for all the connectors. However the other list, or rather
> lists, is the one actively being used. That list is tracked per-crtc
> and currently doesn't hold any extra references. Let's grab those
> extra references to avoid oopsing when the connector vanishes. The
> list of all possible connectors should get updated when the hpd happens,
> but the list of actively used connectors would not get updated until
> the next time the fb-helper picks through the set of possible connectors.
> And so we need to hang on to the connectors until that time.
> 
> Since we need to clean up in drm_fb_helper_crtc_free() as well,
> let's pull the code to a common place. And while at it let's
> pull in up the modeset->mode cleanup in there as well. The case
> of modeset->fb is a bit less clear. I'm thinking we should probably
> hold a reference to it, but for now I just slapped on a FIXME.
> 
> v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 58 +++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index db469d12d195..83961f1a97d2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_blank);
>  
> +static void drm_fb_helper_modeset_free(struct drm_fb_helper *helper,
> +				       struct drm_mode_set *modeset)
> +{
> +	int i;
> +
> +	for (i = 0; i < modeset->num_connectors; i++) {
> +		drm_connector_unreference(modeset->connectors[i]);
> +		modeset->connectors[i] = NULL;
> +	}
> +	modeset->num_connectors = 0;
> +
> +	drm_mode_destroy(helper->dev, modeset->mode);
> +	modeset->mode = NULL;
> +
> +	/* FIXME should hold a ref? */
> +	modeset->fb = NULL;
> +}
> +
>  static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
>  {
>  	int i;
> @@ -614,11 +632,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
>  		kfree(helper->connector_info[i]);
>  	}
>  	kfree(helper->connector_info);
> -	for (i = 0; i < helper->crtc_count; i++) {
> -		kfree(helper->crtc_info[i].mode_set.connectors);
> -		if (helper->crtc_info[i].mode_set.mode)
> -			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
> -	}
> +
> +	for (i = 0; i < helper->crtc_count; i++)
> +		drm_fb_helper_modeset_free(helper,
> +					   &helper->crtc_info[i].mode_set);

We lose the kfree(mode_set.connectors) here.

So 
	for (i = 0; i < helper->crtc_count; i++)
		struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set);

		drm_fb_helper_modeset_release(helper, modeset);
		kfree(modeset->connectors);
	}
?

Couldn't spot any other missing calls to release the new ref, so with
the tiny leak fixed,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 2/4] drm/fb-helper: Keep references for the current set of used connectors

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 02:54:45PM +0100, Chris Wilson wrote:
> On Wed, Oct 26, 2016 at 04:31:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > The fbdev helper code keeps around two lists of connectors. One is the
> > list of all connectors it could use, and that list already holds
> > references for all the connectors. However the other list, or rather
> > lists, is the one actively being used. That list is tracked per-crtc
> > and currently doesn't hold any extra references. Let's grab those
> > extra references to avoid oopsing when the connector vanishes. The
> > list of all possible connectors should get updated when the hpd happens,
> > but the list of actively used connectors would not get updated until
> > the next time the fb-helper picks through the set of possible connectors.
> > And so we need to hang on to the connectors until that time.
> > 
> > Since we need to clean up in drm_fb_helper_crtc_free() as well,
> > let's pull the code to a common place. And while at it let's
> > pull in up the modeset->mode cleanup in there as well. The case
> > of modeset->fb is a bit less clear. I'm thinking we should probably
> > hold a reference to it, but for now I just slapped on a FIXME.
> > 
> > v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 58 +++++++++++++++++++++++------------------
> >  1 file changed, 32 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index db469d12d195..83961f1a97d2 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_blank);
> >  
> > +static void drm_fb_helper_modeset_free(struct drm_fb_helper *helper,
> > +				       struct drm_mode_set *modeset)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < modeset->num_connectors; i++) {
> > +		drm_connector_unreference(modeset->connectors[i]);
> > +		modeset->connectors[i] = NULL;
> > +	}
> > +	modeset->num_connectors = 0;
> > +
> > +	drm_mode_destroy(helper->dev, modeset->mode);
> > +	modeset->mode = NULL;
> > +
> > +	/* FIXME should hold a ref? */
> > +	modeset->fb = NULL;
> > +}
> > +
> >  static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> >  {
> >  	int i;
> > @@ -614,11 +632,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> >  		kfree(helper->connector_info[i]);
> >  	}
> >  	kfree(helper->connector_info);
> > -	for (i = 0; i < helper->crtc_count; i++) {
> > -		kfree(helper->crtc_info[i].mode_set.connectors);
> > -		if (helper->crtc_info[i].mode_set.mode)
> > -			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
> > -	}
> > +
> > +	for (i = 0; i < helper->crtc_count; i++)
> > +		drm_fb_helper_modeset_free(helper,
> > +					   &helper->crtc_info[i].mode_set);
> 
> We lose the kfree(mode_set.connectors) here.

Doh.

> 
> So 
> 	for (i = 0; i < helper->crtc_count; i++)
> 		struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set);
> 
> 		drm_fb_helper_modeset_release(helper, modeset);

Hmm. Yeah _release() does seem better than _free() if we don't actually
free all of it.

> 		kfree(modeset->connectors);
> 	}
> ?
> 
> Couldn't spot any other missing calls to release the new ref, so with
> the tiny leak fixed,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks. I'll respin it once more.

-- 
Ville Syrj�l�
Intel OTC

[PATCH v3 2/4] drm/fb-helper: Keep references for the current set of used connectors

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The fbdev helper code keeps around two lists of connectors. One is the
list of all connectors it could use, and that list already holds
references for all the connectors. However the other list, or rather
lists, is the one actively being used. That list is tracked per-crtc
and currently doesn't hold any extra references. Let's grab those
extra references to avoid oopsing when the connector vanishes. The
list of all possible connectors should get updated when the hpd happens,
but the list of actively used connectors would not get updated until
the next time the fb-helper picks through the set of possible connectors.
And so we need to hang on to the connectors until that time.

Since we need to clean up in drm_fb_helper_crtc_free() as well,
let's pull the code to a common place. And while at it let's
pull in up the modeset->mode cleanup in there as well. The case
of modeset->fb is a bit less clear. I'm thinking we should probably
hold a reference to it, but for now I just slapped on a FIXME.

v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)
v3: Don't leak modeset->connectors (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_fb_helper.c | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index db469d12d195..813280e64311 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_blank);
 
+static void drm_fb_helper_modeset_release(struct drm_fb_helper *helper,
+					  struct drm_mode_set *modeset)
+{
+	int i;
+
+	for (i = 0; i < modeset->num_connectors; i++) {
+		drm_connector_unreference(modeset->connectors[i]);
+		modeset->connectors[i] = NULL;
+	}
+	modeset->num_connectors = 0;
+
+	drm_mode_destroy(helper->dev, modeset->mode);
+	modeset->mode = NULL;
+
+	/* FIXME should hold a ref? */
+	modeset->fb = NULL;
+}
+
 static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 {
 	int i;
@@ -614,10 +632,12 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 		kfree(helper->connector_info[i]);
 	}
 	kfree(helper->connector_info);
+
 	for (i = 0; i < helper->crtc_count; i++) {
-		kfree(helper->crtc_info[i].mode_set.connectors);
-		if (helper->crtc_info[i].mode_set.mode)
-			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
+		struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set;
+
+		drm_fb_helper_modeset_release(helper, modeset);
+		kfree(modeset->connectors);
 	}
 	kfree(helper->crtc_info);
 }
@@ -2090,7 +2110,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	struct drm_fb_helper_crtc **crtcs;
 	struct drm_display_mode **modes;
 	struct drm_fb_offset *offsets;
-	struct drm_mode_set *modeset;
 	bool *enabled;
 	int width, height;
 	int i;
@@ -2138,45 +2157,35 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
-		modeset->num_connectors = 0;
-		modeset->fb = NULL;
-	}
+	for (i = 0; i < fb_helper->crtc_count; i++)
+		drm_fb_helper_modeset_release(fb_helper,
+					      &fb_helper->crtc_info[i].mode_set);
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
 		struct drm_fb_offset *offset = &offsets[i];
-		modeset = &fb_crtc->mode_set;
+		struct drm_mode_set *modeset = &fb_crtc->mode_set;
 
 		if (mode && fb_crtc) {
+			struct drm_connector *connector =
+				fb_helper->connector_info[i]->connector;
+
 			DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n",
 				      mode->name, fb_crtc->mode_set.crtc->base.id, offset->x, offset->y);
+
 			fb_crtc->desired_mode = mode;
 			fb_crtc->x = offset->x;
 			fb_crtc->y = offset->y;
-			if (modeset->mode)
-				drm_mode_destroy(dev, modeset->mode);
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
-			modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
+			drm_connector_reference(connector);
+			modeset->connectors[modeset->num_connectors++] = connector;
 			modeset->fb = fb_helper->fb;
 			modeset->x = offset->x;
 			modeset->y = offset->y;
 		}
 	}
-
-	/* Clear out any old modes if there are no more connected outputs. */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
-		if (modeset->num_connectors == 0) {
-			BUG_ON(modeset->fb);
-			if (modeset->mode)
-				drm_mode_destroy(dev, modeset->mode);
-			modeset->mode = NULL;
-		}
-	}
 out:
 	kfree(crtcs);
 	kfree(modes);
-- 
2.7.4

[PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The i2c adapter is only relevant for some peer device types, so
let's clear the pdt if it's still the same as the old_pdt when we
tear down the i2c adapter.

I don't really like this design pattern of updating port->whatever
before doing the accompanying changes and passing around old_whatever
to figure stuff out. Would make much more sense to me to the pass the
new value around and only update the port->whatever when things are
consistent. But let's try to work with what we have right now.

Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com>
Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 04e457117980..956babc161e5 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -882,6 +882,9 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
 		drm_dp_put_mst_branch_device(mstb);
 		break;
 	}
+
+	if (port->pdt == old_pdt)
+		port->pdt = DP_PEER_DEVICE_NONE;
 }
 
 static void drm_dp_destroy_port(struct kref *kref)
-- 
2.7.4

Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[Daniel Vetter]

On Wed, Oct 26, 2016 at 12:05:54PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> The i2c adapter is only relevant for some peer device types, so
> let's clear the pdt if it's still the same as the old_pdt when we
> tear down the i2c adapter.
> 
> I don't really like this design pattern of updating port->whatever
> before doing the accompanying changes and passing around old_whatever
> to figure stuff out. Would make much more sense to me to the pass the
> new value around and only update the port->whatever when things are
> consistent. But let's try to work with what we have right now.
> 
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e457117980..956babc161e5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -882,6 +882,9 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
>  		drm_dp_put_mst_branch_device(mstb);
>  		break;
>  	}
> +
> +	if (port->pdt == old_pdt)
> +		port->pdt = DP_PEER_DEVICE_NONE;

So from my understanding this is needed for the callsite in
drm_dp_destroy_connector_work(). All others are either the final destroy
path, or set up the ->pdt to something before calling this function. Only
this call site passes port->pdt. I think we should instead change this
callsite to set the port->pdt to NONE after the call, i.e.


diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 04e457117980..36f47092c703 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2919,6 +2919,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		mgr->cbs->destroy_connector(mgr, port->connector);
 
 		drm_dp_port_teardown_pdt(port, port->pdt);
+		port->pdt = DP_PEER_DEVICE_NONE;
 
 		if (!port->input && port->vcpi.vcpi > 0) {
 			drm_dp_mst_reset_vcpi_slots(mgr, port);


I think that would be more consistent than spreading the control flow even
more like in your patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 02:53:01PM +0200, Daniel Vetter wrote:
> On Wed, Oct 26, 2016 at 12:05:54PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > The i2c adapter is only relevant for some peer device types, so
> > let's clear the pdt if it's still the same as the old_pdt when we
> > tear down the i2c adapter.
> > 
> > I don't really like this design pattern of updating port->whatever
> > before doing the accompanying changes and passing around old_whatever
> > to figure stuff out. Would make much more sense to me to the pass the
> > new value around and only update the port->whatever when things are
> > consistent. But let's try to work with what we have right now.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com>
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 04e457117980..956babc161e5 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -882,6 +882,9 @@ static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
> >  		drm_dp_put_mst_branch_device(mstb);
> >  		break;
> >  	}
> > +
> > +	if (port->pdt == old_pdt)
> > +		port->pdt = DP_PEER_DEVICE_NONE;
> 
> So from my understanding this is needed for the callsite in
> drm_dp_destroy_connector_work(). All others are either the final destroy
> path, or set up the ->pdt to something before calling this function. Only
> this call site passes port->pdt. I think we should instead change this
> callsite to set the port->pdt to NONE after the call, i.e.
> 
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e457117980..36f47092c703 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2919,6 +2919,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
>  		mgr->cbs->destroy_connector(mgr, port->connector);
>  
>  		drm_dp_port_teardown_pdt(port, port->pdt);
> +		port->pdt = DP_PEER_DEVICE_NONE;
>  
>  		if (!port->input && port->vcpi.vcpi > 0) {
>  			drm_dp_mst_reset_vcpi_slots(mgr, port);
> 
> 
> I think that would be more consistent than spreading the control flow even
> more like in your patch.

Yeah, makes sense.

-- 
Ville Syrj�l�
Intel OTC

[PATCH v2 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The i2c adapter is only relevant for some peer device types, so
let's clear the pdt if it's still the same as the old_pdt when we
tear down the i2c adapter.

I don't really like this design pattern of updating port->whatever
before doing the accompanying changes and passing around old_whatever
to figure stuff out. Would make much more sense to me to the pass the
new value around and only update the port->whatever when things are
consistent. But let's try to work with what we have right now.

v2: Clear port->pdt in the caller, if needed (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 04e457117980..ba13f9d8720b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -914,6 +914,7 @@ static void drm_dp_destroy_port(struct kref *kref)
 		/* no need to clean up vcpi
 		 * as if we have no connector we never setup a vcpi */
 		drm_dp_port_teardown_pdt(port, port->pdt);
+		port->pdt = DP_PEER_DEVICE_NONE;
 	}
 	kfree(port);
 }
@@ -2919,6 +2920,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		mgr->cbs->destroy_connector(mgr, port->connector);
 
 		drm_dp_port_teardown_pdt(port, port->pdt);
+		port->pdt = DP_PEER_DEVICE_NONE;
 
 		if (!port->input && port->vcpi.vcpi > 0) {
 			drm_dp_mst_reset_vcpi_slots(mgr, port);
-- 
2.7.4

Re: [PATCH v2 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 04:30:33PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> The i2c adapter is only relevant for some peer device types, so
> let's clear the pdt if it's still the same as the old_pdt when we
> tear down the i2c adapter.
> 
> I don't really like this design pattern of updating port->whatever
> before doing the accompanying changes and passing around old_whatever
> to figure stuff out. Would make much more sense to me to the pass the
> new value around and only update the port->whatever when things are
> consistent. But let's try to work with what we have right now.
> 
> v2: Clear port->pdt in the caller, if needed (Daniel)
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e457117980..ba13f9d8720b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -914,6 +914,7 @@ static void drm_dp_destroy_port(struct kref *kref)
>  		/* no need to clean up vcpi
>  		 * as if we have no connector we never setup a vcpi */
>  		drm_dp_port_teardown_pdt(port, port->pdt);
> +		port->pdt = DP_PEER_DEVICE_NONE;

And naturally I forgot to amend the commit message w.r.t. this guy.
We don't really need to do this here, but I figured I'd try to be a
bit more consistent by having it, just to avoid accidental mistakes
if/when someone changes this stuff again later.

>  	}
>  	kfree(port);
>  }
> @@ -2919,6 +2920,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
>  		mgr->cbs->destroy_connector(mgr, port->connector);
>  
>  		drm_dp_port_teardown_pdt(port, port->pdt);
> +		port->pdt = DP_PEER_DEVICE_NONE;
>  
>  		if (!port->input && port->vcpi.vcpi > 0) {
>  			drm_dp_mst_reset_vcpi_slots(mgr, port);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v2 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[Daniel Vetter]

On Wed, Oct 26, 2016 at 04:35:50PM +0300, Ville Syrj�l� wrote:
> On Wed, Oct 26, 2016 at 04:30:33PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > The i2c adapter is only relevant for some peer device types, so
> > let's clear the pdt if it's still the same as the old_pdt when we
> > tear down the i2c adapter.
> > 
> > I don't really like this design pattern of updating port->whatever
> > before doing the accompanying changes and passing around old_whatever
> > to figure stuff out. Would make much more sense to me to the pass the
> > new value around and only update the port->whatever when things are
> > consistent. But let's try to work with what we have right now.
> > 
> > v2: Clear port->pdt in the caller, if needed (Daniel)
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 04e457117980..ba13f9d8720b 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -914,6 +914,7 @@ static void drm_dp_destroy_port(struct kref *kref)
> >  		/* no need to clean up vcpi
> >  		 * as if we have no connector we never setup a vcpi */
> >  		drm_dp_port_teardown_pdt(port, port->pdt);
> > +		port->pdt = DP_PEER_DEVICE_NONE;
> 
> And naturally I forgot to amend the commit message w.r.t. this guy.
> We don't really need to do this here, but I figured I'd try to be a
> bit more consistent by having it, just to avoid accidental mistakes
> if/when someone changes this stuff again later.

With the commit message amended like you say here:

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

> 
> >  	}
> >  	kfree(port);
> >  }
> > @@ -2919,6 +2920,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
> >  		mgr->cbs->destroy_connector(mgr, port->connector);
> >  
> >  		drm_dp_port_teardown_pdt(port, port->pdt);
> > +		port->pdt = DP_PEER_DEVICE_NONE;
> >  
> >  		if (!port->input && port->vcpi.vcpi > 0) {
> >  			drm_dp_mst_reset_vcpi_slots(mgr, port);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrj�l�
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[Daniel Vetter]

On Wed, Oct 26, 2016 at 04:02:57PM +0200, Daniel Vetter wrote:
> On Wed, Oct 26, 2016 at 04:35:50PM +0300, Ville Syrj�l� wrote:
> > On Wed, Oct 26, 2016 at 04:30:33PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > 
> > > The i2c adapter is only relevant for some peer device types, so
> > > let's clear the pdt if it's still the same as the old_pdt when we
> > > tear down the i2c adapter.
> > > 
> > > I don't really like this design pattern of updating port->whatever
> > > before doing the accompanying changes and passing around old_whatever
> > > to figure stuff out. Would make much more sense to me to the pass the
> > > new value around and only update the port->whatever when things are
> > > consistent. But let's try to work with what we have right now.
> > > 
> > > v2: Clear port->pdt in the caller, if needed (Daniel)
> > > 
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: stable@vger.kernel.org
> > > Cc: Carlos Santa <carlos.santa@intel.com>
> > > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > > Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> > > Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 04e457117980..ba13f9d8720b 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -914,6 +914,7 @@ static void drm_dp_destroy_port(struct kref *kref)
> > >  		/* no need to clean up vcpi
> > >  		 * as if we have no connector we never setup a vcpi */
> > >  		drm_dp_port_teardown_pdt(port, port->pdt);
> > > +		port->pdt = DP_PEER_DEVICE_NONE;
> > 
> > And naturally I forgot to amend the commit message w.r.t. this guy.
> > We don't really need to do this here, but I figured I'd try to be a
> > bit more consistent by having it, just to avoid accidental mistakes
> > if/when someone changes this stuff again later.
> 
> With the commit message amended like you say here:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Bikeshedded while applying and put the bunch into drm-misc-fixes.
-Daniel

> 
> > 
> > >  	}
> > >  	kfree(port);
> > >  }
> > > @@ -2919,6 +2920,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
> > >  		mgr->cbs->destroy_connector(mgr, port->connector);
> > >  
> > >  		drm_dp_port_teardown_pdt(port, port->pdt);
> > > +		port->pdt = DP_PEER_DEVICE_NONE;
> > >  
> > >  		if (!port->input && port->vcpi.vcpi > 0) {
> > >  			drm_dp_mst_reset_vcpi_slots(mgr, port);
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrj�l�
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 3/4] drm/dp/mst: Clear port->pdt when tearing down the i2c adapter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The i2c adapter is only relevant for some peer device types, so
let's clear the pdt if it's still the same as the old_pdt when we
tear down the i2c adapter.

I don't really like this design pattern of updating port->whatever
before doing the accompanying changes and passing around old_whatever
to figure stuff out. Would make much more sense to me to the pass the
new value around and only update the port->whatever when things are
consistent. But let's try to work with what we have right now.

Clearing port->pdt in drm_dp_destroy_connector_work() would be enough,
but I chose to do it also in drm_dp_destroy_port(). The reason being
extra consistency so that we don't lose this by accident should someone
choose to refactor the code later.

v2: Cleart port->pdt in the caller, if needed (Daniel)
v3: Amend commit message a bit (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 04e457117980..ba13f9d8720b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -914,6 +914,7 @@ static void drm_dp_destroy_port(struct kref *kref)
 		/* no need to clean up vcpi
 		 * as if we have no connector we never setup a vcpi */
 		drm_dp_port_teardown_pdt(port, port->pdt);
+		port->pdt = DP_PEER_DEVICE_NONE;
 	}
 	kfree(port);
 }
@@ -2919,6 +2920,7 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
 		mgr->cbs->destroy_connector(mgr, port->connector);
 
 		drm_dp_port_teardown_pdt(port, port->pdt);
+		port->pdt = DP_PEER_DEVICE_NONE;
 
 		if (!port->input && port->vcpi.vcpi > 0) {
 			drm_dp_mst_reset_vcpi_slots(mgr, port);
-- 
2.7.4

[PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Only certain types of pdts have the DDC bus registered, so check for
that before we attempt the EDID read. Othwewise we risk playing around
with an i2c adapter that doesn't actually exist.

Cc: stable@vger.kernel.org
Cc: Carlos Santa <carlos.santa@intel.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Tested-by: Carlos Santa <carlos.santa@intel.com>
Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 956babc161e5..690d1b407a90 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1162,7 +1162,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 			drm_dp_put_port(port);
 			goto out;
 		}
-		if (port->port_num >= DP_MST_LOGICAL_PORT_0) {
+		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
+		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
+		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
 			port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc);
 			drm_mode_connector_set_tile_property(port->connector);
 		}
-- 
2.7.4

Re: [PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read

[Daniel Vetter]

On Wed, Oct 26, 2016 at 12:05:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Only certain types of pdts have the DDC bus registered, so check for
> that before we attempt the EDID read. Othwewise we risk playing around
> with an i2c adapter that doesn't actually exist.
> 
> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 956babc161e5..690d1b407a90 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1162,7 +1162,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>  			drm_dp_put_port(port);
>  			goto out;
>  		}
> -		if (port->port_num >= DP_MST_LOGICAL_PORT_0) {
> +		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> +		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> +		    port->port_num >= DP_MST_LOGICAL_PORT_0) {

Matches what's in drm_dp_port_setup_pdt. Not sure this is the most
reliable way to do this though, but I can't come up with anything better.

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

>  			port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc);
>  			drm_mode_connector_set_tile_property(port->connector);
>  		}
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 02:55:14PM +0200, Daniel Vetter wrote:
> On Wed, Oct 26, 2016 at 12:05:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > Only certain types of pdts have the DDC bus registered, so check for
> > that before we attempt the EDID read. Othwewise we risk playing around
> > with an i2c adapter that doesn't actually exist.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com>
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 956babc161e5..690d1b407a90 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1162,7 +1162,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
> >  			drm_dp_put_port(port);
> >  			goto out;
> >  		}
> > -		if (port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > +		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > +		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > +		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> 
> Matches what's in drm_dp_port_setup_pdt. Not sure this is the most
> reliable way to do this though, but I can't come up with anything better.

Yeah. I just went with whatever looks semi-correct based on where the
thing is set up. I'd have to go read the actual spec to figure out if
this makes any real sense. But at least it avoids the explosion.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >  			port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc);
> >  			drm_mode_connector_set_tile_property(port->connector);
> >  		}
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read

[Carlos Santa]

On Wed, 2016-10-26 at 02:05 -0700, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Only certain types of pdts have the DDC bus registered, so check for
> that before we attempt the EDID read. Othwewise we risk playing 

typo above

> around
> with an i2c adapter that doesn't actually exist.
> 

Would it be worth pasting the stack trace of the crash??

Carlos


> Cc: stable@vger.kernel.org
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Tested-by: Carlos Santa <carlos.santa@intel.com>
> Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 956babc161e5..690d1b407a90 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1162,7 +1162,9 @@ static void drm_dp_add_port(struct
> drm_dp_mst_branch *mstb,
>  			drm_dp_put_port(port);
>  			goto out;
>  		}
> -		if (port->port_num >= DP_MST_LOGICAL_PORT_0) {
> +		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> +		     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> +		    port->port_num >= DP_MST_LOGICAL_PORT_0) {
>  			port->cached_edid = drm_get_edid(port-
> >connector, &port->aux.ddc);
>  			drm_mode_connector_set_tile_property(port-
> >connector);
>  		}
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] drm/dp/mst: Check peer device type before attempting EDID read

[Ville Syrjälä]

On Wed, Oct 26, 2016 at 11:31:55AM -0700, Carlos Santa wrote:
> On Wed, 2016-10-26 at 02:05 -0700, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > Only certain types of pdts have the DDC bus registered, so check for
> > that before we attempt the EDID read. Othwewise we risk playing 
> 
> typo above
> 
> > around
> > with an i2c adapter that doesn't actually exist.
> > 
> 
> Would it be worth pasting the stack trace of the crash??

Can't hurt.

> 
> Carlos
> 
> 
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com>
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> > �drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
> > �1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 956babc161e5..690d1b407a90 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1162,7 +1162,9 @@ static void drm_dp_add_port(struct
> > drm_dp_mst_branch *mstb,
> > �			drm_dp_put_port(port);
> > �			goto out;
> > �		}
> > -		if (port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > +		if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > +		�����port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > +		����port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > �			port->cached_edid = drm_get_edid(port-
> > >connector, &port->aux.ddc);
> > �			drm_mode_connector_set_tile_property(port-
> > >connector);
> > �		}
> > --�
> > 2.7.4
> > 

-- 
Ville Syrj�l�
Intel OTC