Armada DRM fixes
Subsystems (2)
Russell King - ARM Linux
maintainercover Armada DRM fixes
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(-)
Signature · 7 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/5 drm/armada: fix leak of crtc structure
Fix the leak of the CRTC structure in the failure paths of armada_drm_crtc_create().
--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +8 -2
@@ -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);
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/armada: fix leak of crtc structure
Quoted 33 lines · Russell King
Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/armada/armada_crtc.c +5
-1@@ -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);
drivers/gpu/drm/armada/armada_crtc.c static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, · line 1277
Quoted 10 lines
drivers/gpu/drm/armada/armada_crtc.c +2
-1kfree (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
Quoted 11 lines
drivers/gpu/drm/armada/armada_crtc.c +1
-0+ }drm_crtc_helper_add (&dcrtc->crtc, &armada_crtc_helper_funcs);Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Russell King - ARM Linux
maintainerRe: [PATCH 1/5] drm/armada: fix leak of crtc structure
Quoted 38 lines · Sean Paul
Quoted 37 lines · Russell King
Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/armada/armada_crtc.c +5
-1@@ -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; }
Quoted 10 lines
drivers/gpu/drm/armada/armada_crtc.c static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, · line 1277
Quoted 6 lines
drivers/gpu/drm/armada/armada_crtc.c +2
-1ret =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.
Quoted 2 lines
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.
drivers/gpu/drm/armada/armada_crtc.c +12 -17
@@ -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; }
Signature · 7 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/armada: fix leak of crtc structure
Quoted 50 lines · Russell King - ARM Linux
Quoted 38 lines · Sean Paul
Quoted 37 lines · Russell King
Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/armada/armada_crtc.c +5
-1@@ -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; }Quoted 10 lines
drivers/gpu/drm/armada/armada_crtc.c static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, · line 1270
Quoted 6 lines
drivers/gpu/drm/armada/armada_crtc.c +2
-1ret =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?
Quoted 3 lines
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.
Quoted 9 lines
Quoted 3 lines
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 inarmada_drm_bind()inarmada_drv.cIt's rather non-obvious because it uses the ->destroy callbacks.
Got it, thanks!
Sean
Quoted 82 lines
drivers/gpu/drm/armada/armada_crtc.c +12
-17@@ -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; }Signature · 4 lines
-- 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
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Russell King - ARM Linux
maintainerRe: [PATCH 1/5] drm/armada: fix leak of crtc structure
Quoted 51 lines · Sean Paul
Quoted 50 lines · Russell King - ARM Linux
Quoted 48 lines · Sean Paul
Quoted 33 lines · Russell King
Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/armada/armada_crtc.c +5
-1@@ -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);drivers/gpu/drm/armada/armada_crtc.c static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, · line 1277
Quoted 10 lines
drivers/gpu/drm/armada/armada_crtc.c +2
-1kfree (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?
Quoted 8 lines
Quoted 3 lines
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_initdid the allocation as well asuniversal_plane_init(likearmada_overlay_plane_createalready 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.)
Signature · 7 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/armada: fix leak of crtc structure
Quoted 84 lines · Russell King - ARM Linux
Quoted 51 lines · Sean Paul
Quoted 50 lines · Russell King - ARM Linux
Quoted 48 lines · Sean Paul
Quoted 33 lines · Russell King
Fix the leak of the CRTC structure in the failure paths of
armada_drm_crtc_create().Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)drivers/gpu/drm/armada/armada_crtc.c +5
-1@@ -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);drivers/gpu/drm/armada/armada_crtc.c static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, · line 1277
Quoted 10 lines
drivers/gpu/drm/armada/armada_crtc.c +2
-1kfree (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?
Quoted 8 lines
Quoted 3 lines
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_initdid the allocation as well asuniversal_plane_init(likearmada_overlay_plane_createalready 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);
Quoted 5 lines
Moving the
drm_universal_plane_init()call intoarmada_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
Quoted 5 lines
Signature · 4 lines
-- 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
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/5 drm/armada: fix SRAM powerdown
In reply to Russell King - ARM LinuxAvoid 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.
--- drivers/gpu/drm/armada/armada_crtc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +4 -5
@@ -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 ; }
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 3/5 drm/armada: fix UV swap code
In reply to Russell King - ARM LinuxThe UV swap code was not always programming things correctly when the source origin box has been offset. Fix this.
--- drivers/gpu/drm/armada/armada_crtc.h | 2 ++ drivers/gpu/drm/armada/armada_overlay.c | 38 ++++++++++++++++----------------- 2 files changed, 21 insertions(+), 19 deletions(-)
drivers/gpu/drm/armada/armada_crtc.h +2 -0
@@ -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;
drivers/gpu/drm/armada/armada_overlay.c +19 -19
@@ -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],
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 4/5 drm/armada: improve efficiency of armada_drm_plane_calc_addrs()
In reply to Russell King - ARM LinuxLookup 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().
--- drivers/gpu/drm/armada/armada_crtc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +3 -2
@@ -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 ; }
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 5/5 drm/armada: fix YUV planar format framebuffer offsets
In reply to Russell King - ARM LinuxWe weren't correctly calculating the YUV planar offsets for subsampled chroma planes correctly - fix up the coordinates for planes 1 and 2.
--- drivers/gpu/drm/armada/armada_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/gpu/drm/armada/armada_crtc.c +7 -1
@@ -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++)
Signature · 6 lines
2.7.4 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel