[PATCH 4/4] drm/msm: add OCMEM driver

[Stephen Boyd]

On 09/28, Rob Clark wrote:
> On Mon, Sep 28, 2015 at 6:10 PM, Stephen Boyd  wrote:
> > On 09/28, Rob Clark wrote:
> >> diff --git a/drivers/gpu/drm/msm/ocmem/ocmem.c 
> >> b/drivers/gpu/drm/msm/ocmem/ocmem.c
> >> new file mode 100644
> >> index 000..d3cdd64
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/ocmem/ocmem.c
> >> @@ -0,0 +1,396 @@
> >> +/*
> >> + * Copyright (C) 2015 Red Hat
> >> + * Author: Rob Clark 
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms of the GNU General Public License version 2 as 
> >> published by
> >> + * the Free Software Foundation.
> >> + *
> >> + * 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 
> >
> > What is this include for?
> 
> needed for qcom_scm.h, although I guess I could just add the missing
> #includes in qcom_scm.h instead..

Ok, we should fix that in scm header files. It probably needs a
forward declare of struct cpumask and it should be struct cpumask
* instead of cpumask_t *.

> 
> >> +#include 
> >> +
> >> +#include "msm_drv.h"
> >> +#include "ocmem.h"
> >> +#include "ocmem.xml.h"
> >> +
[..]
> >> +
> >> +static void update_ocmem(struct ocmem *ocmem)
> >> +{
> >> + uint32_t region_mode_ctrl = 0x0;
> >> + unsigned pos = 0;
> >> + unsigned i = 0;
> >> +
> >> + if (!qcom_scm_ocmem_lock_available()) {
> >> + for (i = 0; i < ocmem->config->num_regions; i++) {
> >> + struct ocmem_region *region = >regions[i];
> >> + pos = i << 2;
> >> + if (region->mode == THIN_MODE)
> >> + region_mode_ctrl |= BIT(pos);
> >> + }
> >> + dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n", 
> >> region_mode_ctrl);
> >> + ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, 
> >> region_mode_ctrl);
> >> + /* Barrier to commit the region mode */
> >> + mb();
> >
> > msm_writel() already has a barrier, so now we have a double
> > barrier?
> 
> hmm, msm_writel() doesn't have any more barrier than writel().. so I
> kept the mb() from downstream..

Yes writel() already has a barrier. Downstream is using
writel_relaxed() instead of writel() in ocmem_write() and then
adding the barrier explicitly after ocmem_write() in the right
places.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/4] drm/msm: add OCMEM driver

[Rob Clark]

On Mon, Sep 28, 2015 at 6:10 PM, Stephen Boyd  wrote:
> On 09/28, Rob Clark wrote:
>> @@ -322,10 +319,8 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>>
>>   adreno_gpu_cleanup(adreno_gpu);
>>
>> -#ifdef CONFIG_MSM_OCMEM
>>   if (a3xx_gpu->ocmem_base)
>
> Is this supposed to be ocmem_base or ocmem_hdl? Perhaps this
> check could be put inside the ocmem_free() itself so that the
> caller doesn't have to care.

yeah, should be ocmem_hdl

I would kind of prefer to keep the check for null in the caller, just
to simplify backports to 3.10 kernel (since otherwise the API matches
downstream).. Although I guess downstream checks for null and spits
out error msg and returns -EINVAL, so maybe that is enough..

