[PATCH] [media] coda: disable BWB for all codecs on CODA 960

[PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Philipp Zabel]

I don't know what the BWB unit is, I guess W is for write and one of the
Bs is for burst. All I know is that there repeatedly have been issues
with it hanging on certain streams (ENGR00223231, ENGR00293425), with
various firmware versions, sometimes blocking something related to the
GDI bus or the GDI AXI adapter. There are some error cases that we don't
know how to recover from without a reboot. Apparently this unit can be
disabled by setting bit 12 in the FRAME_MEM_CTRL mailbox register to
zero, so do that to avoid crashes.

Side effects are reduced burst lengths when writing out decoded frames
to memory, so there is an "enable_bwb" module parameter to turn it back
on.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 7 ++++++-
 drivers/media/platform/coda/coda_regs.h   | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 4d25ca1981301..aeb2456830348 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -71,6 +71,10 @@ static int disable_vdoa;
 module_param(disable_vdoa, int, 0644);
 MODULE_PARM_DESC(disable_vdoa, "Disable Video Data Order Adapter tiled to raster-scan conversion");
 
+static int enable_bwb = 0;
+module_param(enable_bwb, int, 0644);
+MODULE_PARM_DESC(enable_bwb, "Enable BWB unit, may crash on certain streams");
+
 void coda_write(struct coda_dev *dev, u32 data, u32 reg)
 {
 	v4l2_dbg(2, coda_debug, &dev->v4l2_dev,
@@ -1891,7 +1895,8 @@ static int coda_open(struct file *file)
 	ctx->idx = idx;
 	switch (dev->devtype->product) {
 	case CODA_960:
-		ctx->frame_mem_ctrl = 1 << 12;
+		if (enable_bwb)
+			ctx->frame_mem_ctrl = CODA9_FRAME_ENABLE_BWB;
 		/* fallthrough */
 	case CODA_7541:
 		ctx->reg_idx = 0;
diff --git a/drivers/media/platform/coda/coda_regs.h b/drivers/media/platform/coda/coda_regs.h
index 3490602fa6e1e..77ee46a934272 100644
--- a/drivers/media/platform/coda/coda_regs.h
+++ b/drivers/media/platform/coda/coda_regs.h
@@ -51,6 +51,7 @@
 #define		CODA7_STREAM_SEL_64BITS_ENDIAN	(1 << 1)
 #define		CODA_STREAM_ENDIAN_SELECT	(1 << 0)
 #define CODA_REG_BIT_FRAME_MEM_CTRL		0x110
+#define		CODA9_FRAME_ENABLE_BWB		(1 << 12)
 #define		CODA9_FRAME_TILED2LINEAR	(1 << 11)
 #define		CODA_FRAME_CHROMA_INTERLEAVE	(1 << 2)
 #define		CODA_IMAGE_ENDIAN_SELECT	(1 << 0)
-- 
2.11.0

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

Hi Philipp,

On 02/03/17 10:19, Philipp Zabel wrote:
> I don't know what the BWB unit is, I guess W is for write and one of the
> Bs is for burst. All I know is that there repeatedly have been issues
> with it hanging on certain streams (ENGR00223231, ENGR00293425), with
> various firmware versions, sometimes blocking something related to the
> GDI bus or the GDI AXI adapter. There are some error cases that we don't
> know how to recover from without a reboot. Apparently this unit can be
> disabled by setting bit 12 in the FRAME_MEM_CTRL mailbox register to
> zero, so do that to avoid crashes.

Both those FSL ENGR patches to Android and VPU lib are specific to 
decode, with the first being specific to VC1 decode only.

> Side effects are reduced burst lengths when writing out decoded frames
> to memory, so there is an "enable_bwb" module parameter to turn it back
> on.

These side effects are dramatically reducing the VPU throughput during 
H.264 encode as well. Prior to this patch I was just about managing to 
capture and stream 1080p25 H.264. After this change it fell to about 
19fps. Reverting this patch (or presumably using the module param) 
restores the frame rate.

Can we at least make this decode specific? The VPU library patches do it 
in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream 
start would be OK.

It might also be worth adding some sort of /* HACK */ marker, since 
disabling the BWB feels very like a hack to me.

Regards,
Ian

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Philipp Zabel]

Hi Ian,

On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:
> Hi Philipp,
> 
> On 02/03/17 10:19, Philipp Zabel wrote:
> > I don't know what the BWB unit is, I guess W is for write and one of the
> > Bs is for burst. All I know is that there repeatedly have been issues
> > with it hanging on certain streams (ENGR00223231, ENGR00293425), with
> > various firmware versions, sometimes blocking something related to the
> > GDI bus or the GDI AXI adapter. There are some error cases that we don't
> > know how to recover from without a reboot. Apparently this unit can be
> > disabled by setting bit 12 in the FRAME_MEM_CTRL mailbox register to
> > zero, so do that to avoid crashes.
> 
> Both those FSL ENGR patches to Android and VPU lib are specific to 
> decode, with the first being specific to VC1 decode only.

The first one is older, I suppose VC1 is where the issue has been
observed first.
I have seen sporadic BWB hangups when decoding h.264 RTP streams.

> > Side effects are reduced burst lengths when writing out decoded frames
> > to memory, so there is an "enable_bwb" module parameter to turn it back
> > on.
> 
> These side effects are dramatically reducing the VPU throughput during 
> H.264 encode as well. Prior to this patch I was just about managing to 
> capture and stream 1080p25 H.264. After this change it fell to about 
> 19fps. Reverting this patch (or presumably using the module param) 
> restores the frame rate.
> 
> Can we at least make this decode specific? The VPU library patches do it 
> in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream 
> start would be OK.

Yes, since ENGR00293425 only talks about decoders, and I haven't seen
any BWB related hangups during encoding yet, I'm inclined to agree.

> It might also be worth adding some sort of /* HACK */ marker, since 
> disabling the BWB feels very like a hack to me.

I don't think HACK in itself is very descriptive, but a reference to the
original workaround could be helpful in the future.

regards
Philipp

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

On 19/07/17 09:09, Philipp Zabel wrote:
> Hi Ian,
> 
> On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:
>> Hi Philipp,
>>
>> On 02/03/17 10:19, Philipp Zabel wrote:
>>> Side effects are reduced burst lengths when writing out decoded frames
>>> to memory, so there is an "enable_bwb" module parameter to turn it back
>>> on.
>>
>> These side effects are dramatically reducing the VPU throughput during
>> H.264 encode as well. Prior to this patch I was just about managing to
>> capture and stream 1080p25 H.264. After this change it fell to about
>> 19fps. Reverting this patch (or presumably using the module param)
>> restores the frame rate.
>>
>> Can we at least make this decode specific? The VPU library patches do it
>> in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream
>> start would be OK.
> 
> Yes, since ENGR00293425 only talks about decoders, and I haven't seen
> any BWB related hangups during encoding yet, I'm inclined to agree.

I took a look at where ctx->frame_mem_ctrl is used, i.e.
coda_start_encoding and __coda_start_decoding. Is it possible to have
one instance of coda open for encode and one for decode at the same
time? If so, how is the CODA_REG_BIT_FRAME_MEM_CTRL setting
updated between runs, eg. for differing CODA_FRAME_CHROMA_INTERLEAVE?
This code is in the start_streaming path rather than the prepare_run
path. Does each context switch stop & restart streaming?

Regards,
Ian

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Philipp Zabel]

On Wed, 2017-07-19 at 10:15 +0100, Ian Arkver wrote:
> On 19/07/17 09:09, Philipp Zabel wrote:
> > Hi Ian,
> > 
> > On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:
> >> Hi Philipp,
> >>
> >> On 02/03/17 10:19, Philipp Zabel wrote:
> >>> Side effects are reduced burst lengths when writing out decoded frames
> >>> to memory, so there is an "enable_bwb" module parameter to turn it back
> >>> on.
> >>
> >> These side effects are dramatically reducing the VPU throughput during
> >> H.264 encode as well. Prior to this patch I was just about managing to
> >> capture and stream 1080p25 H.264. After this change it fell to about
> >> 19fps. Reverting this patch (or presumably using the module param)
> >> restores the frame rate.
> >>
> >> Can we at least make this decode specific? The VPU library patches do it
> >> in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream
> >> start would be OK.
> > 
> > Yes, since ENGR00293425 only talks about decoders, and I haven't seen
> > any BWB related hangups during encoding yet, I'm inclined to agree.
> 
> I took a look at where ctx->frame_mem_ctrl is used, i.e.
> coda_start_encoding and __coda_start_decoding. Is it possible to have
> one instance of coda open for encode and one for decode at the same
> time? 

Sure, when transcoding with a decoder / encoder pair, for example.

> If so, how is the CODA_REG_BIT_FRAME_MEM_CTRL setting
> updated between runs, eg. for differing CODA_FRAME_CHROMA_INTERLEAVE?

Yes. coda_command_async writes ctx->frame_mem_ctrl to
CODA_REG_BIT_FRAME_MEM_CTRL with each PIC_RUN command at from
prepare_encode/decode.

> This code is in the start_streaming path rather than the prepare_run
> path. Does each context switch stop & restart streaming?

No, see above.

regards
Philipp

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

On 19/07/17 10:32, Philipp Zabel wrote:
> On Wed, 2017-07-19 at 10:15 +0100, Ian Arkver wrote:
>> On 19/07/17 09:09, Philipp Zabel wrote:
>>> Hi Ian,
>>>
>>> On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:
>>>> Hi Philipp,
>>>>
>>>> On 02/03/17 10:19, Philipp Zabel wrote:
>>>>> Side effects are reduced burst lengths when writing out decoded frames
>>>>> to memory, so there is an "enable_bwb" module parameter to turn it back
>>>>> on.
>>>>
>>>> These side effects are dramatically reducing the VPU throughput during
>>>> H.264 encode as well. Prior to this patch I was just about managing to
>>>> capture and stream 1080p25 H.264. After this change it fell to about
>>>> 19fps. Reverting this patch (or presumably using the module param)
>>>> restores the frame rate.
>>>>
>>>> Can we at least make this decode specific? The VPU library patches do it
>>>> in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream
>>>> start would be OK.
>>>
>>> Yes, since ENGR00293425 only talks about decoders, and I haven't seen
>>> any BWB related hangups during encoding yet, I'm inclined to agree.
>>
>> I took a look at where ctx->frame_mem_ctrl is used, i.e.
>> coda_start_encoding and __coda_start_decoding. Is it possible to have
>> one instance of coda open for encode and one for decode at the same
>> time?
> 
> Sure, when transcoding with a decoder / encoder pair, for example.
> 
>> If so, how is the CODA_REG_BIT_FRAME_MEM_CTRL setting
>> updated between runs, eg. for differing CODA_FRAME_CHROMA_INTERLEAVE?
> 
> Yes. coda_command_async writes ctx->frame_mem_ctrl to
> CODA_REG_BIT_FRAME_MEM_CTRL with each PIC_RUN command at from
> prepare_encode/decode.

Totally missed that somehow. Thanks for the pointer.

>> This code is in the start_streaming path rather than the prepare_run
>> path. Does each context switch stop & restart streaming?
> 
> No, see above.
> 
> regards
> Philipp
>