Re: [PATCH v6 2/3] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

["Gjorgji Rosikopulos (Consultant)" <gjorgji.rosikopulos@oss.qualcomm.com>]

Hi Dmitry,

On 8/3/2026 9:37 PM, Dmitry Baryshkov wrote:
> On Mon, Aug 03, 2026 at 06:57:09PM +0300, Gjorgji Rosikopulos (Consultant) wrote:
>> Hi Dmitry, 
>>
>> On 7/30/2026 6:57 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jul 30, 2026 at 06:05:23PM +0300, Atanas Filipov wrote:
>>>> Add a Qualcomm JPEG encoder driver implemented on top of the
>>>> V4L2 mem2mem framework.
>>>>
>>>> The driver wires vb2 queue handling, format negotiation, JPEG header
>>>> handling, interrupt-driven job completion, and runtime PM/clock/ICC
>>>> integration for the standalone JPEG encode hardware block.
>>>>
>>
>> <snip>
>>
>>>> + */
>>>> +
>>>> +#define JFIF_HEADER_WIDTH_OFFS		0x07
>>>> +#define JFIF_HEADER_HEIGHT_OFFS		0x05
>>>
>>> And you've ignored feedback here. PLEASE move all standard-related
>>> defines and code to the common helpers. Are there any other drivers
>>> which construct JPEG files manually? If not, you are lucky and you can
>>> just push you code. If they are, find a way to unify the codebase.
>>>
>>> At the very least, it would make you split this commit into at least
>>> two, making them more readable.
>>
>> Yes, this comment was incorporated, maybe partially, or it wasn't fully understood.
>> The helper is used for the quantization tables and wherever helpers are available, 
>> similar to other drivers: hantro_jpeg.c, e5010-jpeg-enc.c.
>>
>> The missing JFIF tags and additional helpers can certainly be added,
>> but is holding up the current driver just for that a reasonable ask? 
>> This discussion may continue for a long time — is it reasonable to wait that long?
> 
> From my point of view, yes. From your comment it feels like each driver
> having their own way of wriing JPEG framings.
> 
>>
>> I agree it would be good to have all other upstream JPEG encoder drivers move to shared helpers,
>> but the effort isn't uniform across them. 
>>
>> Five drivers — hantro_jpeg.c, mxc-jpeg.c, rcar_jpu.c, gspca/jpeg.h, and solo6x10-jpeg.h,
>> build a fixed byte-array template and patch width/height/table values at hardcoded offsets, 
>> so they could plausibly migrate to a shared builder with a similar shape to what we're proposing. 
> 
> Can we start with something as simple as this for our driver too?

Yes i agree we can add helpers, and qcom jpeg to be first driver to use them.

> 
> Then you can converge all these drivers to use those simple helpers
> (this should not require the actual hardware to test), then improve the
> helpers.

I don not fell confident to touch other platform drivers which i can not verify, but i think
that can be done as part of separate patchset after initial helpers are reviewed-merged.

> 
>> The other two, e5010-jpeg-enc.c and coda-jpeg.c, use incremental byte-by-byte writers instead, 
>> so their migration would look quite different and isn't a drop-in fit for the same API.
> 
> Ok, these are more difficult cases.
> 
>>
>> Either way, we don't have access to most of these devices and can't verify the changes ourselves, 
>> so migrating them is not a simple effort to undertake as part of this series.
> 
> Which reads: "we already have 7 different implementations of JPEG
> framing / file format, can we add 8th?" The typical answer would be
> "no".

Yes i agree we will add helpers and be qcom jpeg as first driver uses those.

> 
>>
>>>
>>>> +#define JFIF_APP0_LENGTH_HI		0x00
>>>> +#define JFIF_APP0_LENGTH_LO		0x10
>>
>> <snip>
>>
>>>> +#include "qcom_jenc_dev.h"
>>>> +
>>>> +/*
>>>> + * JENC encoder hardware operations.
>>>> + */
>>>> +struct qcom_jpeg_hw_ops {
>>>> +	void (*hw_get_cap)
>>>> +		(struct qcom_jenc_dev *jenc_dev, u32 *hw_caps);
>>>> +
>>>> +	int (*hw_acquire)
>>>> +		(struct jenc_context *ectx, struct vb2_queue *queue);
>>>> +
>>>> +	int (*hw_release)
>>>> +		(struct jenc_context *ectx, struct vb2_queue *queue);
>>>> +
>>>> +	int (*hw_prepare)
>>>> +		(struct qcom_jenc_dev *jenc);
>>>> +
>>>> +	struct qcom_jenc_queue * (*get_queue)
>>>> +		(struct jenc_context *ectx, enum qcom_enc_qid id);
>>>> +
>>>> +	int (*queue_setup)
>>>> +		(struct jenc_context *ectx, enum qcom_enc_qid id);
>>>> +
>>>> +	int (*src_fmt_update)
>>>> +		(struct jenc_context *ectx, u32 old_fourcc, u32 new_fourcc);
>>>> +
>>>> +	int (*buf_prepare)
>>>> +		(struct jenc_context *ectx, struct vb2_buffer *vb2);
>>>> +
>>>> +	int (*process_exec)
>>>> +		(struct qcom_jenc_dev *jenc, struct jenc_context *ectx, struct vb2_buffer *vb2);
>>>> +
>>>> +	irqreturn_t (*hw_irq_top)(int irq_num, void *data);
>>>> +	irqreturn_t (*hw_irq_bot)(int irq_num, void *data);
>>>
>>> How many non-default platforms do you support? Zero?
>>>
>>> Drop the call table.
>>
>> There is plan to add support for more platforms, if the preference is to remove platform based ops now,
>> and introduce them when new platform is added i am ok with that. But will require more work now and
>> for the new platform...
> 
> Yes. When you add a platform, we (reviewers) can see, what exactly is
> required for that platform. For now, you are adding complexity for no
> added value.

Ok the ops will be dropped in next patchset.

~Gjorgji

> 
>>>> +
>>>> +/*
>>>> + * V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO - enable adaptive performance scaling.
>>>> + *
>>>> + * When set to 1 the driver selects the core clock OPP level based on the
>>>> + * encoded frame resolution and fps target. When set to 0 (default) the
>>>> + * driver always runs at NOMINAL (highest) OPP level.
>>>> + */
>>>> +#define V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO	(V4L2_CID_USER_QCOM_JENC_BASE + 0)
>>>> +
>>>> +/*
>>>> + * V4L2_CID_QCOM_JPEG_FPS_TARGET - target encode rate in frames per second.
>>>> + *
>>>> + * Used together with V4L2_CID_QCOM_JPEG_PERF_LEVEL_AUTO to select the lowest
>>>> + * OPP level whose throughput is sufficient for the requested frame rate.
>>>> + * Has no effect when perf_level_auto is 0. Range: 1-240, default: 30.
>>>
>>> I assume 1-240 is only applicable to your driver.
>>>
>> I think we can drop those controls and use s_param on output(source) video node as it
>> was done for some of the other m2m drivers including OPE. Which make sense we tell the
>> the driver at what rate source buffers will be received, then the driver will choose op
>> level to satisfy that requirement.
> 
> Ok (if you say that there are other m2m drivers doing this).
>