>>   ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
>> -#endif
>>
>>   kfree(a3xx_gpu);
>>  }
>> @@ -289,10 +288,8 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>>
>>   adreno_gpu_cleanup(adreno_gpu);
>>
>> -#ifdef CONFIG_MSM_OCMEM
>> - if (a4xx_gpu->ocmem_base)
>> + if (a4xx_gpu->ocmem_hdl)
>
> This one changed, so a3xx above seems highly suspicious.
>
>>   ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
>> -#endif
>>
>>   kfree(a4xx_gpu);
>>  }
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
>> index 2bbe85a..f042ba8 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.h
>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
>> @@ -172,4 +172,7 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev);
>>  void __init adreno_register(void);
>>  void __exit adreno_unregister(void);
>>
>> +void __init ocmem_register(void);
>> +void __exit ocmem_unregister(void);
>
> __init and __exit in header files is useless
>
>> +
>>  #endif /* __MSM_GPU_H__ */
>> diff --git a/drivers/gpu/drm/msm/ocmem/ocmem.c 
>> b/drivers/gpu/drm/msm/ocmem/ocmem.c
>> new file mode 100644
>> index 000..d3cdd64
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/ocmem/ocmem.c
>> @@ -0,0 +1,396 @@
>> +/*
>> + * Copyright (C) 2015 Red Hat
>> + * Author: Rob Clark 
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published 
>> by
>> + * the Free Software Foundation.
>> + *
>> + * 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 
>
> What is this include for?

needed for qcom_scm.h, although I guess I could just add the missing
#includes in qcom_scm.h instead..

>> +#include 
>> +
>> +#include "msm_drv.h"
>> +#include "ocmem.h"
>> +#include "ocmem.xml.h"
>> +
>> +enum region_mode {
>> + WIDE_MODE = 0x0,
>> + THIN_MODE,
>> + MODE_DEFAULT = WIDE_MODE,
>> +};
>> +
>> +enum ocmem_tz_client {
>> + TZ_UNUSED = 0x0,
>> + TZ_GRAPHICS,
>> + TZ_VIDEO,
>> + TZ_LP_AUDIO,
>> + TZ_SENSORS,
>> + TZ_OTHER_OS,
>> + TZ_DEBUG,
>> +};
>> +
>> +struct ocmem_region {
>> + unsigned psgsc_ctrl;
>> + bool interleaved;
>> + enum region_mode mode;
>> + unsigned int num_macros;
>> + enum ocmem_macro_state macro_state[4];
>> + unsigned long macro_size;
>> + unsigned long region_size;
>> +};
>> +
>> +struct ocmem_config {
>> + uint8_t  num_regions;
>> + uint32_t macro_size;
>> +};
>> +
>> +struct ocmem {
>> + struct device *dev;
>> + const struct ocmem_config *config;
>> + struct resource *ocmem_mem;
>> + struct clk *core_clk;
>> + struct clk *iface_clk;
>> + void __iomem *mmio;
>> +
>> + unsigned num_ports;
>
> Is this used after probe?

not currently.. downstream was saving it off in pdata but on closer
look it doesn't seem to use it after probe either..

>> + unsigned num_macros;
>> + bool interleaved;
>
> Is this used after probe?

again, cargo culted from downstream, but it looks like we can drop..

>> +
>> + struct ocmem_region *regions;
>> +};
>> +
>> +struct ocmem *ocmem;
>
> static?
>
>> +
>> +static bool ocmem_exists(void);
>> +
>> +static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
>> +{
>> + msm_writel(data, ocmem->mmio + reg);
>> +}
>> +
>> +static inline u32 ocmem_read(struct ocmem *ocmem, u32 reg)
>> +{
>> + return msm_readl(ocmem->mmio + reg);
>> +}
>> +
>> +static int ocmem_clk_enable(struct ocmem *ocmem)
>> +{
>> + int ret;
>> +
>> + ret = clk_prepare_enable(ocmem->core_clk);
>> + if (ret)
>> + return ret;
>> +
>> + if (ocmem->iface_clk) {
>> + ret = clk_prepare_enable(ocmem->iface_clk);
>
> clk_prepare_enable() on NULL does nothing so it should be safe to
> drop the if.
>
>> + if (ret)
>> + 

[PATCH 4/4] drm/msm: add OCMEM driver

[Stephen Boyd]

On 09/28, Rob Clark wrote:
> @@ -322,10 +319,8 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>   adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
>   if (a3xx_gpu->ocmem_base)

Is this supposed to be ocmem_base or ocmem_hdl? Perhaps this
check could be put inside the ocmem_free() itself so that the
caller doesn't have to care.

>   ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
>  
>   kfree(a3xx_gpu);
>  }
> @@ -289,10 +288,8 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>   adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> - if (a4xx_gpu->ocmem_base)
> + if (a4xx_gpu->ocmem_hdl)

This one changed, so a3xx above seems highly suspicious.

>   ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
>  
>   kfree(a4xx_gpu);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 2bbe85a..f042ba8 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -172,4 +172,7 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev);
>  void __init adreno_register(void);
>  void __exit adreno_unregister(void);
>  
> +void __init ocmem_register(void);
> +void __exit ocmem_unregister(void);

__init and __exit in header files is useless

> +
>  #endif /* __MSM_GPU_H__ */
> diff --git a/drivers/gpu/drm/msm/ocmem/ocmem.c 
> b/drivers/gpu/drm/msm/ocmem/ocmem.c
> new file mode 100644
> index 000..d3cdd64
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/ocmem/ocmem.c
> @@ -0,0 +1,396 @@
> +/*
> + * Copyright (C) 2015 Red Hat
> + * Author: Rob Clark 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * 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 

What is this include for?

> +#include 
> +
> +#include "msm_drv.h"
> +#include "ocmem.h"
> +#include "ocmem.xml.h"
> +
> +enum region_mode {
> + WIDE_MODE = 0x0,
> + THIN_MODE,
> + MODE_DEFAULT = WIDE_MODE,
> +};
> +
> +enum ocmem_tz_client {
> + TZ_UNUSED = 0x0,
> + TZ_GRAPHICS,
> + TZ_VIDEO,
> + TZ_LP_AUDIO,
> + TZ_SENSORS,
> + TZ_OTHER_OS,
> + TZ_DEBUG,
> +};
> +
> +struct ocmem_region {
> + unsigned psgsc_ctrl;
> + bool interleaved;
> + enum region_mode mode;
> + unsigned int num_macros;
> + enum ocmem_macro_state macro_state[4];
> + unsigned long macro_size;
> + unsigned long region_size;
> +};
> +
> +struct ocmem_config {
> + uint8_t  num_regions;
> + uint32_t macro_size;
> +};
> +
> +struct ocmem {
> + struct device *dev;
> + const struct ocmem_config *config;
> + struct resource *ocmem_mem;
> + struct clk *core_clk;
> + struct clk *iface_clk;
> + void __iomem *mmio;
> +
> + unsigned num_ports;

Is this used after probe?

> + unsigned num_macros;
> + bool interleaved;

Is this used after probe?

> +
> + struct ocmem_region *regions;
> +};
> +
> +struct ocmem *ocmem;

static?

> +
> +static bool ocmem_exists(void);
> +
> +static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
> +{
> + msm_writel(data, ocmem->mmio + reg);
> +}
> +
> +static inline u32 ocmem_read(struct ocmem *ocmem, u32 reg)
> +{
> + return msm_readl(ocmem->mmio + reg);
> +}
> +
> +static int ocmem_clk_enable(struct ocmem *ocmem)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(ocmem->core_clk);
> + if (ret)
> + return ret;
> +
> + if (ocmem->iface_clk) {
> + ret = clk_prepare_enable(ocmem->iface_clk);

clk_prepare_enable() on NULL does nothing so it should be safe to
drop the if.

> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void update_ocmem(struct ocmem *ocmem)
> +{
> + uint32_t region_mode_ctrl = 0x0;
> + unsigned pos = 0;
> + unsigned i = 0;
> +
> + if (!qcom_scm_ocmem_lock_available()) {
> + for (i = 0; i < ocmem->config->num_regions; i++) {
> + struct ocmem_region *region = >regions[i];
> + pos = i << 2;
> + if (region->mode == THIN_MODE)
> + region_mode_ctrl |= BIT(pos);
> + }
> + dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n", 
> region_mode_ctrl);
> + ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, region_mode_ctrl);
> + /* Barrier to commit the region mode */
> + 

[PATCH 4/4] drm/msm: add OCMEM driver

[Rob Clark]

For now, since the GPU is the only upstream consumer, just stuff this
into drm/msm.  Eventually if we have other consumers, we'll have to
split this out and make the allocation less hard coded.  But I'll punt
on that until I better understand the non-gpu uses-cases (and whether
the allocation *really* needs to be as complicated as it is in the
downstream driver).

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/Makefile  |   3 +-
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c |  17 +-
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c |  19 +-
 drivers/gpu/drm/msm/msm_drv.c |   2 +
 drivers/gpu/drm/msm/msm_gpu.h |   3 +
 drivers/gpu/drm/msm/ocmem/ocmem.c | 396 ++
 drivers/gpu/drm/msm/ocmem/ocmem.h |  46 
 7 files changed, 459 insertions(+), 27 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/ocmem/ocmem.c
 create mode 100644 drivers/gpu/drm/msm/ocmem/ocmem.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 0a543eb..8ddf6fa 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -48,7 +48,8 @@ msm-y := \
msm_iommu.o \
msm_perf.o \
msm_rd.o \
-   msm_ringbuffer.o
+   msm_ringbuffer.o \
+   ocmem/ocmem.o

 msm-$(CONFIG_DRM_MSM_FBDEV) += msm_fbdev.o
 msm-$(CONFIG_COMMON_CLK) += mdp/mdp4/mdp4_lvds_pll.o
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index ca29688..29bbb80 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -17,10 +17,7 @@
  * this program.  If not, see .
  */

-#ifdef CONFIG_MSM_OCMEM
-#  include 
-#endif
-
+#include "ocmem/ocmem.h"
 #include "a3xx_gpu.h"

 #define A3XX_INT0_MASK \
@@ -322,10 +319,8 @@ static void a3xx_destroy(struct msm_gpu *gpu)

adreno_gpu_cleanup(adreno_gpu);

-#ifdef CONFIG_MSM_OCMEM
if (a3xx_gpu->ocmem_base)
ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
-#endif

kfree(a3xx_gpu);
 }
@@ -539,6 +534,7 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
struct msm_gpu *gpu;
struct msm_drm_private *priv = dev->dev_private;
struct platform_device *pdev = priv->gpu_pdev;
+   struct ocmem_buf *ocmem_hdl;
int ret;

if (!pdev) {
@@ -569,18 +565,13 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
goto fail;

/* if needed, allocate gmem: */
-   if (adreno_is_a330(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-   /* TODO this is different/missing upstream: */
-   struct ocmem_buf *ocmem_hdl =
-   ocmem_allocate(OCMEM_GRAPHICS, 
adreno_gpu->gmem);
-
+   ocmem_hdl = ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
+   if (!IS_ERR(ocmem_hdl)) {
a3xx_gpu->ocmem_hdl = ocmem_hdl;
a3xx_gpu->ocmem_base = ocmem_hdl->addr;
adreno_gpu->gmem = ocmem_hdl->len;
DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
a3xx_gpu->ocmem_base);
-#endif
}

if (!gpu->mmu) {
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index a53f1be..17f084d 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -10,10 +10,9 @@
  * GNU General Public License for more details.
  *
  */
+
+#include "ocmem/ocmem.h"
 #include "a4xx_gpu.h"
-#ifdef CONFIG_MSM_OCMEM
-#  include 
-#endif

 #define A4XX_INT0_MASK \
(A4XX_INT0_RBBM_AHB_ERROR |\
@@ -289,10 +288,8 @@ static void a4xx_destroy(struct msm_gpu *gpu)

adreno_gpu_cleanup(adreno_gpu);

-#ifdef CONFIG_MSM_OCMEM
-   if (a4xx_gpu->ocmem_base)
+   if (a4xx_gpu->ocmem_hdl)
ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
-#endif

kfree(a4xx_gpu);
 }
@@ -538,6 +535,7 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
struct msm_gpu *gpu;
struct msm_drm_private *priv = dev->dev_private;
struct platform_device *pdev = priv->gpu_pdev;
+   struct ocmem_buf *ocmem_hdl;
int ret;

if (!pdev) {
@@ -568,18 +566,13 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
goto fail;

/* if needed, allocate gmem: */
-   if (adreno_is_a4xx(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-   /* TODO this is different/missing upstream: */
-   struct ocmem_buf *ocmem_hdl =
-   ocmem_allocate(OCMEM_GRAPHICS, 
adreno_gpu->gmem);
-
+   ocmem_hdl = ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
+   if (!IS_ERR(ocmem_hdl)) {
a4xx_gpu->ocmem_hdl = ocmem_hdl;
a4xx_gpu->ocmem_base = ocmem_hdl->addr;
adreno_gpu->gmem = ocmem_hdl->len;
DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,