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

[tiffany lin]

On Tue, 2015-12-01 at 15:42 +0000, Daniel Thompson wrote:
> 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).
> 
We prevent that one function set the flag and others clear the flag.
So there is no special lock to protect state.

> 
> Daniel.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/