Hi, On Mon 04 Nov 19, 17:33, Jernej Škrabec wrote: > Dne ponedeljek, 04. november 2019 ob 11:02:28 CET je Paul Kocialkowski > napisal(a): > > Hi Jernej, > > > > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote: > > > Mode register also holds information if video width is bigger than 2048 > > > and if it is equal to 4096. > > > > > > Rework cedrus_engine_enable() to properly signal this properties. > > > > Thanks for the patch, looks good to me! > > > > Acked-by: Paul Kocialkowski > > > > One minor thing: maybe we should have a way to set the maximum dimensions > > depending on the generation of the engine in use and the actual maximum > > supported by the hardware. > > > > Maybe either as dedicated new fields in struct cedrus_variant or as > > capability flags. > > I was thinking about first solution, but after going trough manuals, it was > unclear what are real limitations. For example, H3 manual states that it is > capable of decoding H264 1080p@60Hz. However, I know for a fact that it is > also capable of decoding 4k videos, but probably not at 60 Hz. I don't own > anything older that A83T, so I don't know what are capabilities of those SoCs. So I guess in this case we should try and see. I could try to look into it at some point in the future too if you're not particulary interested. > Anyway, being slow is still ok for some tasks, like transcoding, so we can't > limit decoding to 1080p just because it's slow. It is probably still faster > than doing it in SW. Not to mention that it's still ok for some videos, a lot > of them uses 24 fps. I agree, it's best to expose the maximum supported resolution by the hardware, even if it means running at a lower fps. Do you know if we have a way to report some estimation of the maximum supported fps to userspace? It would be useful to let userspace decide whether it's a better fit than software decoding. Cheers, Paul > Best regards, > Jernej > > > > > Anyway that can be done later since we were already hardcoding this. > > > > Cheers, > > > > Paul > > > > > Signed-off-by: Jernej Skrabec > > > --- > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 2 +- > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- > > > drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 9 +++++++-- > > > drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 +- > > > drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +- > > > drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 ++ > > > 6 files changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > 7487f6ab7576..d2c854ecdf15 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx, > > > > > > { > > > > > > struct cedrus_dev *dev = ctx->dev; > > > > > > - cedrus_engine_enable(dev, CEDRUS_CODEC_H264); > > > + cedrus_engine_enable(ctx, CEDRUS_CODEC_H264); > > > > > > cedrus_write(dev, VE_H264_SDROT_CTRL, 0); > > > cedrus_write(dev, VE_H264_EXTRA_BUFFER1, > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > 9bc921866f70..6945dc74e1d7 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx, > > > > > > } > > > > > > /* Activate H265 engine. */ > > > > > > - cedrus_engine_enable(dev, CEDRUS_CODEC_H265); > > > + cedrus_engine_enable(ctx, CEDRUS_CODEC_H265); > > > > > > /* Source offset and length in bits. */ > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index > > > 570a9165dd5d..3acfa21bc124 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > > @@ -30,7 +30,7 @@ > > > > > > #include "cedrus_hw.h" > > > #include "cedrus_regs.h" > > > > > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec) > > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec) > > > > > > { > > > > > > u32 reg = 0; > > > > > > @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum > > > cedrus_codec codec)> > > > return -EINVAL; > > > > > > } > > > > > > - cedrus_write(dev, VE_MODE, reg); > > > + if (ctx->src_fmt.width == 4096) > > > + reg |= VE_MODE_PIC_WIDTH_IS_4096; > > > + if (ctx->src_fmt.width > 2048) > > > + reg |= VE_MODE_PIC_WIDTH_MORE_2048; > > > + > > > + cedrus_write(ctx->dev, VE_MODE, reg); > > > > > > return 0; > > > > > > } > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index > > > 27d0882397aa..604ff932fbf5 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > > @@ -16,7 +16,7 @@ > > > > > > #ifndef _CEDRUS_HW_H_ > > > #define _CEDRUS_HW_H_ > > > > > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec > > > codec); > > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec > > > codec); > > > > > > void cedrus_engine_disable(struct cedrus_dev *dev); > > > > > > void cedrus_dst_format_set(struct cedrus_dev *dev, > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index > > > 13c34927bad5..8bcd6b8f9e2d 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > > @@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, > > > struct cedrus_run *run)> > > > quantization = run->mpeg2.quantization; > > > > > > /* Activate MPEG engine. */ > > > > > > - cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2); > > > + cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2); > > > > > > /* Set intra quantization matrix. */ > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index > > > 4275a307d282..ace3d49fcd82 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > @@ -35,6 +35,8 @@ > > > > > > #define VE_MODE 0x00 > > > > > > +#define VE_MODE_PIC_WIDTH_IS_4096 BIT(22) > > > +#define VE_MODE_PIC_WIDTH_MORE_2048 BIT(21) > > > > > > #define VE_MODE_REC_WR_MODE_2MB (0x01 << 20) > > > #define VE_MODE_REC_WR_MODE_1MB (0x00 << 20) > > > #define VE_MODE_DDR_MODE_BW_128 (0x03 << 16) > > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com