subject:"Re\: \[PATCH v3 10\/23\] media\: camss\: Add VFE files"

Re: [PATCH v3 10/23] media: camss: Add VFE files

2017-08-07 Thread Todor Tomov

Hi Sakari,

On  4.08.2017 21:02, Sakari Ailus wrote:
> Hi Todor,
> 
> Todor Tomov wrote:
>> Hi Sakari,
>>
>> Thank you for the review.
>>
>> On 20.07.2017 17:59, Sakari Ailus wrote:
>>> Hi Todor,
>>>
>>> On Mon, Jul 17, 2017 at 01:33:36PM +0300, Todor Tomov wrote:
 These files control the VFE module. The VFE has different input interfaces.
 The PIX input interface feeds the input data to an image processing 
 pipeline.
 Three RDI input interfaces bypass the image processing pipeline. The VFE 
 also
 contains the AXI bus interface which writes the output data to memory.

 RDI interfaces are supported in this version. PIX interface is not 
 supported.

 Signed-off-by: Todor Tomov 
 ---
  drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 1913 
 
  drivers/media/platform/qcom/camss-8x16/camss-vfe.h |  114 ++
  2 files changed, 2027 insertions(+)
  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.c
  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.h

 diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c 
 b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
 new file mode 100644
 index 000..b6dd29b
 --- /dev/null
 +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
 @@ -0,0 +1,1913 @@
>>
>> 
>>
 +
 +static void vfe_set_qos(struct vfe_device *vfe)
 +{
 +  u32 val = 0xaaa5aaa5;
 +  u32 val7 = 0x0001aaa5;
>>>
>>> Huh. What do these mean? :-)
>>
>> For these here I don't have understanding of the values. I'll remove the 
>> magic
>> values here and on all the other places but these here I'll just move to a 
>> #define.
> 
> If there is no documentation then I guess that's all that can be done.
> Works for me.
> 
>>
>>>
 +
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_0);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_1);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_2);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_3);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_4);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_5);
 +  writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_6);
 +  writel_relaxed(val7, vfe->base + VFE_0_BUS_BDG_QOS_CFG_7);
 +}
 +
>>
>> 
>>
 +
 +/*
 + * msm_vfe_subdev_init - Initialize VFE device structure and resources
 + * @vfe: VFE device
 + * @res: VFE module resources table
 + *
 + * Return 0 on success or a negative error code otherwise
 + */
 +int msm_vfe_subdev_init(struct vfe_device *vfe, const struct resources 
 *res)
 +{
 +  struct device *dev = to_device(vfe);
 +  struct platform_device *pdev = to_platform_device(dev);
 +  struct resource *r;
 +  struct camss *camss = to_camss(vfe);
 +
 +  int i;
 +  int ret;
 +
 +  mutex_init(>power_lock);
 +  vfe->power_count = 0;
 +
 +  mutex_init(>stream_lock);
 +  vfe->stream_count = 0;
 +
 +  spin_lock_init(>output_lock);
 +
 +  vfe->id = 0;
 +  vfe->reg_update = 0;
 +
 +  for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++) {
 +  vfe->line[i].video_out.type =
 +  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 +  vfe->line[i].video_out.camss = camss;
 +  vfe->line[i].id = i;
 +  }
 +
 +  /* Memory */
 +
 +  r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]);
 +  vfe->base = devm_ioremap_resource(dev, r);
 +  if (IS_ERR(vfe->base)) {
 +  dev_err(dev, "could not map memory\n");
>>>
>>> mutex_destroy() for bothof the mutexes. The same below.
>>>
>>> Do you have a corresponding cleanup function?
>>
>> msm_vfe_subdev_init() and msm_vfe_register_entities() are called on probe().
>> msm_vfe_unregister_entities() is called on remove() - this is the cleanup
>> function. The mutexes are destroyed there. Is there something else that you
>> are concerned about?
> 
> What about the error case above then? Where are the mutexes destroyed?
> 

Right, I'll add mutex_destroy() for the error cases too.


-- 
Best regards,
Todor Tomov

Re: [PATCH v3 10/23] media: camss: Add VFE files

2017-08-05 Thread Sakari Ailus

Hi Todor,

Todor Tomov wrote:
> Hi Sakari,
> 
> Thank you for the review.
> 
> On 20.07.2017 17:59, Sakari Ailus wrote:
>> Hi Todor,
>>
>> On Mon, Jul 17, 2017 at 01:33:36PM +0300, Todor Tomov wrote:
>>> These files control the VFE module. The VFE has different input interfaces.
>>> The PIX input interface feeds the input data to an image processing 
>>> pipeline.
>>> Three RDI input interfaces bypass the image processing pipeline. The VFE 
>>> also
>>> contains the AXI bus interface which writes the output data to memory.
>>>
>>> RDI interfaces are supported in this version. PIX interface is not 
>>> supported.
>>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>  drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 1913 
>>> 
>>>  drivers/media/platform/qcom/camss-8x16/camss-vfe.h |  114 ++
>>>  2 files changed, 2027 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c 
>>> b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>>> new file mode 100644
>>> index 000..b6dd29b
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>>> @@ -0,0 +1,1913 @@
> 
> 
> 
>>> +
>>> +static void vfe_set_qos(struct vfe_device *vfe)
>>> +{
>>> +   u32 val = 0xaaa5aaa5;
>>> +   u32 val7 = 0x0001aaa5;
>>
>> Huh. What do these mean? :-)
> 
> For these here I don't have understanding of the values. I'll remove the magic
> values here and on all the other places but these here I'll just move to a 
> #define.

If there is no documentation then I guess that's all that can be done.
Works for me.

> 
>>
>>> +
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_0);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_1);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_2);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_3);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_4);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_5);
>>> +   writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_6);
>>> +   writel_relaxed(val7, vfe->base + VFE_0_BUS_BDG_QOS_CFG_7);
>>> +}
>>> +
> 
> 
> 
>>> +
>>> +/*
>>> + * msm_vfe_subdev_init - Initialize VFE device structure and resources
>>> + * @vfe: VFE device
>>> + * @res: VFE module resources table
>>> + *
>>> + * Return 0 on success or a negative error code otherwise
>>> + */
>>> +int msm_vfe_subdev_init(struct vfe_device *vfe, const struct resources 
>>> *res)
>>> +{
>>> +   struct device *dev = to_device(vfe);
>>> +   struct platform_device *pdev = to_platform_device(dev);
>>> +   struct resource *r;
>>> +   struct camss *camss = to_camss(vfe);
>>> +
>>> +   int i;
>>> +   int ret;
>>> +
>>> +   mutex_init(>power_lock);
>>> +   vfe->power_count = 0;
>>> +
>>> +   mutex_init(>stream_lock);
>>> +   vfe->stream_count = 0;
>>> +
>>> +   spin_lock_init(>output_lock);
>>> +
>>> +   vfe->id = 0;
>>> +   vfe->reg_update = 0;
>>> +
>>> +   for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++) {
>>> +   vfe->line[i].video_out.type =
>>> +   V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>>> +   vfe->line[i].video_out.camss = camss;
>>> +   vfe->line[i].id = i;
>>> +   }
>>> +
>>> +   /* Memory */
>>> +
>>> +   r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]);
>>> +   vfe->base = devm_ioremap_resource(dev, r);
>>> +   if (IS_ERR(vfe->base)) {
>>> +   dev_err(dev, "could not map memory\n");
>>
>> mutex_destroy() for bothof the mutexes. The same below.
>>
>> Do you have a corresponding cleanup function?
> 
> msm_vfe_subdev_init() and msm_vfe_register_entities() are called on probe().
> msm_vfe_unregister_entities() is called on remove() - this is the cleanup
> function. The mutexes are destroyed there. Is there something else that you
> are concerned about?

What about the error case above then? Where are the mutexes destroyed?

-- 
Sakari Ailus
sakari.ai...@iki.fi

Re: [PATCH v3 10/23] media: camss: Add VFE files

2017-07-25 Thread Todor Tomov

Hi Sakari,

Thank you for the review.

On 20.07.2017 17:59, Sakari Ailus wrote:
> Hi Todor,
> 
> On Mon, Jul 17, 2017 at 01:33:36PM +0300, Todor Tomov wrote:
>> These files control the VFE module. The VFE has different input interfaces.
>> The PIX input interface feeds the input data to an image processing pipeline.
>> Three RDI input interfaces bypass the image processing pipeline. The VFE also
>> contains the AXI bus interface which writes the output data to memory.
>>
>> RDI interfaces are supported in this version. PIX interface is not supported.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 1913 
>> 
>>  drivers/media/platform/qcom/camss-8x16/camss-vfe.h |  114 ++
>>  2 files changed, 2027 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c 
>> b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>> new file mode 100644
>> index 000..b6dd29b
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>> @@ -0,0 +1,1913 @@



>> +
>> +static void vfe_set_qos(struct vfe_device *vfe)
>> +{
>> +u32 val = 0xaaa5aaa5;
>> +u32 val7 = 0x0001aaa5;
> 
> Huh. What do these mean? :-)

For these here I don't have understanding of the values. I'll remove the magic
values here and on all the other places but these here I'll just move to a 
#define.

> 
>> +
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_0);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_1);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_2);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_3);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_4);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_5);
>> +writel_relaxed(val, vfe->base + VFE_0_BUS_BDG_QOS_CFG_6);
>> +writel_relaxed(val7, vfe->base + VFE_0_BUS_BDG_QOS_CFG_7);
>> +}
>> +



>> +
>> +/*
>> + * msm_vfe_subdev_init - Initialize VFE device structure and resources
>> + * @vfe: VFE device
>> + * @res: VFE module resources table
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +int msm_vfe_subdev_init(struct vfe_device *vfe, const struct resources *res)
>> +{
>> +struct device *dev = to_device(vfe);
>> +struct platform_device *pdev = to_platform_device(dev);
>> +struct resource *r;
>> +struct camss *camss = to_camss(vfe);
>> +
>> +int i;
>> +int ret;
>> +
>> +mutex_init(>power_lock);
>> +vfe->power_count = 0;
>> +
>> +mutex_init(>stream_lock);
>> +vfe->stream_count = 0;
>> +
>> +spin_lock_init(>output_lock);
>> +
>> +vfe->id = 0;
>> +vfe->reg_update = 0;
>> +
>> +for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++) {
>> +vfe->line[i].video_out.type =
>> +V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>> +vfe->line[i].video_out.camss = camss;
>> +vfe->line[i].id = i;
>> +}
>> +
>> +/* Memory */
>> +
>> +r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]);
>> +vfe->base = devm_ioremap_resource(dev, r);
>> +if (IS_ERR(vfe->base)) {
>> +dev_err(dev, "could not map memory\n");
> 
> mutex_destroy() for bothof the mutexes. The same below.
> 
> Do you have a corresponding cleanup function?

msm_vfe_subdev_init() and msm_vfe_register_entities() are called on probe().
msm_vfe_unregister_entities() is called on remove() - this is the cleanup
function. The mutexes are destroyed there. Is there something else that you
are concerned about?

> 
>> +return PTR_ERR(vfe->base);
>> +}
>> +
>> +/* Interrupt */
>> +
>> +r = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
>> + res->interrupt[0]);
>> +if (!r) {
>> +dev_err(dev, "missing IRQ\n");
>> +return -EINVAL;
>> +}
>> +
>> +vfe->irq = r->start;
>> +snprintf(vfe->irq_name, sizeof(vfe->irq_name), "%s_%s%d",
>> + dev_name(dev), MSM_VFE_NAME, vfe->id);
>> +ret = devm_request_irq(dev, vfe->irq, vfe_isr,
>> +   IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
>> +if (ret < 0) {
>> +dev_err(dev, "request_irq failed: %d\n", ret);
>> +return ret;
>> +}
>> +
>> +/* Clocks */
>> +
>> +vfe->nclocks = 0;
>> +while (res->clock[vfe->nclocks])
>> +vfe->nclocks++;
>> +
>> +vfe->clock = devm_kzalloc(dev, vfe->nclocks * sizeof(*vfe->clock),
>> +  GFP_KERNEL);
>> +if (!vfe->clock)
>> +return -ENOMEM;
>> +
>> +for (i = 0; i < vfe->nclocks; i++) {
>> +vfe->clock[i] = devm_clk_get(dev, res->clock[i]);
>> +if (IS_ERR(vfe->clock[i]))
>> +

Re: [PATCH v3 10/23] media: camss: Add VFE files

2017-07-20 Thread Sakari Ailus

Hi Todor,

On Mon, Jul 17, 2017 at 01:33:36PM +0300, Todor Tomov wrote:
> These files control the VFE module. The VFE has different input interfaces.
> The PIX input interface feeds the input data to an image processing pipeline.
> Three RDI input interfaces bypass the image processing pipeline. The VFE also
> contains the AXI bus interface which writes the output data to memory.
> 
> RDI interfaces are supported in this version. PIX interface is not supported.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/platform/qcom/camss-8x16/camss-vfe.c | 1913 
> 
>  drivers/media/platform/qcom/camss-8x16/camss-vfe.h |  114 ++
>  2 files changed, 2027 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.c
>  create mode 100644 drivers/media/platform/qcom/camss-8x16/camss-vfe.h
> 
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss-vfe.c 
> b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> new file mode 100644
> index 000..b6dd29b
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.c
> @@ -0,0 +1,1913 @@
> +/*
> + * camss-vfe.c
> + *
> + * Qualcomm MSM Camera Subsystem - VFE Module
> + *
> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2015-2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only 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.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "camss-vfe.h"
> +#include "camss.h"
> +
> +#define MSM_VFE_NAME "msm_vfe"
> +
> +#define vfe_line_array(ptr_line) \
> + ((const struct vfe_line (*)[]) &(ptr_line[-(ptr_line->id)]))

If you have something like that used by multiple parts of the driver, it'd
be good to define it in a common header. (I ignore if the types are exactly
the same though. Ignore the comment if they're different.)

> +
> +#define to_vfe(ptr_line) \
> + container_of(vfe_line_array(ptr_line), struct vfe_device, ptr_line)
> +
> +#define VFE_0_HW_VERSION 0x000
> +
> +#define VFE_0_GLOBAL_RESET_CMD   0x00c
> +#define VFE_0_GLOBAL_RESET_CMD_CORE  (1 << 0)
> +#define VFE_0_GLOBAL_RESET_CMD_CAMIF (1 << 1)
> +#define VFE_0_GLOBAL_RESET_CMD_BUS   (1 << 2)
> +#define VFE_0_GLOBAL_RESET_CMD_BUS_BDG   (1 << 3)
> +#define VFE_0_GLOBAL_RESET_CMD_REGISTER  (1 << 4)
> +#define VFE_0_GLOBAL_RESET_CMD_TIMER (1 << 5)
> +#define VFE_0_GLOBAL_RESET_CMD_PM(1 << 6)
> +#define VFE_0_GLOBAL_RESET_CMD_BUS_MISR  (1 << 7)
> +#define VFE_0_GLOBAL_RESET_CMD_TESTGEN   (1 << 8)
> +
> +#define VFE_0_IRQ_CMD0x024
> +#define VFE_0_IRQ_CMD_GLOBAL_CLEAR   (1 << 0)
> +
> +#define VFE_0_IRQ_MASK_0 0x028
> +#define VFE_0_IRQ_MASK_0_RDIn_REG_UPDATE(n)  (1 << ((n) + 5))
> +#define VFE_0_IRQ_MASK_0_IMAGE_MASTER_n_PING_PONG(n) (1 << ((n) + 8))
> +#define VFE_0_IRQ_MASK_0_RESET_ACK   (1 << 31)
> +#define VFE_0_IRQ_MASK_1 0x02c
> +#define VFE_0_IRQ_MASK_1_VIOLATION   (1 << 7)
> +#define VFE_0_IRQ_MASK_1_BUS_BDG_HALT_ACK(1 << 8)
> +#define VFE_0_IRQ_MASK_1_IMAGE_MASTER_n_BUS_OVERFLOW(n)  (1 << ((n) + 9))
> +
> +#define VFE_0_IRQ_CLEAR_00x030
> +#define VFE_0_IRQ_CLEAR_10x034
> +
> +#define VFE_0_IRQ_STATUS_0   0x038
> +#define VFE_0_IRQ_STATUS_0_RDIn_REG_UPDATE(n)(1 << ((n) + 5))
> +#define VFE_0_IRQ_STATUS_0_IMAGE_MASTER_n_PING_PONG(n)   (1 << ((n) + 8))
> +#define VFE_0_IRQ_STATUS_0_RESET_ACK (1 << 31)
> +#define VFE_0_IRQ_STATUS_1   0x03c
> +#define VFE_0_IRQ_STATUS_1_VIOLATION (1 << 7)
> +#define VFE_0_IRQ_STATUS_1_BUS_BDG_HALT_ACK  (1 << 8)
> +
> +#define VFE_0_VIOLATION_STATUS   0x48
> +
> +#define VFE_0_BUS_CMD0x4c
> +#define VFE_0_BUS_CMD_Mx_RLD_CMD(x)  (1 << (x))
> +
> +#define VFE_0_BUS_CFG0x050
> +
> +#define VFE_0_BUS_XBAR_CFG_x(x)  (0x58 + 0x4 * ((x) / 2))
> +#define VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_SHIFT   8
> +#define VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_VAL_RDI05
> +#define VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_VAL_RDI16
> +#define VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_VAL_RDI27
> +
> +#define VFE_0_BUS_IMAGE_MASTER_n_WR_CFG(n)   (0x06c + 0x24 * (n))
> +#define VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_WR_PATH_SHIFT0
> +#define

Re: [PATCH v3 10/23] media: camss: Add VFE files

Re: [PATCH v3 10/23] media: camss: Add VFE files

Re: [PATCH v3 10/23] media: camss: Add VFE files

Re: [PATCH v3 10/23] media: camss: Add VFE files

4 matches

Site Navigation

Mail list logo

Footer information