[PATCH 0/5] Armada DRM fixes

[PATCH 0/5] Armada DRM fixes

[Russell King - ARM Linux]

Hi David,

This series fixes some issues in the Armada DRM driver:
- A memory leak while cleaning up in the CRTC initialisation path
- Avoid powering down YUV FIFOs when disabling the graphics plane
- Several fixes for the overlay plane positioning

I'll send a pull request for this as is normal for Armada stuff next week.

Thanks.

 drivers/gpu/drm/armada/armada_crtc.c    | 32 ++++++++++++++++++---------
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_overlay.c | 38 ++++++++++++++++-----------------
 3 files changed, 43 insertions(+), 29 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/armada: fix leak of crtc structure

[Russell King]

Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 2e065facdce7..844d488b6654 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->crtc.port = port;
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-	if (!primary)
+	if (!primary) {
+		kfree(dcrtc);
 		return -ENOMEM;
+	}
 
 	ret = armada_drm_plane_init(primary);
 	if (ret) {
+		kfree(dcrtc);
 		kfree(primary);
 		return ret;
 	}
@@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 				       NULL,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
+		kfree(dcrtc);
 		kfree(primary);
 		return ret;
 	}
 
 	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
 					&armada_crtc_funcs, NULL);
-	if (ret)
+	if (ret) {
+		kfree(dcrtc);
 		goto err_crtc_init;
+	}
 
 	drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/armada: fix SRAM powerdown

[Russell King]

Avoid powering down the overlay SRAM banks when disabling the primary
plane, thereby masking any overlay video.  This feature is supposed to
allow us to cut the bandwidth required while displaying full-frame
overlay video.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 844d488b6654..8696e74d1a6e 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -744,15 +744,14 @@ void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 	if (plane->fb)
 		drm_framebuffer_put(plane->fb);
 
-	/* Power down the Y/U/V FIFOs */
-	sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
-
 	/* Power down most RAMs and FIFOs if this is the primary plane */
 	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
-		sram_para1 |= CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
-			      CFG_PDWN32x32 | CFG_PDWN64x66;
+		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
+			     CFG_PDWN32x32 | CFG_PDWN64x66;
 		dma_ctrl0_mask = CFG_GRA_ENA;
 	} else {
+		/* Power down the Y/U/V FIFOs */
+		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
 		dma_ctrl0_mask = CFG_DMA_ENA;
 	}
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/armada: fix UV swap code

[Russell King]

