On 07/21/2016 11:49 AM, Hans Verkuil wrote:
>
>
> On 07/21/2016 09:30 AM, Jean Christophe TROTIN wrote:
>>
>> On 07/18/2016 01:45 PM, Hans Verkuil wrote:
>>> Hi Jean-Christophe,
>>>
>>> See my review comments below. Nothing really major, but I do need to know more
>>> about the g/s_parm and the restriction on the number of open()s has to be lifted.
>>> That's not allowed.
>>>
>>
>> Hi Hans,
>>
>> Thank you for your comments.
>> I've explained below why I would like to keep 'hva' as driver's name and why the
>> frame rate is needed (g/s_parm).
>> I've followed your advice for managing the hardware restriction with regards to
>> the number of codec instances (see also below).
>> Finally, I've taken into account all the other comments.
>> All these modifications will be reflected in the version 3.
>>
>> Best regards,
>> Jean-Christophe.
>>
>>> On 07/11/2016 05:14 PM, Jean-Christophe Trotin wrote:
>>>> This patch adds V4L2 HVA (Hardware Video Accelerator) video encoder
>>>> driver for STMicroelectronics SoC. It uses the V4L2 mem2mem framework.
>>>>
>>>> This patch only contains the core parts of the driver:
>>>> - the V4L2 interface with the userland (hva-v4l2.c)
>>>> - the hardware services (hva-hw.c)
>>>> - the memory management utilities (hva-mem.c)
>>>>
>>>> This patch doesn't include the support of specific codec (e.g. H.264)
>>>> video encoding: this support is part of subsequent patches.
>>>>
>>>> Signed-off-by: Yannick Fertre <yannick.fertre@xxxxxx>
>>>> Signed-off-by: Jean-Christophe Trotin <jean-christophe.trotin@xxxxxx>
>>>> ---
>>>> drivers/media/platform/Kconfig | 14 +
>>>> drivers/media/platform/Makefile | 1 +
>>>> drivers/media/platform/sti/hva/Makefile | 2 +
>>>> drivers/media/platform/sti/hva/hva-hw.c | 534 ++++++++++++
>>>> drivers/media/platform/sti/hva/hva-hw.h | 42 +
>>>> drivers/media/platform/sti/hva/hva-mem.c | 60 ++
>>>> drivers/media/platform/sti/hva/hva-mem.h | 36 +
>>>> drivers/media/platform/sti/hva/hva-v4l2.c | 1299 +++++++++++++++++++++++++++++
>>>> drivers/media/platform/sti/hva/hva.h | 284 +++++++
>>>> 9 files changed, 2272 insertions(+)
>>>> create mode 100644 drivers/media/platform/sti/hva/Makefile
>>>> create mode 100644 drivers/media/platform/sti/hva/hva-hw.c
>>>> create mode 100644 drivers/media/platform/sti/hva/hva-hw.h
>>>> create mode 100644 drivers/media/platform/sti/hva/hva-mem.c
>>>> create mode 100644 drivers/media/platform/sti/hva/hva-mem.h
>>>> create mode 100644 drivers/media/platform/sti/hva/hva-v4l2.c
>>>> create mode 100644 drivers/media/platform/sti/hva/hva.h
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index 382f393..182d63f 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -227,6 +227,20 @@ config VIDEO_STI_BDISP
>>>> help
>>>> This v4l2 mem2mem driver is a 2D blitter for STMicroelectronics SoC.
>>>>
>>>> +config VIDEO_STI_HVA
>>>> + tristate "STMicroelectronics STiH41x HVA multi-format video encoder V4L2 driver"
>>>> + depends on VIDEO_DEV && VIDEO_V4L2
>>>> + depends on ARCH_STI || COMPILE_TEST
>>>> + select VIDEOBUF2_DMA_CONTIG
>>>> + select V4L2_MEM2MEM_DEV
>>>> + help
>>>> + This V4L2 driver enables HVA multi-format video encoder of
>>>
>>> Please mention here what HVA stands for.
>>>
>>
>> Done in version 3.
>> HVA stands for "Hardware Video Accelerator".
>>
>>>> + STMicroelectronics SoC STiH41x series, allowing hardware encoding of raw
>>>> + uncompressed formats in various compressed video bitstreams format.
>>>> +
>>>> + To compile this driver as a module, choose M here:
>>>> + the module will be called hva.
>>>
>>> How about sti-hva as the module name? 'hva' is a bit too generic.
>>>
>>
>> 'hva' is a generic IP which could be used on different STMicroelectronics SoCs.
>> That's the reason why I would like to keep this name. It's not specific to the
>> STiH41x series: thus, I've reworked the Kconfig's comment.
>
> How about st-hva? I really like it to be a bit more specific.
>
Hi Hans,
Thank you for your comments.
As discussed through IRC with you and Benjamin Gaignard last week, I will rename
the module "st-hva" in the version 4.
>>>> +static int hva_s_parm(struct file *file, void *fh, struct v4l2_streamparm *sp)
>>>> +{
>>>> + struct hva_ctx *ctx = fh_to_ctx(file->private_data);
>>>> + struct device *dev = ctx_to_dev(ctx);
>>>> + struct v4l2_fract *time_per_frame = &ctx->ctrls.time_per_frame;
>>>> +
>>>> + time_per_frame->numerator = sp->parm.capture.timeperframe.numerator;
>>>> + time_per_frame->denominator =
>>>> + sp->parm.capture.timeperframe.denominator;
>>>> +
>>>> + dev_dbg(dev, "%s set parameters %d/%d\n", ctx->name,
>>>> + time_per_frame->numerator, time_per_frame->denominator);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int hva_g_parm(struct file *file, void *fh, struct v4l2_streamparm *sp)
>>>> +{
>>>> + struct hva_ctx *ctx = fh_to_ctx(file->private_data);
>>>> + struct device *dev = ctx_to_dev(ctx);
>>>> + struct v4l2_fract *time_per_frame = &ctx->ctrls.time_per_frame;
>>>> +
>>>> + sp->parm.capture.timeperframe.numerator = time_per_frame->numerator;
>>>> + sp->parm.capture.timeperframe.denominator =
>>>> + time_per_frame->denominator;
>>>> +
>>>> + dev_dbg(dev, "%s get parameters %d/%d\n", ctx->name,
>>>> + time_per_frame->numerator, time_per_frame->denominator);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> This is strange. Normally codecs don't need this. You give them a buffer and
>>> it will be encoded/decoded and then you give it the next one if it is available.
>>> There is normally no frame rate involved.
>>>
>>> How does this work in this SoC? I need to know a bit more about this to be
>>> certain there isn't a misunderstanding somewhere.
>>>
>>
>> Among the parameters dimensioning its buffer model, the 'hva' HW IP needs to
>> calculate the depletion that is the quantity of bits at the output of the
>> encoder in each time slot, basically bitrate/framerate. That's the reason for
>> these 2 functions.
>> Furthermore, I've seen that mtk-vcodec and coda encoders also get the frame rate
>> to configure their HW IPs (vidioc_venc_s_parm & coda_s_parm).
>
> Ah, OK. That makes sense. So this is *only* used in calculating the bitrate(s),
> not in actual scheduling of threads or something like that, right?
>
> Regards,
>
> Hans
>
About the frame rate (G/S-PARM), I confirm that it's not used for any scheduling
of threads or anything like that: it's only for bitrate(s) calculation.
Best regards,
Jean-Christophe.--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
