On 01/12/15 10:42, tiffany lin wrote:

quoted

quoted

quoted

  > 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]

quoted

quoted

quoted

  > +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.

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

quoted

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.

quoted

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html