The UV swap code was not always programming things correctly when
the source origin box has been offset.  Fix this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_overlay.c | 38 ++++++++++++++++-----------------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h
index bab11f483575..bfd3514fbe9b 100644
--- a/drivers/gpu/drm/armada/armada_crtc.h
+++ b/drivers/gpu/drm/armada/armada_crtc.h
@@ -42,6 +42,8 @@ struct armada_plane_work {
 };
 
 struct armada_plane_state {
+	u16 src_x;
+	u16 src_y;
 	u32 src_hw;
 	u32 dst_hw;
 	u32 dst_yx;
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
index b411b608821a..aba947696178 100644
--- a/drivers/gpu/drm/armada/armada_overlay.c
+++ b/drivers/gpu/drm/armada/armada_overlay.c
@@ -99,6 +99,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 {
 	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
 	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
+	const struct drm_format_info *format;
 	struct drm_rect src = {
 		.x1 = src_x,
 		.y1 = src_y,
@@ -117,7 +118,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	};
 	uint32_t val, ctrl0;
 	unsigned idx = 0;
-	bool visible;
+	bool visible, fb_changed;
 	int ret;
 
 	trace_armada_ovl_plane_update(plane, crtc, fb,
@@ -138,6 +139,18 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!visible)
 		ctrl0 &= ~CFG_DMA_ENA;
 
+	/*
+	 * Shifting a YUV packed format image by one pixel causes the U/V
+	 * planes to swap.  Compensate for it by also toggling the UV swap.
+	 */
+	format = fb->format;
+	if (format->num_planes == 1 && src.x1 >> 16 & (format->hsub - 1))
+		ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
+
+	fb_changed = plane->fb != fb ||
+		     dplane->base.state.src_x != src.x1 >> 16 ||
+	             dplane->base.state.src_y != src.y1 >> 16;
+
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
@@ -145,7 +158,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	/* FIXME: overlay on an interlaced display */
 	/* Just updating the position/size? */
-	if (plane->fb == fb && dplane->base.state.ctrl0 == ctrl0) {
+	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
 		val = (drm_rect_height(&src) & 0xffff0000) |
 		      drm_rect_width(&src) >> 16;
 		dplane->base.state.src_hw = val;
@@ -169,9 +182,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
 		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
 
-	if (plane->fb != fb) {
-		u32 addrs[3], pixel_format;
-		int num_planes, hsub;
+	if (fb_changed) {
+		u32 addrs[3];
 
 		/*
 		 * Take a reference on the new framebuffer - we want to
@@ -182,23 +194,11 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		if (plane->fb)
 			armada_ovl_retire_fb(dplane, plane->fb);
 
-		src_y = src.y1 >> 16;
-		src_x = src.x1 >> 16;
+		dplane->base.state.src_y = src_y = src.y1 >> 16;
+		dplane->base.state.src_x = src_x = src.x1 >> 16;
 
 		armada_drm_plane_calc_addrs(addrs, fb, src_x, src_y);
 
-		pixel_format = fb->format->format;
-		hsub = drm_format_horz_chroma_subsampling(pixel_format);
-		num_planes = fb->format->num_planes;
-
-		/*
-		 * Annoyingly, shifting a YUYV-format image by one pixel
-		 * causes the U/V planes to toggle.  Toggle the UV swap.
-		 * (Unfortunately, this causes momentary colour flickering.)
-		 */
-		if (src_x & (hsub - 1) && num_planes == 1)
-			ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
-
 		armada_reg_queue_set(dplane->vbl.regs, idx, addrs[0],
 				     LCD_SPU_DMA_START_ADDR_Y0);
 		armada_reg_queue_set(dplane->vbl.regs, idx, addrs[1],
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/armada: improve efficiency of armada_drm_plane_calc_addrs()

[Russell King]

Lookup the drm_format_info structure once when computing all the
framebuffer plane addresses by using drm_format_info(), rather than
repetitive lookups via drm_format_plane_cpp().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 8696e74d1a6e..7b80273a49bb 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -168,8 +168,9 @@ static void armada_drm_crtc_update(struct armada_crtc *dcrtc)
 void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 	int x, int y)
 {
+	const struct drm_format_info *format = fb->format;
+	unsigned int num_planes = format->num_planes;
 	u32 addr = drm_fb_obj(fb)->dev_addr;
-	int num_planes = fb->format->num_planes;
 	int i;
 
 	if (num_planes > 3)
@@ -177,7 +178,7 @@ void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 
 	for (i = 0; i < num_planes; i++)
 		addrs[i] = addr + fb->offsets[i] + y * fb->pitches[i] +
-			     x * fb->format->cpp[i];
+			     x * format->cpp[i];
 	for (; i < 3; i++)
 		addrs[i] = 0;
 }
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/armada: fix YUV planar format framebuffer offsets

[Russell King]

We weren't correctly calculating the YUV planar offsets for subsampled
chroma planes correctly - fix up the coordinates for planes 1 and 2.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 7b80273a49bb..c28c2c533994 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -176,7 +176,13 @@ void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 	if (num_planes > 3)
 		num_planes = 3;
 
-	for (i = 0; i < num_planes; i++)
+	addrs[0] = addr + fb->offsets[0] + y * fb->pitches[0] +
+		   x * format->cpp[0];
+
+	y /= format->vsub;
+	x /= format->hsub;
+
+	for (i = 1; i < num_planes; i++)
 		addrs[i] = addr + fb->offsets[i] + y * fb->pitches[i] +
 			     x * format->cpp[i];
 	for (; i < 3; i++)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Sean Paul]

On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> Fix the leak of the CRTC structure in the failure paths of
> armada_drm_crtc_create().
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index 2e065facdce7..844d488b6654 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  	dcrtc->crtc.port = port;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> -	if (!primary)
> +	if (!primary) {
> +		kfree(dcrtc);
>  		return -ENOMEM;
> +	}
>  
>  	ret = armada_drm_plane_init(primary);
>  	if (ret) {
> +		kfree(dcrtc);
>  		kfree(primary);
>  		return ret;
>  	}
> @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  				       NULL,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
> +		kfree(dcrtc);
>  		kfree(primary);
>  		return ret;
>  	}
>  
>  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
>  					&armada_crtc_funcs, NULL);
> -	if (ret)
> +	if (ret) {
> +		kfree(dcrtc);
>  		goto err_crtc_init;

The mixed use of cleaning up at every exit and the lone label is a bit messy and
prone to bugs such as the one you're fixing. How about you label everything
and then just clean up once?

It also seems like you're missing cleanup if armada_overlay_plane_create()
fails?

Sean

> +	}
>  
>  	drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Russell King - ARM Linux]

On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > Fix the leak of the CRTC structure in the failure paths of
> > armada_drm_crtc_create().
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > index 2e065facdce7..844d488b6654 100644
> > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> >  	dcrtc->crtc.port = port;
> >  
> >  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > -	if (!primary)
> > +	if (!primary) {
> > +		kfree(dcrtc);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	ret = armada_drm_plane_init(primary);
> >  	if (ret) {
> > +		kfree(dcrtc);
> >  		kfree(primary);
> >  		return ret;
> >  	}
> > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> >  				       NULL,
> >  				       DRM_PLANE_TYPE_PRIMARY, NULL);
> >  	if (ret) {
> > +		kfree(dcrtc);
> >  		kfree(primary);
> >  		return ret;
> >  	}
> >  
> >  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> >  					&armada_crtc_funcs, NULL);
> > -	if (ret)
> > +	if (ret) {
> > +		kfree(dcrtc);
> >  		goto err_crtc_init;
> 
> The mixed use of cleaning up at every exit and the lone label is a bit messy and
> prone to bugs such as the one you're fixing. How about you label everything
> and then just clean up once?

It's not that simple - see alternative patch below.  How we clean
stuff up depends on how far through the initialisation we are, and
that doesn't lend itself to a sequential cleanup methodology.

> It also seems like you're missing cleanup if armada_overlay_plane_create()
> fails?

It's not missing.  At that point, the CRTC has been registered, and any
failure will be cleaned up by the drm_mode_config_cleanup() in the
error paths in armada_drm_bind() in armada_drv.c

It's rather non-obvious because it uses the ->destroy callbacks.

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index d987198a2ac6..79ce877bf45f 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 	ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc",
 			       dcrtc);
