Date:	Tue, 1 Dec 2015 15:42:43 +0000
From:	Daniel Thompson <daniel.thompson@...aro.org>
To:	tiffany lin <tiffany.lin@...iatek.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Daniel Kurtz <djkurtz@...omium.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Hongzhou Yang <hongzhou.yang@...iatek.com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Sakari Ailus <sakari.ailus@....fi>,
	Mikhail Ulyanov <mikhail.ulyanov@...entembedded.com>,
	Fabien Dessenne <fabien.dessenne@...com>,
	Arnd Bergmann <arnd@...db.de>,
	Darren Etheridge <detheridge@...com>,
	Peter Griffin <peter.griffin@...aro.org>,
	Benoit Parrot <bparrot@...com>,
	Andrew-CT Chen (陳智迪) 
	<Andrew-CT.Chen@...iatek.com>,
	Eddie Huang (黃智傑) 
	<eddie.huang@...iatek.com>,
	Yingjoe Chen (陳英洲) 
	<Yingjoe.Chen@...iatek.com>,
	JamesJJ Liao (廖建智) 
	<jamesjj.liao@...iatek.com>,
	Daniel Hsiao (蕭伯剛) 
	<daniel.hsiao@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" 
	<linux-mediatek@...ts.infradead.org>,
	PoChun Lin (林柏君) 
	<PoChun.Lin@...iatek.com>
Subject: Re: [RESEND RFC/PATCH 6/8] media: platform: mtk-vcodec: Add Mediatek
 V4L2 Video Encoder Driver

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

Powered by blists - more mailing lists