driverdev-devel.linuxdriverproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jernej Škrabec" <jernej.skrabec@siol.net>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org,
	linux-sunxi@googlegroups.com, linux-kernel@vger.kernel.org,
	mripard@kernel.org, wens@csie.org, hverkuil-cisco@xs4all.nl,
	mchehab@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
Date: Wed, 06 Nov 2019 18:41:01 +0100	[thread overview]
Message-ID: <2224545.8hcbHn5fu6@jernej-laptop> (raw)
In-Reply-To: <20191105081034.GC584930@aptenodytes>

Dne torek, 05. november 2019 ob 09:10:34 CET je Paul Kocialkowski napisal(a):
> 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 <paul.kocialkowski@bootlin.com>
> > > 
> > > 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.

Well, I can take a look at my HW, but I have only few SoCs with more or less 
same capability.

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

I took a quick look at existing controls, but I don't see anything 
appropriate.

Best regards,
Jernej

> 
> Cheers,
> 
> Paul
> 
> > Best regards,
> > Jernej
> > 
> > > Anyway that can be done later since we were already hardcoding this.
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > > 
> > > >  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)




_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply	other threads:[~2019-11-06 17:41 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-26  7:49 [PATCH 0/3] media: cedrus: Add support for 4k videos Jernej Skrabec
2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
2019-11-04 10:02   ` Paul Kocialkowski
2019-11-04 16:33     ` Jernej Škrabec
2019-11-05  8:10       ` Paul Kocialkowski
2019-11-06 17:41         ` Jernej Škrabec [this message]
2019-10-26  7:49 ` [PATCH 2/3] media: cedrus: Fix H264 4k support Jernej Skrabec
2019-11-04 10:13   ` Paul Kocialkowski
2019-11-04 16:53     ` Jernej Škrabec
2019-11-05  8:07       ` Paul Kocialkowski
2019-10-26  7:49 ` [PATCH 3/3] media: cedrus: Increase maximum supported size Jernej Skrabec
2019-11-04 10:14   ` Paul Kocialkowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2224545.8hcbHn5fu6@jernej-laptop \
    --to=jernej.skrabec@siol.net \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=mchehab@kernel.org \
    --cc=mripard@kernel.org \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).