-	if (ret < 0) {
-		kfree(dcrtc);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_crtc;
 
 	if (dcrtc->variant->init) {
 		ret = dcrtc->variant->init(dcrtc, dev);
-		if (ret) {
-			kfree(dcrtc);
-			return ret;
-		}
+		if (ret)
+			goto err_crtc;
 	}
 
 	/* Ensure AXI pipeline is enabled */
@@ -1247,15 +1243,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
 	if (!primary) {
-		kfree(dcrtc);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_crtc;
 	}
 
 	ret = armada_drm_plane_init(primary);
 	if (ret) {
-		kfree(dcrtc);
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_universal_plane_init(drm, &primary->base, 0,
@@ -1265,17 +1260,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 				       NULL,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
-		kfree(dcrtc);
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
 					&armada_crtc_funcs, NULL);
-	if (ret) {
-		kfree(dcrtc);
+	if (ret)
 		goto err_crtc_init;
-	}
 
 	drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs);
 
@@ -1288,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 err_crtc_init:
 	primary->base.funcs->destroy(&primary->base);
+err_crtc:
+	kfree(dcrtc);
+
 	return ret;
 }
 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Sean Paul]

On Wed, Nov 29, 2017 at 07:16:43PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > > Fix the leak of the CRTC structure in the failure paths of
> > > armada_drm_crtc_create().
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > > index 2e065facdce7..844d488b6654 100644
> > > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > >  	dcrtc->crtc.port = port;
> > >  
> > >  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > -	if (!primary)
> > > +	if (!primary) {
> > > +		kfree(dcrtc);
> > >  		return -ENOMEM;
> > > +	}
> > >  
> > >  	ret = armada_drm_plane_init(primary);
> > >  	if (ret) {
> > > +		kfree(dcrtc);
> > >  		kfree(primary);
> > >  		return ret;
> > >  	}
> > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > >  				       NULL,
> > >  				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > >  	if (ret) {
> > > +		kfree(dcrtc);
> > >  		kfree(primary);
> > >  		return ret;
> > >  	}
> > >  
> > >  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> > >  					&armada_crtc_funcs, NULL);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		kfree(dcrtc);
> > >  		goto err_crtc_init;
> > 
> > The mixed use of cleaning up at every exit and the lone label is a bit messy and
> > prone to bugs such as the one you're fixing. How about you label everything
> > and then just clean up once?
> 
> It's not that simple - see alternative patch below.  How we clean
> stuff up depends on how far through the initialisation we are, and
> that doesn't lend itself to a sequential cleanup methodology.

It can be that simple, depending on how you setup your initialization sequence.
For instance, if armada_plane_init did the allocation as well as
universal_plane_init (like armada_overlay_plane_create already does), then you
wouldn't need to worry about kfree(primary) at all.

> 
> > It also seems like you're missing cleanup if armada_overlay_plane_create()
> > fails?
> 
> It's not missing.  At that point, the CRTC has been registered, and any
> failure will be cleaned up by the drm_mode_config_cleanup() in the
> error paths in armada_drm_bind() in armada_drv.c
> 
> It's rather non-obvious because it uses the ->destroy callbacks.

