[RESEND RFC/PATCH 6/8] media: platform: mtk-vcodec: Add Mediatek V4L2 Video Encoder Driver

[Daniel Thompson]

On 01/12/15 10:42, tiffany lin wrote:
>>>>   > diff --git a/drivers/media/platform/mtk-vcodec/common/venc_drv_if.c
>>>> b/drivers/media/platform/mtk-vcodec/common/venc_drv_if.c
>>>>   > new file mode 100644
>>>>   > index 0000000..9b3f025
>>>>   > --- /dev/null
> [snip]
>>>>   > +int venc_if_create(void *ctx, unsigned int fourcc, unsigned long
>>>> *handle)
>>>>   > +{
>>>>   > +  struct venc_handle *h;
>>>>   > +  char str[10];
>>>>   > +
>>>>   > +  mtk_vcodec_fmt2str(fourcc, str);
>>>>   > +
>>>>   > +  h = kzalloc(sizeof(*h), GFP_KERNEL);
>>>>   > +  if (!h)
>>>>   > +          return -ENOMEM;
>>>>   > +
>>>>   > +  h->fourcc = fourcc;
>>>>   > +  h->ctx = ctx;
>>>>   > +  mtk_vcodec_debug(h, "fmt = %s handle = %p", str, h);
>>>>   > +
>>>>   > +  switch (fourcc) {
>>>>   > +  default:
>>>>   > +          mtk_vcodec_err(h, "invalid format %s", str);
>>>>   > +          goto err_out;
>>>>   > +  }
>>>>   > +
>>>>   > +  *handle = (unsigned long)h;
>>>>   > +  return 0;
>>>>   > +
>>>>   > +err_out:
>>>>   > +  kfree(h);
>>>>   > +  return -EINVAL;
>>>>   > +}
>>>>   > +
>>>>   > +int venc_if_init(unsigned long handle)
>>>>   > +{
>>>>   > +  int ret = 0;
>>>>   > +  struct venc_handle *h = (struct venc_handle *)handle;
>>>>   > +
>>>>   > +  mtk_vcodec_debug_enter(h);
>>>>   > +
>>>>   > +  mtk_venc_lock(h->ctx);
>>>>   > +  mtk_vcodec_enc_clock_on();
>>>>   > +  vpu_enable_clock(vpu_get_plat_device(h->ctx->dev->plat_dev));
>>>>   > +  ret = h->enc_if->init(h->ctx, (unsigned long *)&h->drv_handle);
>>>>   > +  vpu_disable_clock(vpu_get_plat_device(h->ctx->dev->plat_dev));
>>>>   > +  mtk_vcodec_enc_clock_off();
>>>>   > +  mtk_venc_unlock(h->ctx);
>>>>   > +
>>>>   > +  return ret;
>>>>   > +}
>>>>
>>>> To me this looks more like an obfuscation layer rather than a
>>>> abstraction layer. I don't understand why we need to hide things from
>>>> the V4L2 implementation that this code forms part of.
>>>>
>>>> More importantly, if this code was included somewhere where it could be
>>>> properly integrated with the device model you might be able to use the
>>>> pm_runtime system to avoid this sort of "heroics" to manage the clocks
>>>> anyway.
>>>>
>>> We want to abstract common part from encoder driver.
>>> Every encoder driver follow same calling flow and only need to take care
>>> about how to communicate with vpu to encode specific format.
>>> Encoder driver do not need to take care clock and multiple instance
>>> issue.
>>
>> Looking at each of those stages:
>>
>> mtk_venc_lock():
>> Why isn't one of the existing V4L2 locking strategies ok for you?
>>
> We only has one encoder hw.
> To support multiple encode instances.
> When one encoder ctx access encoder hw, it need to get lock first.
>
>> mtk_vcodec_enc_clock_on():
>> This does seem like something a sub-driver *should* be doing for itself
> This is for enabling encoder hw related clock.
> To support multiple instances, one encode ctx must get hw lock first
> then clock on/off hw relate clock.
>
>> vpu_enable_clock():
>> Why can't the VPU driver manage this internally using pm_runtime?
>>
> Our VPU do not have power domain.
> We will remove VPU clock on/off and let vpu control it in next version.
>
>>
>> That is why I described this as an obfuscation layer. It is collecting
>> a bunch of stuff that can be handled using the kernel driver model and
>> clumping them together in a special middle layer.
>>
> We do use kernel driver model, but we put it in
> mtk_vcodec_enc_clock_on/mtk_vcodec_enc_clock_off.
> Every sub-driver has no need to write the same code.
> And once clock configuration change or porting to other chips, we don't
> need to change sub-driver one-by-one, just change abstract layer.

I'm afraid I remain extremely unconvinced by the value of this API. It 
is possible that once the types are fixed and it is tidied up it won't 
stick out so much but I will be very surprised.

Either way, I can wait until v2 before we discuss it further.


>>>> If the start streaming operation implemented cleanup-on-error properly
>>>> then there would only be two useful states: Started and stopped. Even
>>>> the "sticky" error behavior looks unnecessary to me (meaning we don't
>>>> need to track its state).
>>>>
>>> We cannot guaranteed that IOCTLs called from the user space follow
>>> required sequence.
>>> We need states to know if our driver could accept IOCTL command.
>>
>> I believe that knowing whether the streaming is started or stopped
>> (e.g. two states) is sufficient for a driver to correctly handle
>> abitrary ioctls from userspace and even then, the core code tracks
>> this state for you so there's no need for you do it.
>>
>> The queue/dequeue ioctls succeed or fail based on the length of the
>> queue (i.e. is the buffer queue overflowing or not) and have no need
>> to check the streaming state.
>
>> If you are absolutely sure that the other states are needed then
>> please provide an example of an ioctl() sequence where the additional
>> state is needed.
>>
> I know your point that we have too many state changes in start_streaming
> and stop_streaming function.
> We will refine these two functions in next version.
>
> For the example, we need MTK_STATE_HEADER state, to make sure before
> encode start, driver already get information to set encode parameters.

Interesting. Again, I'll wait to see how the state simplifcation goes 
before commenting further.


> We need MTK_STATE_ABORT to inform encoder thread (mtk_venc_worker) that
> stop encodeing job from stopped ctx instance.
> When user space qbuf, we need to make sure everything is ready to sent
> buf to encode.

Agree that you need a flag here. In fact currently you have two, 
MTK_STATE_ABORT and an unused one called aborting.

You need to be very careful with these flags though. They are a magnet 
for data race bugs (especially combined with SMP).

For example at present I can't see any locking in the worker code. This 
means there is nothing to make all those read-modify-write sequences 
that manage the state atomic (thus risking state corruption).


Daniel.