[PATCH] drm/tilcdc: Allocate register storage based on the actual number registers
Reviewed-By: Michael Bode
Jyri,
good idea to allocate the space for registers dynamically.
This eliminates the need of synchronizing driver and header file in an
elegant way.
Only thing I personally don't like, is potentially passing a NULL
pointer to kfree.
But I learned that this is valid kernel code.
Br,
Michael
On 07/03/2015 12:45 PM, Jyri Sarha wrote:
> Allocate suspend/resume register storage based on the actual number
> registers the driver is aware of. The static allocation for register
> storage had falen behind badly.
>
> Reported-by: Michael Bode
> Signed-off-by: Jyri Sarha
> ---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 20 +++-
> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 +-
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 4908c1f..2f87263 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -139,11 +139,14 @@ static int tilcdc_unload(struct drm_device *dev)
>
> pm_runtime_disable(dev->dev);
>
> + kfree(priv->saved_register);
> kfree(priv);
>
> return 0;
> }
>
> +static size_t tilcdc_num_regs(void);
> +
> static int tilcdc_load(struct drm_device *dev, unsigned long flags)
> {
> struct platform_device *pdev = dev->platformdev;
> @@ -155,7 +158,11 @@ static int tilcdc_load(struct drm_device *dev, unsigned
> long flags)
> int ret;
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> - if (!priv) {
> + if (priv)
> + priv->saved_register = kcalloc(sizeof(*priv->saved_register),
> +tilcdc_num_regs(), GFP_KERNEL);
> + if (!priv || !priv->saved_register) {
> + kfree(priv);
> dev_err(dev->dev, "failed to allocate private data\n");
> return -ENOMEM;
> }
> @@ -345,6 +352,7 @@ fail_free_wq:
>
> fail_free_priv:
> dev->dev_private = NULL;
> + kfree(priv->saved_register);
> kfree(priv);
> return ret;
> }
> @@ -467,6 +475,16 @@ static const struct {
> REG(2, true, LCDC_INT_ENABLE_SET_REG),
> #undef REG
> };
> +
> +static size_t tilcdc_num_regs(void)
> +{
> + return ARRAY_SIZE(registers);
> +}
> +#else
> +static size_t tilcdc_num_regs(void)
> +{
> + return 0;
> +}
> #endif
>
> #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index e863ad0..bc94835 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -67,7 +67,7 @@ struct tilcdc_drm_private {
> uint32_t max_width;
>
> /* register contents saved across suspend/resume: */
> - u32 saved_register[12];
> + u32 *saved_register;
>
> #ifdef CONFIG_CPU_FREQ
> struct notifier_block freq_transition;
-- next part --
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20150703/991d16c8/attachment.html>
TILCDC driver - Array to keep register values during PM suspend is too small
Hi,
i currently try to write a LIDD version of the TILCDC driver in
driver/gpu/drm.
During my investigations I found the function "tilcdc_pm_suspend" in
tilcdc_drv.c:
#ifdef CONFIG_PM_SLEEP
static int tilcdc_pm_suspend(struct device *dev)
{
.
/* Save register state: */
for (i = 0; i < ARRAY_SIZE(registers); i++)
if (registers[i].save && (priv->rev >= registers[i].rev))
priv->saved_register[n++] = tilcdc_read(ddev,
registers[i].reg);
priv->ctx_valid = true;
return 0;
}
The member "saved_register" that is filled with register values here is
an array defined in tilcdc_drv.h:
struct tilcdc_drm_private {
.
/* register contents saved across suspend/resume: */
u32 saved_register[12];
bool ctx_valid;
.
As you can see the size of that array is 12 times u32, so it can keep 12
register values.
But if you have a look at the register list to be saved, you will find:
#if defined(CONFIG_DEBUG_FS) || defined(CONFIG_PM_SLEEP)
static const struct {
const char *name;
uint8_t rev;
uint8_t save;
uint32_t reg;
} registers[] ={
#define REG(rev, save, reg) { #reg, rev, save, reg }
/* exists in revision 1: */
REG(1, false, LCDC_PID_REG),
REG(1, true, LCDC_CTRL_REG),
REG(1, false, LCDC_STAT_REG),
REG(1, true, LCDC_RASTER_CTRL_REG),
REG(1, true, LCDC_RASTER_TIMING_0_REG),
REG(1, true, LCDC_RASTER_TIMING_1_REG),
REG(1, true, LCDC_RASTER_TIMING_2_REG),
REG(1, true, LCDC_DMA_CTRL_REG),
REG(1, true, LCDC_DMA_FB_BASE_ADDR_0_REG),
REG(1, true, LCDC_DMA_FB_CEILING_ADDR_0_REG),
REG(1, true, LCDC_DMA_FB_BASE_ADDR_1_REG),
REG(1, true, LCDC_DMA_FB_CEILING_ADDR_1_REG),
/* new in revision 2: */
REG(2, false, LCDC_RAW_STAT_REG),
REG(2, false, LCDC_MASKED_STAT_REG),
REG(2, false, LCDC_INT_ENABLE_SET_REG),
REG(2, false, LCDC_INT_ENABLE_CLR_REG),
REG(2, false, LCDC_END_OF_INT_IND_REG),
REG(2, true, LCDC_CLK_ENABLE_REG),
REG(2, true, LCDC_INT_ENABLE_SET_REG),
#undef REG
};
#endif
And if I count correctly in case you have an LCDC Revision 2, there will
be 19 registers saved to the array.
So probably you will write over the end of that array.
I hope the form of my post fits the posting rules of this forum. If not,
sorry. It is my very first post!
Br,
Michael