Got it, thanks!

Sean

> 
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index d987198a2ac6..79ce877bf45f 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  
>  	ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc",
>  			       dcrtc);
> -	if (ret < 0) {
> -		kfree(dcrtc);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto err_crtc;
>  
>  	if (dcrtc->variant->init) {
>  		ret = dcrtc->variant->init(dcrtc, dev);
> -		if (ret) {
> -			kfree(dcrtc);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_crtc;
>  	}
>  
>  	/* Ensure AXI pipeline is enabled */
> @@ -1247,15 +1243,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
>  	if (!primary) {
> -		kfree(dcrtc);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_crtc;
>  	}
>  
>  	ret = armada_drm_plane_init(primary);
>  	if (ret) {
> -		kfree(dcrtc);
>  		kfree(primary);
> -		return ret;
> +		goto err_crtc;
>  	}
>  
>  	ret = drm_universal_plane_init(drm, &primary->base, 0,
> @@ -1265,17 +1260,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  				       NULL,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
> -		kfree(dcrtc);
>  		kfree(primary);
> -		return ret;
> +		goto err_crtc;
>  	}
>  
>  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
>  					&armada_crtc_funcs, NULL);
> -	if (ret) {
> -		kfree(dcrtc);
> +	if (ret)
>  		goto err_crtc_init;
> -	}
>  
>  	drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs);
>  
> @@ -1288,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
>  
>  err_crtc_init:
>  	primary->base.funcs->destroy(&primary->base);
> +err_crtc:
> +	kfree(dcrtc);
> +
>  	return ret;
>  }
>  
> 
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Russell King - ARM Linux]

On Wed, Nov 29, 2017 at 03:03:09PM -0500, Sean Paul wrote:
> On Wed, Nov 29, 2017 at 07:16:43PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> > > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > > > Fix the leak of the CRTC structure in the failure paths of
> > > > armada_drm_crtc_create().
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > > > index 2e065facdce7..844d488b6654 100644
> > > > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > > > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > >  	dcrtc->crtc.port = port;
> > > >  
> > > >  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > > -	if (!primary)
> > > > +	if (!primary) {
> > > > +		kfree(dcrtc);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	ret = armada_drm_plane_init(primary);
> > > >  	if (ret) {
> > > > +		kfree(dcrtc);
> > > >  		kfree(primary);
> > > >  		return ret;
> > > >  	}
> > > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > >  				       NULL,
> > > >  				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > > >  	if (ret) {
> > > > +		kfree(dcrtc);
> > > >  		kfree(primary);
> > > >  		return ret;
> > > >  	}
> > > >  
> > > >  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> > > >  					&armada_crtc_funcs, NULL);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > > +		kfree(dcrtc);
> > > >  		goto err_crtc_init;
> > > 
> > > The mixed use of cleaning up at every exit and the lone label is a bit messy and
> > > prone to bugs such as the one you're fixing. How about you label everything
> > > and then just clean up once?
> > 
> > It's not that simple - see alternative patch below.  How we clean
> > stuff up depends on how far through the initialisation we are, and
> > that doesn't lend itself to a sequential cleanup methodology.
> 
> It can be that simple, depending on how you setup your initialization sequence.
> For instance, if armada_plane_init did the allocation as well as
> universal_plane_init (like armada_overlay_plane_create already does), then you
> wouldn't need to worry about kfree(primary) at all.

Two problems with that:

1. The underlying plane structures for the primary and overlay planes
   are different.

2. the calls to drm_universal_plane_init() are rather different:

- primary:
        ret = drm_universal_plane_init(drm, &primary->base, 0,
                                       &armada_primary_plane_funcs,
                                       armada_primary_formats,
                                       ARRAY_SIZE(armada_primary_formats),
                                       NULL,
                                       DRM_PLANE_TYPE_PRIMARY, NULL);

- overlay:
        ret = drm_universal_plane_init(dev, &dplane->base.base, crtcs,
                                       &armada_ovl_plane_funcs,
                                       armada_ovl_formats,
                                       ARRAY_SIZE(armada_ovl_formats),
                                       NULL,
                                       DRM_PLANE_TYPE_OVERLAY, NULL);

Moving the drm_universal_plane_init() call into armada_drm_plane_init()
in a patch to fix a leak doesn't make sense - you're actually asking for
a massive rewrite of the code (I do have patches which I'm intending to
send after this series which do quite a lot of re-organisation of this
plane code, but I want to get the fixes sorted before I post that.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Sean Paul]

