On 27/07/2023 13:25, Devarsh Thakkar wrote:
> This adds support for stateful V4L2 M2M based driver
> for Imagination E5010 JPEG Encoder [1] which supports baseline
> encoding with two different quantization tables and compression
> ratio as demanded.
>
> Support for both contigous and non-contigous YUV420 and YUV422
> semiplanar formats is added along with alignment restrictions
> as required by the hardware.
>
> System and runtime PM hooks are added in the driver along with v4l2
> crop and selection API support.
>
> Minimum resolution supported is 64x64 and
> Maximum resolution supported is 8192x8192.
>
...
> +
> +static int e5010_release(struct file *file)
> +{
> + struct e5010_dev *dev = video_drvdata(file);
> + struct e5010_context *ctx = file->private_data;
> +
> + dprintk(dev, 1, "Releasing instance: 0x%p, m2m_ctx: 0x%p\n", ctx, ctx->fh.m2m_ctx);
Why do you print pointers? Looks like code is buggy and you still keep
debugging it.
> + mutex_lock(&dev->mutex);
> + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> + v4l2_fh_del(&ctx->fh);
> + v4l2_fh_exit(&ctx->fh);
> + kfree(ctx);
> + mutex_unlock(&dev->mutex);
> +
> + return 0;
> +}
> +
...
> +static int e5010_init_device(struct e5010_dev *dev)
> +{
> + int ret = 0;
> +
> + /*TODO: Set MMU in bypass mode until support for the same is added in driver*/
> + e5010_hw_bypass_mmu(dev->mmu_base, 1);
> +
> + if (e5010_hw_enable_auto_clock_gating(dev->jasper_base, 1))
> + dev_warn(dev->dev, "Failed to enable auto clock gating\n");
> +
> + if (e5010_hw_enable_manual_clock_gating(dev->jasper_base, 0))
> + dev_warn(dev->dev, "Failed to disable manual clock gating\n");
> +
> + if (e5010_hw_enable_crc_check(dev->jasper_base, 0))
> + dev_warn(dev->dev, "Failed to disable CRC check\n");
> +
> + if (e5010_hw_enable_output_address_error_irq(dev->jasper_base, 1))
> + dev_err(dev->dev, "Failed to enable Output Address Error interrupts\n");
> +
> + ret = e5010_hw_set_input_source_to_memory(dev->jasper_base, 1);
> + if (ret) {
> + dev_err(dev->dev, "Failed to set input source to memory\n");
> + goto fail;
retturn ret;
> + }
> +
> + ret = e5010_hw_enable_picture_done_irq(dev->jasper_base, 1);
> + if (ret)
> + dev_err(dev->dev, "Failed to enable Picture Done interrupts\n");
> +fail:
> + return ret;
> +}
> +
> +static int e5010_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *of_dev_id;
> + struct e5010_dev *dev;
dev is struct device, so call it differently.
> + struct resource *res;
> + int irq, ret = 0;
> +
> + of_dev_id = of_match_device(e5010_of_match, &pdev->dev);
> + if (!of_dev_id) {
> + dev_err(&pdev->dev, "%s: Unable to match device\n", __func__);
I don't think this can happen.
> + return -ENODEV;
> + }
> +
> + ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(&pdev->dev, "32-bit consistent DMA enable failed\n");
> + return ret;
> + }
> +
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, dev);
> +
> + dev->dev = &pdev->dev;
> +
> + mutex_init(&dev->mutex);
> + spin_lock_init(&dev->hw_lock);
> +
> + dev->vdev = &e5010_videodev;
> + dev->vdev->v4l2_dev = &dev->v4l2_dev;
> + dev->vdev->lock = &dev->mutex;
> + dev->vdev->queue = NULL;
> + dev->vdev->prio = NULL;
> + dev->vdev->dev_parent = NULL;
> + dev->vdev->minor = -1;
> +
> + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> + if (ret) {
> + dev_err(dev->dev, "Failed to register v4l2 device\n");
> + return ret;
return dev_err_probe
> + }
> +
> + dev->m2m_dev = v4l2_m2m_init(&e5010_m2m_ops);
> + if (!dev->m2m_dev) {
This cannot happen. Read the function.
> + dev_err(dev->dev, "Failed to initialize m2m device\n");
> + ret = -ENOMEM;
> + goto fail_after_v4l2_register;
> + }
> +
> + video_set_drvdata(dev->vdev, dev);
> +
> + ret = video_register_device(dev->vdev, VFL_TYPE_VIDEO, 0);
> + if (ret) {
> + dev_err(dev->dev, "Failed to register video device\n");
> + ret = -ENOMEM;
Why?
> + goto fail_after_v4l2_register;
> + }
> +
> + dev_info(dev->dev, "Device registered as /dev/video%d\n",
> + dev->vdev->num);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regjasper");
> + if (!res) {
> + dev_err(dev->dev, "Missing 'regjasper' resources area\n");
> + ret = -ENOMEM;
> + goto fail_after_video_register_device;
> + }
> + dev->jasper_base = devm_ioremap_resource(&pdev->dev, res);
Use helper function to combine two calls into one.
> + if (!dev->jasper_base) {
> + ret = -ENOMEM;
This shouldn't be ENOMEM
> + goto fail_after_video_register_device;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regmmu");
> + if (!res) {
> + dev_err(dev->dev, "Missing 'regmmu' resources area\n");
> + ret = -ENOMEM;
> + goto fail_after_video_register_device;
> + }
> + dev->mmu_base = devm_ioremap_resource(&pdev->dev, res);
> + if (!dev->mmu_base) {
> + ret = -ENOMEM;
The same.
> + goto fail_after_video_register_device;
> + }
> +
> + dev->last_context_run = NULL;
> +
> + irq = platform_get_irq(pdev, 0);
> + ret = devm_request_irq(dev->dev, irq, e5010_irq, 0,
> + E5010_MODULE_NAME, dev);
> + if (ret) {
> + dev_err(dev->dev, "Failed to register IRQ %d\n", irq);
> + goto fail_after_video_register_device;
> + }
> +
> + dev->clk = devm_clk_get(&pdev->dev, "core_clk");
> + if (IS_ERR(dev->clk)) {
> + dev_err(dev->dev, "failed to get clock\n");
> + ret = PTR_ERR(dev->clk);
ret = dev_err_probe
> + goto fail_after_video_register_device;
> + }
> +
> + pm_runtime_enable(dev->dev);
> +
> + return 0;
> +
> +fail_after_video_register_device:
> + v4l2_m2m_release(dev->m2m_dev);
> +fail_after_v4l2_register:
> + v4l2_device_unregister(&dev->v4l2_dev);
> + return ret;
> +}
> +
> +static int e5010_remove(struct platform_device *pdev)
> +{
> + struct e5010_dev *dev = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(dev->dev);
> + video_unregister_device(dev->vdev);
> + v4l2_m2m_release(dev->m2m_dev);
> + v4l2_device_unregister(&dev->v4l2_dev);
> +
> + return 0;
> +}
...
> +#define MAX_PLANES 2
> +#define HEADER_SIZE 0x025E
> +#define MIN_DIMENSION 64
> +#define MAX_DIMENSION 8192
> +#define DEFAULT_WIDTH 640
> +#define DEFAULT_HEIGHT 480
> +#define E5010_MODULE_NAME "e5010"
> +
> +/* JPEG marker definitions */
> +#define START_OF_IMAGE 0xFFD8
> +#define SOF_BASELINE_DCT 0xFFC0
> +#define END_OF_IMAGE 0xFFD9
> +#define START_OF_SCAN 0xFFDA
> +
> +/* Definitions for the huffman table specification in the Marker segment */
> +#define DHT_MARKER 0xFFC4
> +#define LH_DC 0x001F
> +#define LH_AC 0x00B5
> +
> +/* Definitions for the quantization table specification in the Marker segment */
> +#define DQT_MARKER 0xFFDB
> +#define ACMAX 0x03FF
> +#define DCMAX 0x07FF
> +
> +/* Length and precision of the quantization table parameters */
> +#define LQPQ 0x00430
> +#define QMAX 255
> +
> +/* Misc JPEG header definitions */
> +#define UC_NUM_COMP 3
> +#define PRECISION 8
> +#define HORZ_SAMPLING_FACTOR (2 << 4)
> +#define VERT_SAMPLING_FACTOR_422 1
> +#define VERT_SAMPLING_FACTOR_420 2
> +#define COMPONENTS_IN_SCAN 3
> +#define PELS_IN_BLOCK 64
> +
> +/* Used for Qp table generation */
> +#define LUMINOSITY 10
> +#define CONTRAST 1
> +#define INCREASE 2
> +#define QP_TABLE_SIZE (8 * 8)
> +#define QP_TABLE_FIELD_OFFSET 0x04
> +
> +/*
> + * vb2 queue structure
> + * contains queue data information
> + *
> + * @fmt: format info
> + * @width: frame width
> + * @height: frame height
> + * @bytesperline: bytes per line in memory
> + * @size_image: image size in memory
> + */
> +struct e5010_q_data {
> + struct e5010_fmt *fmt;
> + u32 width;
> + u32 height;
> + u32 width_adjusted;
> + u32 height_adjusted;
> + u32 sizeimage[MAX_PLANES];
> + u32 bytesperline[MAX_PLANES];
> + bool format_set;
> + bool streaming;
> + u32 sequence;
> + struct v4l2_rect crop;
Unexpected indentation.
> +};
> +
> +/*
> + * Driver device structure
> + * Holds all memory handles and global parameters
> + * Shared by all instances
> + */
> +struct e5010_dev {
> + struct device *dev;
> + struct v4l2_device v4l2_dev;
> + struct v4l2_m2m_dev *m2m_dev;
> + struct video_device *vdev;
> + void __iomem *jasper_base;
> + void __iomem *mmu_base;
> + struct clk *clk;
Please keep style consistent.
> + struct e5010_context *last_context_run;
> + /* Protect access to device data */
> + struct mutex mutex;
> + /* Protect access to hardware*/
> + spinlock_t hw_lock;
> +};
> +
Best regards,
Krzysztof
Hi Krzysztof,
Le jeudi 27 juillet 2023 à 14:13 +0200, Krzysztof Kozlowski a écrit :
> On 27/07/2023 13:25, Devarsh Thakkar wrote:
> ...
>
> > +
> > +static int e5010_release(struct file *file)
> > +{
> > + struct e5010_dev *dev = video_drvdata(file);
> > + struct e5010_context *ctx = file->private_data;
> > +
> > + dprintk(dev, 1, "Releasing instance: 0x%p, m2m_ctx: 0x%p\n", ctx, ctx->fh.m2m_ctx);
>
> Why do you print pointers? Looks like code is buggy and you still keep
> debugging it.
Its relatively common practice in linux-media to leave a certain level of traces
to help future debugging if a bug is seen. These uses v4l2 debug helper, and are
only going to print if users enable them through the associated sysfs
configuration. I do hope though there isn't any issue with IRQ triggering after
the instance is released, that would be buggy for sure, but I don't think this
is the case considering the level of documented testing that have been done.
I'd be happy to see what others have to say on the subject, as its been a
recurrent subject of confrontation lately. With pretty agressive messages
associated with that.
regards,
Nicolas
p.s. does not invalidate the question, since for this driver, there is only ever
going to be one m2m_ctx, so the question "Why do you print pointers?" is
entirely valid I believe.
. . .
Hi Krzysztof, Nicholas,
Thanks for the quick review.
On 27/07/23 19:49, Nicolas Dufresne wrote:
> Hi Krzysztof,
>
> Le jeudi 27 juillet 2023 à 14:13 +0200, Krzysztof Kozlowski a écrit :
>> On 27/07/2023 13:25, Devarsh Thakkar wrote:
>> ...
>>
>>> +
>>> +static int e5010_release(struct file *file)
>>> +{
>>> + struct e5010_dev *dev = video_drvdata(file);
>>> + struct e5010_context *ctx = file->private_data;
>>> +
>>> + dprintk(dev, 1, "Releasing instance: 0x%p, m2m_ctx: 0x%p\n", ctx, ctx->fh.m2m_ctx);
>>
>> Why do you print pointers? Looks like code is buggy and you still keep
>> debugging it.
>
> Its relatively common practice in linux-media to leave a certain level of traces
> to help future debugging if a bug is seen. These uses v4l2 debug helper, and are
> only going to print if users enable them through the associated sysfs
> configuration. I do hope though there isn't any issue with IRQ triggering after
> the instance is released, that would be buggy for sure, but I don't think this
> is the case considering the level of documented testing that have been done.
>
> I'd be happy to see what others have to say on the subject, as its been a
> recurrent subject of confrontation lately. With pretty agressive messages
> associated with that.
>
> regards,
> Nicolas
>
> p.s. does not invalidate the question, since for this driver, there is only ever
> going to be one m2m_ctx, so the question "Why do you print pointers?" is
> entirely valid I believe.
>
There is a possible scenario with multiple applications accessing the device
node simultaneously (and so multiple m2m_ctx are possible as seen in below
logs) and these prints were helpful to debug/analyze these scenarios.
[181955.443632] e5010 fd20000.e5010: e5010_open: Created instance:
0x00000000bea83b70, m2m_ctx: 0x00000000d068a951
[181955.449587] e5010 fd20000.e5010: e5010_open: Created instance:
0x0000000046749df9, m2m_ctx: 0x000000000ff56aa6
[181955.450407] e5010 fd20000.e5010: e5010_open: Created instance:
0x00000000e33791b5, m2m_ctx: 0x00000000217634a8
[181955.457067] e5010 fd20000.e5010: e5010_open: Created instance:
0x00000000d77f83fe, m2m_ctx: 0x000000000c8ec99e
Infact, actually I had added these prints while debugging an issue with this
type of multistream scenario, where I was launching like 20 instances of JPEG
encoding and some of the instances were hanging, these prints were helpful to
fix that scenario and I later still kept these prints as they may help in
future in case any issue is encountered while adding a new feature or in
further testing.
I have also already put the logs for this multi-stream scenario in gist shared
in commit message, below is the exact line :
https://gist.github.com/devarsht/ea31179199393c2026ae457219bb6321#file-e5010_jpeg_upstream_manualtests-txt-L89
Regards
Devarsh
> . . .
Hi Krzysztof,
Thanks for the review.
On 27/07/23 17:43, Krzysztof Kozlowski wrote:
> On 27/07/2023 13:25, Devarsh Thakkar wrote:
>> This adds support for stateful V4L2 M2M based driver
>> for Imagination E5010 JPEG Encoder [1] which supports baseline
>> encoding with two different quantization tables and compression
>> ratio as demanded.
>>
>> Support for both contigous and non-contigous YUV420 and YUV422
>> semiplanar formats is added along with alignment restrictions
>> as required by the hardware.
>>
>> System and runtime PM hooks are added in the driver along with v4l2
>> crop and selection API support.
>>
>> Minimum resolution supported is 64x64 and
>> Maximum resolution supported is 8192x8192.
>>
>
>
> ...
>
>> +
>> +static int e5010_release(struct file *file)
>> +{
>> + struct e5010_dev *dev = video_drvdata(file);
>> + struct e5010_context *ctx = file->private_data;
>> +
>> + dprintk(dev, 1, "Releasing instance: 0x%p, m2m_ctx: 0x%p\n", ctx, ctx->fh.m2m_ctx);
>
> Why do you print pointers? Looks like code is buggy and you still keep
> debugging it.
>
This are dynamic debug prints to debug/analyze v4l2 drivers, It's a general
mechanism used in v4l2 drivers for debugging as Nicholas mentioned.
>> + mutex_lock(&dev->mutex);
>> + v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>> + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>> + v4l2_fh_del(&ctx->fh);
>> + v4l2_fh_exit(&ctx->fh);
>> + kfree(ctx);
>> + mutex_unlock(&dev->mutex);
>> +
>> + return 0;
>> +}
>> +
>
> ...
>
>> +static int e5010_init_device(struct e5010_dev *dev)
>> +{
>> + int ret = 0;
>> +
>> + /*TODO: Set MMU in bypass mode until support for the same is added in driver*/
>> + e5010_hw_bypass_mmu(dev->mmu_base, 1);
>> +
>> + if (e5010_hw_enable_auto_clock_gating(dev->jasper_base, 1))
>> + dev_warn(dev->dev, "Failed to enable auto clock gating\n");
>> +
>> + if (e5010_hw_enable_manual_clock_gating(dev->jasper_base, 0))
>> + dev_warn(dev->dev, "Failed to disable manual clock gating\n");
>> +
>> + if (e5010_hw_enable_crc_check(dev->jasper_base, 0))
>> + dev_warn(dev->dev, "Failed to disable CRC check\n");
>> +
>> + if (e5010_hw_enable_output_address_error_irq(dev->jasper_base, 1))
>> + dev_err(dev->dev, "Failed to enable Output Address Error interrupts\n");
>> +
>> + ret = e5010_hw_set_input_source_to_memory(dev->jasper_base, 1);
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to set input source to memory\n");
>> + goto fail;
>
> retturn ret;
>
>> + }
>> +
>> + ret = e5010_hw_enable_picture_done_irq(dev->jasper_base, 1);
>> + if (ret)
>> + dev_err(dev->dev, "Failed to enable Picture Done interrupts\n");
>> +fail:
>> + return ret;
>> +}
>> +
>> +static int e5010_probe(struct platform_device *pdev)
>> +{
>> + const struct of_device_id *of_dev_id;
>> + struct e5010_dev *dev;
>
> dev is struct device, so call it differently.
>
Agreed, will change the name.
>> + struct resource *res;
>> + int irq, ret = 0;
>> +
>> + of_dev_id = of_match_device(e5010_of_match, &pdev->dev);
>> + if (!of_dev_id) {
>> + dev_err(&pdev->dev, "%s: Unable to match device\n", __func__);
>
> I don't think this can happen.
Ok, will remove of_match_device altogether as not needed.
>
>> + return -ENODEV;
>> + }
>> +
>> + ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_err(&pdev->dev, "32-bit consistent DMA enable failed\n");
>> + return ret;
>> + }
>> +
>> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> + if (!dev)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, dev);
>> +
>> + dev->dev = &pdev->dev;
>> +
>> + mutex_init(&dev->mutex);
>> + spin_lock_init(&dev->hw_lock);
>> +
>> + dev->vdev = &e5010_videodev;
>> + dev->vdev->v4l2_dev = &dev->v4l2_dev;
>> + dev->vdev->lock = &dev->mutex;
>> + dev->vdev->queue = NULL;
>> + dev->vdev->prio = NULL;
>> + dev->vdev->dev_parent = NULL;
>> + dev->vdev->minor = -1;
>> +
>> + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to register v4l2 device\n");
>> + return ret;
>
> return dev_err_probe
>
Ok, will check on this.
>> + }
>> +
>> + dev->m2m_dev = v4l2_m2m_init(&e5010_m2m_ops);
>> + if (!dev->m2m_dev) {
>
> This cannot happen. Read the function.
Agreed will use IS_ERR here.
>
>> + dev_err(dev->dev, "Failed to initialize m2m device\n");
>> + ret = -ENOMEM;
>> + goto fail_after_v4l2_register;
>> + }
>> +
>> + video_set_drvdata(dev->vdev, dev);
>> +
>> + ret = video_register_device(dev->vdev, VFL_TYPE_VIDEO, 0);
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to register video device\n");
>> + ret = -ENOMEM;
>
> Why?
>
Will propagate the return value instead.
>> + goto fail_after_v4l2_register;
>> + }
>> +
>> + dev_info(dev->dev, "Device registered as /dev/video%d\n",
>> + dev->vdev->num);
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regjasper");
>> + if (!res) {
>> + dev_err(dev->dev, "Missing 'regjasper' resources area\n");
>> + ret = -ENOMEM;
>> + goto fail_after_video_register_device;
>> + }
>> + dev->jasper_base = devm_ioremap_resource(&pdev->dev, res);
>
> Use helper function to combine two calls into one.
Ok will try using devm_platform_ioremap_resource_byname().
>
>> + if (!dev->jasper_base) {
>> + ret = -ENOMEM;
>
> This shouldn't be ENOMEM
>
Ok, will return the error.
>> + goto fail_after_video_register_device;
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regmmu");
>> + if (!res) {
>> + dev_err(dev->dev, "Missing 'regmmu' resources area\n");
>> + ret = -ENOMEM;
>> + goto fail_after_video_register_device;
>> + }
>> + dev->mmu_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (!dev->mmu_base) {
>> + ret = -ENOMEM;
>
> The same.
>
Ok, will return the error.
>> + goto fail_after_video_register_device;
>> + }
>> +
>> + dev->last_context_run = NULL;
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + ret = devm_request_irq(dev->dev, irq, e5010_irq, 0,
>> + E5010_MODULE_NAME, dev);
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to register IRQ %d\n", irq);
>> + goto fail_after_video_register_device;
>> + }
>> +
>> + dev->clk = devm_clk_get(&pdev->dev, "core_clk");
>> + if (IS_ERR(dev->clk)) {
>> + dev_err(dev->dev, "failed to get clock\n");
>> + ret = PTR_ERR(dev->clk);
>
> ret = dev_err_probe
>
Ok, will use above helper.
>> + goto fail_after_video_register_device;
>> + }
>> +
>> + pm_runtime_enable(dev->dev);
>> +
>> + return 0;
>> +
>> +fail_after_video_register_device:
>> + v4l2_m2m_release(dev->m2m_dev);
>> +fail_after_v4l2_register:
>> + v4l2_device_unregister(&dev->v4l2_dev);
>> + return ret;
>> +}
>> +
>> +static int e5010_remove(struct platform_device *pdev)
>> +{
>> + struct e5010_dev *dev = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_disable(dev->dev);
>> + video_unregister_device(dev->vdev);
>> + v4l2_m2m_release(dev->m2m_dev);
>> + v4l2_device_unregister(&dev->v4l2_dev);
>> +
>> + return 0;
>> +}
>
> ...
>
>> +#define MAX_PLANES 2
>> +#define HEADER_SIZE 0x025E
>> +#define MIN_DIMENSION 64
>> +#define MAX_DIMENSION 8192
>> +#define DEFAULT_WIDTH 640
>> +#define DEFAULT_HEIGHT 480
>> +#define E5010_MODULE_NAME "e5010"
>> +
>> +/* JPEG marker definitions */
>> +#define START_OF_IMAGE 0xFFD8
>> +#define SOF_BASELINE_DCT 0xFFC0
>> +#define END_OF_IMAGE 0xFFD9
>> +#define START_OF_SCAN 0xFFDA
>> +
>> +/* Definitions for the huffman table specification in the Marker segment */
>> +#define DHT_MARKER 0xFFC4
>> +#define LH_DC 0x001F
>> +#define LH_AC 0x00B5
>> +
>> +/* Definitions for the quantization table specification in the Marker segment */
>> +#define DQT_MARKER 0xFFDB
>> +#define ACMAX 0x03FF
>> +#define DCMAX 0x07FF
>> +
>> +/* Length and precision of the quantization table parameters */
>> +#define LQPQ 0x00430
>> +#define QMAX 255
>> +
>> +/* Misc JPEG header definitions */
>> +#define UC_NUM_COMP 3
>> +#define PRECISION 8
>> +#define HORZ_SAMPLING_FACTOR (2 << 4)
>> +#define VERT_SAMPLING_FACTOR_422 1
>> +#define VERT_SAMPLING_FACTOR_420 2
>> +#define COMPONENTS_IN_SCAN 3
>> +#define PELS_IN_BLOCK 64
>> +
>> +/* Used for Qp table generation */
>> +#define LUMINOSITY 10
>> +#define CONTRAST 1
>> +#define INCREASE 2
>> +#define QP_TABLE_SIZE (8 * 8)
>> +#define QP_TABLE_FIELD_OFFSET 0x04
>> +
>> +/*
>> + * vb2 queue structure
>> + * contains queue data information
>> + *
>> + * @fmt: format info
>> + * @width: frame width
>> + * @height: frame height
>> + * @bytesperline: bytes per line in memory
>> + * @size_image: image size in memory
>> + */
>> +struct e5010_q_data {
>> + struct e5010_fmt *fmt;
>> + u32 width;
>> + u32 height;
>> + u32 width_adjusted;
>> + u32 height_adjusted;
>> + u32 sizeimage[MAX_PLANES];
>> + u32 bytesperline[MAX_PLANES];
>> + bool format_set;
>> + bool streaming;
>> + u32 sequence;
>> + struct v4l2_rect crop;
>
> Unexpected indentation.
Agreed, will correct.
>
>> +};
>> +
>> +/*
>> + * Driver device structure
>> + * Holds all memory handles and global parameters
>> + * Shared by all instances
>> + */
>> +struct e5010_dev {
>> + struct device *dev;
>> + struct v4l2_device v4l2_dev;
>> + struct v4l2_m2m_dev *m2m_dev;
>> + struct video_device *vdev;
>> + void __iomem *jasper_base;
>> + void __iomem *mmu_base;
>> + struct clk *clk;
>
> Please keep style consistent.
Agreed, will correct.
Best Regards
Devarsh
>
>> + struct e5010_context *last_context_run;
>> + /* Protect access to device data */
>> + struct mutex mutex;
>> + /* Protect access to hardware*/
>> + spinlock_t hw_lock;
>> +};
>> +
>
>
> Best regards,
> Krzysztof
>