>From: Mirela Rabulea (OSS) <mirela.rabulea@xxxxxxxxxxx>
>Sent: 2023年7月12日 6:59
>To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; Mirela Rabulea
>(OSS) <mirela.rabulea@xxxxxxxxxxx>; hverkuil-cisco@xxxxxxxxx
>Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
>festevam@xxxxxxxxx; X.H. Bao <xiahong.bao@xxxxxxx>; dl-linux-imx <linux-
>imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>Subject: RE: [PATCH 1/2] media: imx-jpeg: Support to assign slot for
>encoder/decoder
>
>Hi Ming,
>
>> -----Original Message-----
>> From: Ming Qian <ming.qian@xxxxxxx>
>> Sent: Tuesday, May 30, 2023 10:17 AM
>> To: mchehab@xxxxxxxxxx; Mirela Rabulea (OSS)
>> <mirela.rabulea@xxxxxxxxxxx>; hverkuil-cisco@xxxxxxxxx
>> Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
>> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; X.H. Bao
>> <xiahong.bao@xxxxxxx>; dl-linux-imx <linux- imx@xxxxxxx>;
>> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Subject: [PATCH 1/2] media: imx-jpeg: Support to assign slot for
>> encoder/decoder
>>
>> imx jpeg encoder and decoder support 4 slots each, aim to support some
>> virtualization scenarios.
>>
>> driver should only enable one slot one time.
>
>I somehow disagree with this, the hardware is capable of doing context
>switching between the 4 slots, unfortunately a feature which we did not get to
>test enough.
>The initial aim for the current slot design in the driver was to allow slot
>assignment per-context (per device node opened file handle), so we could
>have up to 4 opens, each one with its context, to be serviced by the hardware
>in round-robin manner. Since this was limiting the number of opens, and v4l2-
>compliance was failing on the unlimited opens test, I moved the slot acquiring
>to a later point, in device_run, and also limited the maximum slots to 1
>instead of 4, due to issues when running on other slots than the first one.
>
Hi Mirela,
Maybe you're slightly misunderstanding the purpose of the slot. The slot is designed to support virtualization.
That the linux os and some virtualized environment can use the jpeg codec independently. Each os or virtualized environment will use one slot. There is no benefit to using multiple slots in one os, it won't improve performance.
Multiple slots share the codec in time-sharing multiplexing.
So for one os or virtualized environment, only one slot is required, but it doesn't care which slot is configured. The point is different os or virtualized environment need to use different slot.
We need to assign the slot in dts, then driver can be implemented in a generic way.
For imx8qxp/m, there is on difference as there is only one slot can be enabled.
For imx9, we have tested different slot in linux, and we need this patch to work on the virtualization support.
Ming
>>
>> but due to some hardware issue,
>> only slot 0 can be enabled in imx8q platform, and they may be fixed in
>> imx9 platform.
>
>I don't think it's ok to limit the driver to using just one slot, the slot which is
>hardcoded in the dts. I suggest to hold off this patch series until we have a
>more clear picture how we want to change it for imx9.
>
>>
>> Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
>> ---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h | 1 -
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 135 +++++++++---------
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.h | 5 +-
>> 3 files changed, 68 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> index ed15ea348f97..a2b4fb9e29e7 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> @@ -58,7 +58,6 @@
>> #define CAST_OFBSIZE_LO CAST_STATUS18
>> #define CAST_OFBSIZE_HI CAST_STATUS19
>>
>> -#define MXC_MAX_SLOTS 1 /* TODO use all 4 slots*/
>> /* JPEG-Decoder Wrapper Slot Registers 0..3 */
>> #define SLOT_BASE 0x10000
>> #define SLOT_STATUS 0x0
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index c0e49be42450..9512c0a61966 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -745,87 +745,77 @@ static void notify_src_chg(struct mxc_jpeg_ctx *ctx)
>> v4l2_event_queue_fh(&ctx->fh, &ev);
>> }
>>
>> -static int mxc_get_free_slot(struct mxc_jpeg_slot_data slot_data[],
>> int n)
>> +static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
>> {
>> - int free_slot = 0;
>> -
>> - while (slot_data[free_slot].used && free_slot < n)
>> - free_slot++;
>> -
>> - return free_slot; /* >=n when there are no more free slots */
>> + if (!slot_data->used)
>> + return slot_data->slot;
>> + return -1;
>> }
>>
>> -static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
>> - unsigned int slot)
>> +static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>> {
>> struct mxc_jpeg_desc *desc;
>> struct mxc_jpeg_desc *cfg_desc;
>> void *cfg_stm;
>>
>> - if (jpeg->slot_data[slot].desc)
>> + if (jpeg->slot_data.desc)
>> goto skip_alloc; /* already allocated, reuse it */
>>
>> /* allocate descriptor for decoding/encoding phase */
>> desc = dma_alloc_coherent(jpeg->dev,
>> sizeof(struct mxc_jpeg_desc),
>> - &jpeg->slot_data[slot].desc_handle,
>> + &jpeg->slot_data.desc_handle,
>> GFP_ATOMIC);
>> if (!desc)
>> goto err;
>> - jpeg->slot_data[slot].desc = desc;
>> + jpeg->slot_data.desc = desc;
>>
>> /* allocate descriptor for configuration phase (encoder only) */
>> cfg_desc = dma_alloc_coherent(jpeg->dev,
>> sizeof(struct mxc_jpeg_desc),
>> - &jpeg->slot_data[slot].cfg_desc_handle,
>> + &jpeg->slot_data.cfg_desc_handle,
>> GFP_ATOMIC);
>> if (!cfg_desc)
>> goto err;
>> - jpeg->slot_data[slot].cfg_desc = cfg_desc;
>> + jpeg->slot_data.cfg_desc = cfg_desc;
>>
>> /* allocate configuration stream */
>> cfg_stm = dma_alloc_coherent(jpeg->dev,
>> MXC_JPEG_MAX_CFG_STREAM,
>> - &jpeg->slot_data[slot].cfg_stream_handle,
>> + &jpeg->slot_data.cfg_stream_handle,
>> GFP_ATOMIC);
>> if (!cfg_stm)
>> goto err;
>> - jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
>> + jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
>>
>> skip_alloc:
>> - jpeg->slot_data[slot].used = true;
>> + jpeg->slot_data.used = true;
>>
>> return true;
>> err:
>> - dev_err(jpeg->dev, "Could not allocate descriptors for slot %d", slot);
>> + dev_err(jpeg->dev, "Could not allocate descriptors for slot %d",
>> +jpeg->slot_data.slot);
>>
>> return false;
>> }
>>
>> -static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg,
>> - unsigned int slot)
>> +static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>> {
>> - if (slot >= MXC_MAX_SLOTS) {
>> - dev_err(jpeg->dev, "Invalid slot %d, nothing to free.", slot);
>> - return;
>> - }
>> -
>> /* free descriptor for decoding/encoding phase */
>> dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> - jpeg->slot_data[slot].desc,
>> - jpeg->slot_data[slot].desc_handle);
>> + jpeg->slot_data.desc,
>> + jpeg->slot_data.desc_handle);
>>
>> /* free descriptor for encoder configuration phase / decoder DHT */
>> dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
>> - jpeg->slot_data[slot].cfg_desc,
>> - jpeg->slot_data[slot].cfg_desc_handle);
>> + jpeg->slot_data.cfg_desc,
>> + jpeg->slot_data.cfg_desc_handle);
>>
>> /* free configuration stream */
>> dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
>> - jpeg->slot_data[slot].cfg_stream_vaddr,
>> - jpeg->slot_data[slot].cfg_stream_handle);
>> + jpeg->slot_data.cfg_stream_vaddr,
>> + jpeg->slot_data.cfg_stream_handle);
>>
>> - jpeg->slot_data[slot].used = false;
>> + jpeg->slot_data.used = false;
>> }
>>
>> static void mxc_jpeg_check_and_set_last_buffer(struct mxc_jpeg_ctx
>> *ctx, @@
>> -855,7 +845,7 @@ static void mxc_jpeg_job_finish(struct mxc_jpeg_ctx
>> *ctx, enum vb2_buffer_state
>> v4l2_m2m_buf_done(dst_buf, state);
>>
>> mxc_jpeg_disable_irq(reg, ctx->slot);
>> - ctx->mxc_jpeg->slot_data[ctx->slot].used = false;
>> + jpeg->slot_data.used = false;
>> if (reset)
>> mxc_jpeg_sw_reset(reg);
>> }
>> @@ -919,7 +909,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void
>*priv)
>> goto job_unlock;
>> }
>>
>> - if (!jpeg->slot_data[slot].used)
>> + if (!jpeg->slot_data.used)
>> goto job_unlock;
>>
>> dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); @@ -
>> 1179,13 +1169,13 @@ static void mxc_jpeg_config_dec_desc(struct
>> vb2_buffer *out_buf,
>> struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> void __iomem *reg = jpeg->base_reg;
>> unsigned int slot = ctx->slot;
>> - struct mxc_jpeg_desc *desc = jpeg->slot_data[slot].desc;
>> - struct mxc_jpeg_desc *cfg_desc = jpeg->slot_data[slot].cfg_desc;
>> - dma_addr_t desc_handle = jpeg->slot_data[slot].desc_handle;
>> - dma_addr_t cfg_desc_handle = jpeg->slot_data[slot].cfg_desc_handle;
>> - dma_addr_t cfg_stream_handle = jpeg-
>> >slot_data[slot].cfg_stream_handle;
>> - unsigned int *cfg_size = &jpeg->slot_data[slot].cfg_stream_size;
>> - void *cfg_stream_vaddr = jpeg->slot_data[slot].cfg_stream_vaddr;
>> + struct mxc_jpeg_desc *desc = jpeg->slot_data.desc;
>> + struct mxc_jpeg_desc *cfg_desc = jpeg->slot_data.cfg_desc;
>> + dma_addr_t desc_handle = jpeg->slot_data.desc_handle;
>> + dma_addr_t cfg_desc_handle = jpeg->slot_data.cfg_desc_handle;
>> + dma_addr_t cfg_stream_handle = jpeg->slot_data.cfg_stream_handle;
>> + unsigned int *cfg_size = &jpeg->slot_data.cfg_stream_size;
>> + void *cfg_stream_vaddr = jpeg->slot_data.cfg_stream_vaddr;
>> struct mxc_jpeg_src_buf *jpeg_src_buf;
>>
>> jpeg_src_buf = vb2_to_mxc_buf(src_buf); @@ -1245,18 +1235,18 @@
>> static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
>> struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> void __iomem *reg = jpeg->base_reg;
>> unsigned int slot = ctx->slot;
>> - struct mxc_jpeg_desc *desc = jpeg->slot_data[slot].desc;
>> - struct mxc_jpeg_desc *cfg_desc = jpeg->slot_data[slot].cfg_desc;
>> - dma_addr_t desc_handle = jpeg->slot_data[slot].desc_handle;
>> - dma_addr_t cfg_desc_handle = jpeg->slot_data[slot].cfg_desc_handle;
>> - void *cfg_stream_vaddr = jpeg->slot_data[slot].cfg_stream_vaddr;
>> + struct mxc_jpeg_desc *desc = jpeg->slot_data.desc;
>> + struct mxc_jpeg_desc *cfg_desc = jpeg->slot_data.cfg_desc;
>> + dma_addr_t desc_handle = jpeg->slot_data.desc_handle;
>> + dma_addr_t cfg_desc_handle = jpeg->slot_data.cfg_desc_handle;
>> + void *cfg_stream_vaddr = jpeg->slot_data.cfg_stream_vaddr;
>> struct mxc_jpeg_q_data *q_data;
>> enum mxc_jpeg_image_format img_fmt;
>> int w, h;
>>
>> q_data = mxc_jpeg_get_q_data(ctx, src_buf->vb2_queue->type);
>>
>> - jpeg->slot_data[slot].cfg_stream_size =
>> + jpeg->slot_data.cfg_stream_size =
>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>> q_data->fmt->fourcc,
>> q_data->crop.width,
>> @@ -1265,7 +1255,7 @@ static void mxc_jpeg_config_enc_desc(struct
>> vb2_buffer *out_buf,
>> /* chain the config descriptor with the encoding descriptor */
>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
>>
>> - cfg_desc->buf_base0 = jpeg->slot_data[slot].cfg_stream_handle;
>> + cfg_desc->buf_base0 = jpeg->slot_data.cfg_stream_handle;
>> cfg_desc->buf_base1 = 0;
>> cfg_desc->line_pitch = 0;
>> cfg_desc->stm_bufbase = 0; /* no output expected */ @@ -1408,7
>> +1398,7 @@ static void mxc_jpeg_device_run_timeout(struct work_struct
>> *work)
>> unsigned long flags;
>>
>> spin_lock_irqsave(&ctx->mxc_jpeg->hw_lock, flags);
>> - if (ctx->slot < MXC_MAX_SLOTS && ctx->mxc_jpeg->slot_data[ctx-
>> >slot].used) {
>> + if (ctx->mxc_jpeg->slot_data.used) {
>> dev_warn(jpeg->dev, "%s timeout, cancel it\n",
>> ctx->mxc_jpeg->mode == MXC_JPEG_DECODE ?
>> "decode" : "encode");
>> mxc_jpeg_job_finish(ctx, VB2_BUF_STATE_ERROR, true); @@
>-
>> 1476,12 +1466,12 @@ static void mxc_jpeg_device_run(void *priv)
>> mxc_jpeg_enable(reg);
>> mxc_jpeg_set_l_endian(reg, 1);
>>
>> - ctx->slot = mxc_get_free_slot(jpeg->slot_data, MXC_MAX_SLOTS);
>> - if (ctx->slot >= MXC_MAX_SLOTS) {
>> + ctx->slot = mxc_get_free_slot(&jpeg->slot_data);
>> + if (ctx->slot < 0) {
>> dev_err(dev, "No more free slots\n");
>> goto end;
>> }
>> - if (!mxc_jpeg_alloc_slot_data(jpeg, ctx->slot)) {
>> + if (!mxc_jpeg_alloc_slot_data(jpeg)) {
>> dev_err(dev, "Cannot allocate slot data\n");
>> goto end;
>> }
>> @@ -2101,7 +2091,7 @@ static int mxc_jpeg_open(struct file *file)
>> }
>> ctx->fh.ctrl_handler = &ctx->ctrl_handler;
>> mxc_jpeg_set_default_params(ctx);
>> - ctx->slot = MXC_MAX_SLOTS; /* slot not allocated yet */
>> + ctx->slot = -1; /* slot not allocated yet */
>> INIT_DELAYED_WORK(&ctx->task_timer,
>> mxc_jpeg_device_run_timeout);
>>
>> if (mxc_jpeg->mode == MXC_JPEG_DECODE) @@ -2677,6 +2667,11
>@@ static
>> int mxc_jpeg_attach_pm_domains(struct mxc_jpeg_dev *jpeg)
>> dev_err(dev, "No power domains defined for jpeg node\n");
>> return jpeg->num_domains;
>> }
>> + if (jpeg->num_domains == 1) {
>> + /* genpd_dev_pm_attach() attach automatically if power
>> domains count is 1 */
>
>This looks like dead code to me, we always have at least 2 power domains (ex:
>IMX_SC_R_MJPEG_DEC_MP & IMX_SC_R_MJPEG_DEC_S0) ?
>
>Regards,
>Mirela
>
>> + jpeg->num_domains = 0;
>> + return 0;
>> + }
>>
>> jpeg->pd_dev = devm_kmalloc_array(dev, jpeg->num_domains,
>> sizeof(*jpeg->pd_dev), GFP_KERNEL);
>@@ -2718,7 +2713,6 @@
>> static int mxc_jpeg_probe(struct platform_device
>> *pdev)
>> int ret;
>> int mode;
>> const struct of_device_id *of_id;
>> - unsigned int slot;
>>
>> of_id = of_match_node(mxc_jpeg_match, dev->of_node);
>> if (!of_id)
>> @@ -2742,19 +2736,22 @@ static int mxc_jpeg_probe(struct
>> platform_device
>> *pdev)
>> if (IS_ERR(jpeg->base_reg))
>> return PTR_ERR(jpeg->base_reg);
>>
>> - for (slot = 0; slot < MXC_MAX_SLOTS; slot++) {
>> - dec_irq = platform_get_irq(pdev, slot);
>> - if (dec_irq < 0) {
>> - ret = dec_irq;
>> - goto err_irq;
>> - }
>> - ret = devm_request_irq(&pdev->dev, dec_irq,
>mxc_jpeg_dec_irq,
>> - 0, pdev->name, jpeg);
>> - if (ret) {
>> - dev_err(&pdev->dev, "Failed to request irq %d
>(%d)\n",
>> - dec_irq, ret);
>> - goto err_irq;
>> - }
>> + ret = of_property_read_u32_index(pdev->dev.of_node, "slot", 0,
>> +&jpeg-
>> >slot_data.slot);
>> + if (ret)
>> + jpeg->slot_data.slot = 0;
>> + dev_info(&pdev->dev, "choose slot %d\n", jpeg->slot_data.slot);
>> + dec_irq = platform_get_irq(pdev, 0);
>> + if (dec_irq < 0) {
>> + dev_err(&pdev->dev, "Failed to get irq %d\n", dec_irq);
>> + ret = dec_irq;
>> + goto err_irq;
>> + }
>> + ret = devm_request_irq(&pdev->dev, dec_irq, mxc_jpeg_dec_irq,
>> + 0, pdev->name, jpeg);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq %d (%d)\n",
>> + dec_irq, ret);
>> + goto err_irq;
>> }
>>
>> jpeg->pdev = pdev;
>> @@ -2914,11 +2911,9 @@ static const struct dev_pm_ops
>> mxc_jpeg_pm_ops = {
>>
>> static void mxc_jpeg_remove(struct platform_device *pdev) {
>> - unsigned int slot;
>> struct mxc_jpeg_dev *jpeg = platform_get_drvdata(pdev);
>>
>> - for (slot = 0; slot < MXC_MAX_SLOTS; slot++)
>> - mxc_jpeg_free_slot_data(jpeg, slot);
>> + mxc_jpeg_free_slot_data(jpeg);
>>
>> pm_runtime_disable(&pdev->dev);
>> video_unregister_device(jpeg->dec_vdev);
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> index 87157db78082..d80e94cc9d99 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
>> @@ -97,7 +97,7 @@ struct mxc_jpeg_ctx {
>> struct mxc_jpeg_q_data cap_q;
>> struct v4l2_fh fh;
>> enum mxc_jpeg_enc_state enc_state;
>> - unsigned int slot;
>> + int slot;
>> unsigned int source_change;
>> bool header_parsed;
>> struct v4l2_ctrl_handler ctrl_handler;
>> @@ -106,6 +106,7 @@ struct mxc_jpeg_ctx { };
>>
>> struct mxc_jpeg_slot_data {
>> + int slot;
>> bool used;
>> struct mxc_jpeg_desc *desc; // enc/dec descriptor
>> struct mxc_jpeg_desc *cfg_desc; // configuration descriptor @@
>> -128,7
>> +129,7 @@ struct mxc_jpeg_dev {
>> struct v4l2_device v4l2_dev;
>> struct v4l2_m2m_dev *m2m_dev;
>> struct video_device *dec_vdev;
>> - struct mxc_jpeg_slot_data slot_data[MXC_MAX_SLOTS];
>> + struct mxc_jpeg_slot_data slot_data;
>> int num_domains;
>> struct device **pd_dev;
>> struct device_link **pd_link;
>>
>> base-commit: a23a3041c733e068bed5ece88acb45fe0edf0413
>> --
>> 2.38.1