On Wed, Nov 29, 2017 at 08:11:53PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 29, 2017 at 03:03:09PM -0500, Sean Paul wrote:
> > On Wed, Nov 29, 2017 at 07:16:43PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> > > > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > > > > Fix the leak of the CRTC structure in the failure paths of
> > > > > armada_drm_crtc_create().
> > > > > 
> > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > > ---
> > > > >  drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > > > > index 2e065facdce7..844d488b6654 100644
> > > > > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > > > > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > > > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > > >  	dcrtc->crtc.port = port;
> > > > >  
> > > > >  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > > > -	if (!primary)
> > > > > +	if (!primary) {
> > > > > +		kfree(dcrtc);
> > > > >  		return -ENOMEM;
> > > > > +	}
> > > > >  
> > > > >  	ret = armada_drm_plane_init(primary);
> > > > >  	if (ret) {
> > > > > +		kfree(dcrtc);
> > > > >  		kfree(primary);
> > > > >  		return ret;
> > > > >  	}
> > > > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > > >  				       NULL,
> > > > >  				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > > > >  	if (ret) {
> > > > > +		kfree(dcrtc);
> > > > >  		kfree(primary);
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > >  	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> > > > >  					&armada_crtc_funcs, NULL);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > > +		kfree(dcrtc);
> > > > >  		goto err_crtc_init;
> > > > 
> > > > The mixed use of cleaning up at every exit and the lone label is a bit messy and
> > > > prone to bugs such as the one you're fixing. How about you label everything
> > > > and then just clean up once?
> > > 
> > > It's not that simple - see alternative patch below.  How we clean
> > > stuff up depends on how far through the initialisation we are, and
> > > that doesn't lend itself to a sequential cleanup methodology.
> > 
> > It can be that simple, depending on how you setup your initialization sequence.
> > For instance, if armada_plane_init did the allocation as well as
> > universal_plane_init (like armada_overlay_plane_create already does), then you
> > wouldn't need to worry about kfree(primary) at all.
> 
> Two problems with that:
> 
> 1. The underlying plane structures for the primary and overlay planes
>    are different.
> 
> 2. the calls to drm_universal_plane_init() are rather different:
> 
> - primary:
>         ret = drm_universal_plane_init(drm, &primary->base, 0,
>                                        &armada_primary_plane_funcs,
>                                        armada_primary_formats,
>                                        ARRAY_SIZE(armada_primary_formats),
>                                        NULL,
>                                        DRM_PLANE_TYPE_PRIMARY, NULL);
> 
> - overlay:
>         ret = drm_universal_plane_init(dev, &dplane->base.base, crtcs,
>                                        &armada_ovl_plane_funcs,
>                                        armada_ovl_formats,
>                                        ARRAY_SIZE(armada_ovl_formats),
>                                        NULL,
>                                        DRM_PLANE_TYPE_OVERLAY, NULL);
> 
> Moving the drm_universal_plane_init() call into armada_drm_plane_init()
> in a patch to fix a leak doesn't make sense - you're actually asking for
> a massive rewrite of the code (I do have patches which I'm intending to
> send after this series which do quite a lot of re-organisation of this
> plane code, but I want to get the fixes sorted before I post that.)

Well, not a massive rewrite, but more code than the current patch. At any
rate, I just thought it would be helpful to suggest ways of improving your
cleanup paths. 

I didn't know you had a bigger reorg coming down the pipe. Feel free to
take or leave the advice as you see fit!

Sean

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/armada: fix leak of crtc structure

[Russell King]

Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_crtc.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 2a4d163ac76f..79ce877bf45f 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 	ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc",
 			       dcrtc);
-	if (ret < 0) {
-		kfree(dcrtc);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_crtc;
 
 	if (dcrtc->variant->init) {
 		ret = dcrtc->variant->init(dcrtc, dev);
-		if (ret) {
-			kfree(dcrtc);
-			return ret;
-		}
+		if (ret)
+			goto err_crtc;
 	}
 
 	/* Ensure AXI pipeline is enabled */
@@ -1246,13 +1242,15 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->crtc.port = port;
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-	if (!primary)
-		return -ENOMEM;
+	if (!primary) {
+		ret = -ENOMEM;
+		goto err_crtc;
+	}
 
 	ret = armada_drm_plane_init(primary);
 	if (ret) {
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_universal_plane_init(drm, &primary->base, 0,
@@ -1263,7 +1261,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
@@ -1282,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 err_crtc_init:
 	primary->base.funcs->destroy(&primary->base);
+err_crtc:
+	kfree(dcrtc);
+
 	return ret;
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel