Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

Hi Yixun, thanks for the review

2018-08-03 16:14 GMT+02:00 Yixun Lan :
>> +config MESON_CANVAS
>> +   bool "Amlogic Meson Canvas driver"
> shouldn't this a 'tristate'? since you'd make the driver a kernel module..

Yep it should!

I well noted your other feedback and v2 will have fixes for them :) .

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

Hi Yixun, thanks for the review

2018-08-03 16:14 GMT+02:00 Yixun Lan :
>> +config MESON_CANVAS
>> +   bool "Amlogic Meson Canvas driver"
> shouldn't this a 'tristate'? since you'd make the driver a kernel module..

Yep it should!

I well noted your other feedback and v2 will have fixes for them :) .

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Yixun Lan]

HI Maxime

thanks for contributing the patches ;-)

On Thu, Aug 2, 2018 at 2:51 AM, Maxime Jourdan  wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
>
> They contain metadata like width, height, block mode, endianness [..]
>
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.
>
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>
> +config MESON_CANVAS
> +   bool "Amlogic Meson Canvas driver"
shouldn't this a 'tristate'? since you'd make the driver a kernel module..

> +   depends on ARCH_MESON || COMPILE_TEST
> +   default ARCH_MESON
> +   help
> + Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
> bool "Amlogic Meson GX SoC Information driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
use SPDX license header as Neil already mentioned
check doc: Documentation/process/license-rules.rst

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL  0x48 /* 0x12 offset in data sheet */
> +   #define CANVAS_WIDTH_LBIT   29
> +   #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH  0x4c /* 0x13 offset in data sheet */
> +   #define CANVAS_WIDTH_HBIT   0
> +   #define CANVAS_HEIGHT_BIT   9
> +   #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR   0x50 /* 0x14 offset in data sheet */
> +   #define CANVAS_LUT_WR_EN(0x2 << 8)
> +   #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> +   struct device *dev;
> +   struct regmap *regmap_dmc;
> +   struct mutex lock;
> +   u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> +   uint32_t stride, uint32_t height,
> +   unsigned int wrap,
> +   unsigned int blkmode,
> +   unsigned int endian)
use "./scripts/checkpatch.pl --strict" to check
you will get a few complaints..

> +{
> +   struct regmap *regmap = canvas.regmap_dmc;
> +   u32 val;
> +
> +   mutex_lock();
> +
> +   if (!canvas.used[canvas_index]) {
> +   dev_err(canvas.dev,
> +   "Trying to setup non allocated canvas %u\n",
> +   canvas_index);
> +   mutex_unlock();
> +   return -EINVAL;
> +   }
> +
> +   regmap_write(regmap, DMC_CAV_LUT_DATAL,
> +   ((addr + 7) >> 3) |
> +   (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> +   regmap_write(regmap, 

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Yixun Lan]

HI Maxime

thanks for contributing the patches ;-)

On Thu, Aug 2, 2018 at 2:51 AM, Maxime Jourdan  wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
>
> They contain metadata like width, height, block mode, endianness [..]
>
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.
>
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>
> +config MESON_CANVAS
> +   bool "Amlogic Meson Canvas driver"
shouldn't this a 'tristate'? since you'd make the driver a kernel module..

> +   depends on ARCH_MESON || COMPILE_TEST
> +   default ARCH_MESON
> +   help
> + Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
> bool "Amlogic Meson GX SoC Information driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
use SPDX license header as Neil already mentioned
check doc: Documentation/process/license-rules.rst

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL  0x48 /* 0x12 offset in data sheet */
> +   #define CANVAS_WIDTH_LBIT   29
> +   #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH  0x4c /* 0x13 offset in data sheet */
> +   #define CANVAS_WIDTH_HBIT   0
> +   #define CANVAS_HEIGHT_BIT   9
> +   #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR   0x50 /* 0x14 offset in data sheet */
> +   #define CANVAS_LUT_WR_EN(0x2 << 8)
> +   #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> +   struct device *dev;
> +   struct regmap *regmap_dmc;
> +   struct mutex lock;
> +   u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> +   uint32_t stride, uint32_t height,
> +   unsigned int wrap,
> +   unsigned int blkmode,
> +   unsigned int endian)
use "./scripts/checkpatch.pl --strict" to check
you will get a few complaints..

> +{
> +   struct regmap *regmap = canvas.regmap_dmc;
> +   u32 val;
> +
> +   mutex_lock();
> +
> +   if (!canvas.used[canvas_index]) {
> +   dev_err(canvas.dev,
> +   "Trying to setup non allocated canvas %u\n",
> +   canvas_index);
> +   mutex_unlock();
> +   return -EINVAL;
> +   }
> +
> +   regmap_write(regmap, DMC_CAV_LUT_DATAL,
> +   ((addr + 7) >> 3) |
> +   (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> +   regmap_write(regmap, 

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

Hi Neil,

2018-08-02 10:38 GMT+02:00 Neil Armstrong :
> Please switch to the spdx header format here and in the .h.

> In the DRM driver these are updated in IRQ context, we should make sure we 
> don't sleep
> in interrupt context if IRQ occurs when the VDEC updates it's canvases.
>
> Could you switch to spin_lock_irqsave() instead ?

Will do.

> Can you add the endian defines ?
>
> #define MESON_CANVAS_ENDIAN_SWAP16  0x1
> #define MESON_CANVAS_ENDIAN_SWAP32  0x3
> #define MESON_CANVAS_ENDIAN_SWAP64  0x7
> #define MESON_CANVAS_ENDIAN_SWAP128 0xf
>
> the SWAP64 is the one used in the VDEC and DRM Overlays.

Sure. Maybe I should push the flags (0x1, 0x2, 0x4, 0x8) rather than
the combinations?
Or both, since the combinations are more likely to be used in practice
more than the single flags.

> I rebased my DRM Overlay patch on top of this and :
> Tested-by: Neil Armstrong 
>
> You can keep this tag on future versions.

Thanks!

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

Hi Neil,

2018-08-02 10:38 GMT+02:00 Neil Armstrong :
> Please switch to the spdx header format here and in the .h.

> In the DRM driver these are updated in IRQ context, we should make sure we 
> don't sleep
> in interrupt context if IRQ occurs when the VDEC updates it's canvases.
>
> Could you switch to spin_lock_irqsave() instead ?

Will do.

> Can you add the endian defines ?
>
> #define MESON_CANVAS_ENDIAN_SWAP16  0x1
> #define MESON_CANVAS_ENDIAN_SWAP32  0x3
> #define MESON_CANVAS_ENDIAN_SWAP64  0x7
> #define MESON_CANVAS_ENDIAN_SWAP128 0xf
>
> the SWAP64 is the one used in the VDEC and DRM Overlays.

Sure. Maybe I should push the flags (0x1, 0x2, 0x4, 0x8) rather than
the combinations?
Or both, since the combinations are more likely to be used in practice
more than the single flags.

> I rebased my DRM Overlay patch on top of this and :
> Tested-by: Neil Armstrong 
>
> You can keep this tag on future versions.

Thanks!

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Neil Armstrong]

Hi Maxime,

On 01/08/2018 20:51, Maxime Jourdan wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
> 
> They contain metadata like width, height, block mode, endianness [..]
> 
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.

I rebased my DRM Overlay patch on top of this and :
Tested-by: Neil Armstrong 

You can keep this tag on future versions.

The tree is available at : 
https://github.com/superna/linux/commits/amlogic/v4.19/drm-overlay-canvas

Neil

> 
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
> 
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>  
> +config MESON_CANVAS
> + bool "Amlogic Meson Canvas driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + default ARCH_MESON
> + help
> +   Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
>   bool "Amlogic Meson GX SoC Information driver"
>   depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL0x48 /* 0x12 offset in data sheet */
> + #define CANVAS_WIDTH_LBIT   29
> + #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH0x4c /* 0x13 offset in data sheet */
> + #define CANVAS_WIDTH_HBIT   0
> + #define CANVAS_HEIGHT_BIT   9
> + #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR 0x50 /* 0x14 offset in data sheet */
> + #define CANVAS_LUT_WR_EN(0x2 << 8)
> + #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> + struct device *dev;
> + struct regmap *regmap_dmc;
> + struct mutex lock;
> + u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> + uint32_t stride, uint32_t height,
> + unsigned int wrap,
> + unsigned int blkmode,
> + unsigned int endian)
> +{
> + struct regmap *regmap = canvas.regmap_dmc;
> + u32 val;
> +
> + mutex_lock();
> +
> + if (!canvas.used[canvas_index]) {
> + dev_err(canvas.dev,
> + "Trying to setup non allocated canvas %u\n",
> + canvas_index);
> + mutex_unlock();
> + return -EINVAL;
> + }
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAL,
> + ((addr + 7) >> 3) |
> + (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAH,
> + stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
> + 

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Neil Armstrong]

Hi Maxime,

On 01/08/2018 20:51, Maxime Jourdan wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
> 
> They contain metadata like width, height, block mode, endianness [..]
> 
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.

I rebased my DRM Overlay patch on top of this and :
Tested-by: Neil Armstrong 

You can keep this tag on future versions.

The tree is available at : 
https://github.com/superna/linux/commits/amlogic/v4.19/drm-overlay-canvas

Neil

> 
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
> 
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>  
> +config MESON_CANVAS
> + bool "Amlogic Meson Canvas driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + default ARCH_MESON
> + help
> +   Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
>   bool "Amlogic Meson GX SoC Information driver"
>   depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL0x48 /* 0x12 offset in data sheet */
> + #define CANVAS_WIDTH_LBIT   29
> + #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH0x4c /* 0x13 offset in data sheet */
> + #define CANVAS_WIDTH_HBIT   0
> + #define CANVAS_HEIGHT_BIT   9
> + #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR 0x50 /* 0x14 offset in data sheet */
> + #define CANVAS_LUT_WR_EN(0x2 << 8)
> + #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> + struct device *dev;
> + struct regmap *regmap_dmc;
> + struct mutex lock;
> + u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> + uint32_t stride, uint32_t height,
> + unsigned int wrap,
> + unsigned int blkmode,
> + unsigned int endian)
> +{
> + struct regmap *regmap = canvas.regmap_dmc;
> + u32 val;
> +
> + mutex_lock();
> +
> + if (!canvas.used[canvas_index]) {
> + dev_err(canvas.dev,
> + "Trying to setup non allocated canvas %u\n",
> + canvas_index);
> + mutex_unlock();
> + return -EINVAL;
> + }
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAL,
> + ((addr + 7) >> 3) |
> + (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAH,
> + stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
> + 

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Neil Armstrong]

Hi Maxime,

On 01/08/2018 20:51, Maxime Jourdan wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
> 
> They contain metadata like width, height, block mode, endianness [..]
> 
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.
> 
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
> 
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>  
> +config MESON_CANVAS
> + bool "Amlogic Meson Canvas driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + default ARCH_MESON
> + help
> +   Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
>   bool "Amlogic Meson GX SoC Information driver"
>   depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */

Please switch to the spdx header format here and in the .h.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL0x48 /* 0x12 offset in data sheet */
> + #define CANVAS_WIDTH_LBIT   29
> + #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH0x4c /* 0x13 offset in data sheet */
> + #define CANVAS_WIDTH_HBIT   0
> + #define CANVAS_HEIGHT_BIT   9
> + #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR 0x50 /* 0x14 offset in data sheet */
> + #define CANVAS_LUT_WR_EN(0x2 << 8)
> + #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> + struct device *dev;
> + struct regmap *regmap_dmc;
> + struct mutex lock;
> + u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> + uint32_t stride, uint32_t height,
> + unsigned int wrap,
> + unsigned int blkmode,
> + unsigned int endian)
> +{
> + struct regmap *regmap = canvas.regmap_dmc;
> + u32 val;
> +
> + mutex_lock();

In the DRM driver these are updated in IRQ context, we should make sure we 
don't sleep
in interrupt context if IRQ occurs when the VDEC updates it's canvases.

Could you switch to spin_lock_irqsave() instead ?

> +
> + if (!canvas.used[canvas_index]) {
> + dev_err(canvas.dev,
> + "Trying to setup non allocated canvas %u\n",
> + canvas_index);
> + mutex_unlock();
> + return -EINVAL;
> + }
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAL,
> + ((addr + 7) >> 3) |
> + (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAH,
> + stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
> +

Re: [PATCH 1/4] soc: amlogic: add meson-canvas driver

[Neil Armstrong]

Hi Maxime,

On 01/08/2018 20:51, Maxime Jourdan wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
> 
> They contain metadata like width, height, block mode, endianness [..]
> 
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.
> 
> Signed-off-by: Maxime Jourdan 
> ---
>  drivers/soc/amlogic/Kconfig  |   7 +
>  drivers/soc/amlogic/Makefile |   1 +
>  drivers/soc/amlogic/meson-canvas.c   | 182 +++
>  include/linux/soc/amlogic/meson-canvas.h |  37 +
>  4 files changed, 227 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
> 
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..5bd049899d88 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>  
> +config MESON_CANVAS
> + bool "Amlogic Meson Canvas driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + default ARCH_MESON
> + help
> +   Say yes to support the canvas IP within Amlogic Meson Soc family.
> +
>  config MESON_GX_SOCINFO
>   bool "Amlogic Meson GX SoC Information driver"
>   depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c 
> b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index ..671eb89c8904
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (C) 2018 Maxime Jourdan
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong 
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */

Please switch to the spdx header format here and in the .h.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL0x48 /* 0x12 offset in data sheet */
> + #define CANVAS_WIDTH_LBIT   29
> + #define CANVAS_WIDTH_LWID   3
> +#define DMC_CAV_LUT_DATAH0x4c /* 0x13 offset in data sheet */
> + #define CANVAS_WIDTH_HBIT   0
> + #define CANVAS_HEIGHT_BIT   9
> + #define CANVAS_BLKMODE_BIT  24
> +#define DMC_CAV_LUT_ADDR 0x50 /* 0x14 offset in data sheet */
> + #define CANVAS_LUT_WR_EN(0x2 << 8)
> + #define CANVAS_LUT_RD_EN(0x1 << 8)
> +
> +struct meson_canvas {
> + struct device *dev;
> + struct regmap *regmap_dmc;
> + struct mutex lock;
> + u8 used[NUM_CANVAS];
> +};
> +
> +static struct meson_canvas canvas = { 0 };
> +
> +static int meson_canvas_setup(uint8_t canvas_index, uint32_t addr,
> + uint32_t stride, uint32_t height,
> + unsigned int wrap,
> + unsigned int blkmode,
> + unsigned int endian)
> +{
> + struct regmap *regmap = canvas.regmap_dmc;
> + u32 val;
> +
> + mutex_lock();

In the DRM driver these are updated in IRQ context, we should make sure we 
don't sleep
in interrupt context if IRQ occurs when the VDEC updates it's canvases.

Could you switch to spin_lock_irqsave() instead ?

> +
> + if (!canvas.used[canvas_index]) {
> + dev_err(canvas.dev,
> + "Trying to setup non allocated canvas %u\n",
> + canvas_index);
> + mutex_unlock();
> + return -EINVAL;
> + }
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAL,
> + ((addr + 7) >> 3) |
> + (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> + regmap_write(regmap, DMC_CAV_LUT_DATAH,
> + stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
> +