On Wed, Jan 2, 2019 at 12:22 PM Hans Verkuil <[email protected]> wrote:
>
> On 12/30/2018 02:48 PM, Dafna Hirschfeld wrote:
> >
> >
> > On Sun, Dec 30, 2018 at 2:22 PM Hans Verkuil <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > On 12/29/2018 04:20 PM, Dafna Hirschfeld wrote:
> > > Add support for the selection api for the crop and compose targets.
> > > The driver rounds up the coded width and height such that
> > > all planes dimensions are multiple of 8.
> > >
> > > Signed-off-by: Dafna Hirschfeld <[email protected]
> > <mailto:[email protected]>>
> > > ---
> > > Main changes from v4:
> > > In s/g_selection, check buffer type according to 'multiplanar'
> > variable.
> > > Spliting some lines longer than 80 chars.
> > >
> > > drivers/media/platform/vicodec/codec-fwht.c | 67 +++++---
> > > drivers/media/platform/vicodec/codec-fwht.h | 17 +-
> > > .../media/platform/vicodec/codec-v4l2-fwht.c | 36 ++--
> > > .../media/platform/vicodec/codec-v4l2-fwht.h | 6 +-
> > > drivers/media/platform/vicodec/vicodec-core.c | 161
> > ++++++++++++++----
> > > 5 files changed, 205 insertions(+), 82 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/vicodec/codec-fwht.c
> > b/drivers/media/platform/vicodec/codec-fwht.c
> > > index a6fd0477633b..db2c62c4d9bd 100644
> > > --- a/drivers/media/platform/vicodec/codec-fwht.c
> > > +++ b/drivers/media/platform/vicodec/codec-fwht.c
> > > @@ -10,6 +10,7 @@
> > > */
> > >
> > > #include <linux/string.h>
> > > +#include <linux/kernel.h>
> > > #include "codec-fwht.h"
> > >
> > > /*
> > > @@ -660,7 +661,7 @@ static void add_deltas(s16 *deltas, const u8
> > *ref, int stride)
> > >
> > > static u32 encode_plane(u8 *input, u8 *refp, __be16 **rlco, __be16
> > *rlco_max,
> > > struct fwht_cframe *cf, u32 height, u32 width,
> > > - unsigned int input_step,
> > > + u32 coded_width, unsigned int input_step,
> >
> > coded_width? That should be 'stride'.
> >
> > You have added crop support for the encoder, but you are probably only
> > testing with heights
> > that are not a multiple of 8. When you can crop, then it is perfectly
> > fine to crop e.g.
> > 640x480 from a 1920x1080 buffer. In that case you need the stride in
> > encode_plane (i.e. 1920).
> >
> > Part of the confusion here might be due to a difference between
> > encode_plane and decode_plane:
> > encode_plane reads directly from the vb2 output buffer, decode_plane
> > decodes to state->ref_frame,
> > which is then copied by v4l2_fwht_decode() to the actual vb2 capture
> > buffer.
> >
> > Basically whenever you need to know where the next line starts in the
> > vb2 buffer, you are
> > dealing with the stride, not with the visible or coded width.
> >
> >
> > From what I understand 'stride = coded_width * step'
> > If that is so then encode_plane already computes the stride from the
> > coded_width and step variables. For example, for 1920x1080 buffer
> > with crop of 640x480, then width=640 and coded_width=1920. Then
> > encode_plane calculates the stride from the coded_width and step.
> > For RGB for example the stride is 3*coded_width, for YUV420 it is
> > 1*coded_width and so on.
>
> No, the stride is equal to the 'bytesperline' value in v4l2_pix_format.
>
> Which currently happens to be equal to coded_width * step, but that is
> something that should change in the future for the vicodec driver.
>
> There are two reasons in general why bytesperline can be larger than the
> actual number of bytes needed for all the pixels of one line: either the
> hardware has some alignment requirements (e.g. each line must be a multiple
> of X bytes, where X is usually some power of 2), or the driver supports
> userspace setting bytesperline to a custom value. The latter is allowed
> by the API, although many drivers will ignore the userspace-provided value.
>
> A use-case of that is that userspace allocates a 3840x2160 buffer and decodes
> 4 1080p streams, and each stream is composed to one quadrant of that larger
> buffer.
>
> Right now the vicodec driver ignores bytesperline, but it's something that
> should be added later, so it is good practice to prepare the driver for this
> and refer to 'stride' as the size of a single line in the buffer.
>
> >
> >
> > > bool is_intra, bool next_is_intra)
> > > {
> > > u8 *input_start = input;
> > > @@ -671,7 +672,11 @@ static u32 encode_plane(u8 *input, u8 *refp,
> > __be16 **rlco, __be16 *rlco_max,
> > > unsigned int last_size = 0;
> > > unsigned int i, j;
> > >
> > > + width = round_up(width, 8);
> > > + height = round_up(height, 8);
> > > +
> > > for (j = 0; j < height / 8; j++) {
> > > + input = input_start + j * 8 * coded_width * input_step;
> > > for (i = 0; i < width / 8; i++) {
> > > /* intra code, first frame is always intra
> > coded. */
> > > int blocktype = IBLOCK;
> > > @@ -679,9 +684,9 @@ static u32 encode_plane(u8 *input, u8 *refp,
> > __be16 **rlco, __be16 *rlco_max,
> > >
> > > if (!is_intra)
> > > blocktype = decide_blocktype(input,
> > refp,
> > > - deltablock, width, input_step);
> > > + deltablock, coded_width,
> > input_step);
> > > if (blocktype == IBLOCK) {
> > > - fwht(input, cf->coeffs, width,
> > input_step, 1);
> > > + fwht(input, cf->coeffs, coded_width,
> > input_step, 1);
> > > quantize_intra(cf->coeffs,
> > cf->de_coeffs,
> > > cf->i_frame_qp);
> > > } else {
> > > @@ -722,7 +727,6 @@ static u32 encode_plane(u8 *input, u8 *refp,
> > __be16 **rlco, __be16 *rlco_max,
> > > }
> > > last_size = size;
> > > }
> > > - input += width * 7 * input_step;
> > > }
> > >
> > > exit_loop:
> > > @@ -747,30 +751,32 @@ static u32 encode_plane(u8 *input, u8 *refp,
> > __be16 **rlco, __be16 *rlco_max,
> > > u32 fwht_encode_frame(struct fwht_raw_frame *frm,
> > > struct fwht_raw_frame *ref_frm,
> > > struct fwht_cframe *cf,
> > > - bool is_intra, bool next_is_intra)
> > > + bool is_intra, bool next_is_intra,
> > > + unsigned int width, unsigned int height)
> > > {
> > > - unsigned int size = frm->height * frm->width;
> > > + unsigned int size = height * width;
> > > __be16 *rlco = cf->rlc_data;
> > > __be16 *rlco_max;
> > > u32 encoding;
> > >
> > > rlco_max = rlco + size / 2 - 256;
> > > encoding = encode_plane(frm->luma, ref_frm->luma, &rlco,
> > rlco_max, cf,
> > > - frm->height, frm->width,
> > > + height, width, frm->coded_width,
> >
> > Should be frm->stride.
> >
> > > frm->luma_alpha_step, is_intra,
> > next_is_intra);
> > > if (encoding & FWHT_FRAME_UNENCODED)
> > > encoding |= FWHT_LUMA_UNENCODED;
> > > encoding &= ~FWHT_FRAME_UNENCODED;
> > >
> > > if (frm->components_num >= 3) {
> > > - u32 chroma_h = frm->height / frm->height_div;
> > > - u32 chroma_w = frm->width / frm->width_div;
> > > + u32 chroma_h = height / frm->height_div;
> > > + u32 chroma_w = width / frm->width_div;
> > > + u32 chroma_coded_width = frm->coded_width /
> > frm->width_div;
> >
> > chroma_stride = frm->stride / frm->width_div;
This calculation of chroma_stride does not work for formats such as
YUYV where frm->width_div = 2 but the chromas are not in separate
planes.
How do I calculate the stride in general ?
For yuv420 for example, where the chromas are in separate planes, is
'chroma_stride = stride / 2' ?
> >
> > > unsigned int chroma_size = chroma_h * chroma_w;
> > >
> > > rlco_max = rlco + chroma_size / 2 - 256;
> > > encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco,
> > rlco_max,
> > > cf, chroma_h, chroma_w,
> > > - frm->chroma_step,
> > > + chroma_coded_width,
> > frm->chroma_step,
> > > is_intra, next_is_intra);
> > > if (encoding & FWHT_FRAME_UNENCODED)
> > > encoding |= FWHT_CB_UNENCODED;
> > > @@ -778,7 +784,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
> > > rlco_max = rlco + chroma_size / 2 - 256;
> > > encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco,
> > rlco_max,
> > > cf, chroma_h, chroma_w,
> > > - frm->chroma_step,
> > > + chroma_coded_width,
> > frm->chroma_step,
> > > is_intra, next_is_intra);
> > > if (encoding & FWHT_FRAME_UNENCODED)
> > > encoding |= FWHT_CR_UNENCODED;
> > > @@ -788,8 +794,8 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
> > > if (frm->components_num == 4) {
> > > rlco_max = rlco + size / 2 - 256;
> > > encoding |= encode_plane(frm->alpha, ref_frm->alpha,
> > &rlco,
> > > - rlco_max, cf, frm->height,
> > frm->width,
> > > - frm->luma_alpha_step,
> > > + rlco_max, cf, height, width,
> > > + frm->coded_width,
> > frm->luma_alpha_step,
> > > is_intra, next_is_intra);
> > > if (encoding & FWHT_FRAME_UNENCODED)
> > > encoding |= FWHT_ALPHA_UNENCODED;
> > > @@ -801,7 +807,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
> > > }
> > >
> > > static void decode_plane(struct fwht_cframe *cf, const __be16
> > **rlco, u8 *ref,
> > > - u32 height, u32 width, bool uncompressed)
> > > + u32 height, u32 width, u32 coded_width, bool
> > uncompressed)
> >
> > coded_width is OK here since you are writing into 'ref' and not the vb2
> > capture buffer.
Actually I think coded_width is not needed at all for the decoder
because it only use the internal ref_frame buffer.
So the width and height are enough.
>
> >
> > > {
> > > unsigned int copies = 0;
> > > s16 copy[8 * 8];
> > > @@ -813,6 +819,8 @@ static void decode_plane(struct fwht_cframe *cf,
> > const __be16 **rlco, u8 *ref,
> > > *rlco += width * height / 2;
> > > return;
> > > }
> > > + width = round_up(width, 8);
> > > + height = round_up(height, 8);
> > >
> > > /*
> > > * When decoding each macroblock the rlco pointer will be
> > increased
> > > @@ -822,13 +830,13 @@ static void decode_plane(struct fwht_cframe
> > *cf, const __be16 **rlco, u8 *ref,
> > > */
> > > for (j = 0; j < height / 8; j++) {
> > > for (i = 0; i < width / 8; i++) {
> > > - u8 *refp = ref + j * 8 * width + i * 8;
> > > + u8 *refp = ref + j * 8 * coded_width + i * 8;
> > >
> > > if (copies) {
> > > memcpy(cf->de_fwht, copy, sizeof(copy));
> > > if (stat & PFRAME_BIT)
> > > - add_deltas(cf->de_fwht, refp,
> > width);
> > > - fill_decoder_block(refp, cf->de_fwht,
> > width);
> > > + add_deltas(cf->de_fwht, refp,
> > coded_width);
> > > + fill_decoder_block(refp, cf->de_fwht,
> > coded_width);
> > > copies--;
> > > continue;
> > > }
> > > @@ -847,35 +855,40 @@ static void decode_plane(struct fwht_cframe
> > *cf, const __be16 **rlco, u8 *ref,
> > > if (copies)
> > > memcpy(copy, cf->de_fwht, sizeof(copy));
> > > if (stat & PFRAME_BIT)
> > > - add_deltas(cf->de_fwht, refp, width);
> > > - fill_decoder_block(refp, cf->de_fwht, width);
> > > + add_deltas(cf->de_fwht, refp,
> > coded_width);
> > > + fill_decoder_block(refp, cf->de_fwht,
> > coded_width);
> > > }
> > > }
> > > }
> > >
> > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame
> > *ref,
> > > - u32 hdr_flags, unsigned int components_num)
> > > + u32 hdr_flags, unsigned int components_num,
> > > + unsigned int width, unsigned int height,
> > > + unsigned int coded_width)
> > > {
> > > const __be16 *rlco = cf->rlc_data;
> > >
> > > - decode_plane(cf, &rlco, ref->luma, cf->height, cf->width,
> > > + decode_plane(cf, &rlco, ref->luma, height, width, coded_width,
> > > hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED);
> > >
> > > if (components_num >= 3) {
> > > - u32 h = cf->height;
> > > - u32 w = cf->width;
> > > + u32 h = height;
> > > + u32 w = width;
> > > + u32 c = coded_width;
> > >
> > > if (!(hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT))
> > > h /= 2;
> > > - if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH))
> > > + if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)) {
> > > w /= 2;
> > > - decode_plane(cf, &rlco, ref->cb, h, w,
> > > + c /= 2;
> > > + }
> > > + decode_plane(cf, &rlco, ref->cb, h, w, c,
> > > hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
> > > - decode_plane(cf, &rlco, ref->cr, h, w,
> > > + decode_plane(cf, &rlco, ref->cr, h, w, c,
> > > hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
> > > }
> > >
> > > if (components_num == 4)
> > > - decode_plane(cf, &rlco, ref->alpha, cf->height,
> > cf->width,
> > > + decode_plane(cf, &rlco, ref->alpha, height, width,
> > coded_width,
> > > hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED);
> > > }
> > > diff --git a/drivers/media/platform/vicodec/codec-fwht.h
> > b/drivers/media/platform/vicodec/codec-fwht.h
> > > index 90ff8962fca7..d7e0d9b87c96 100644
> > > --- a/drivers/media/platform/vicodec/codec-fwht.h
> > > +++ b/drivers/media/platform/vicodec/codec-fwht.h
> > > @@ -81,6 +81,13 @@
> > > #define FWHT_FL_COMPONENTS_NUM_MSK GENMASK(17, 16)
> > > #define FWHT_FL_COMPONENTS_NUM_OFFSET 16
> > >
> > > +/*
> > > + * A macro to calculate the needed padding in order to make sure
> > > + * both luma and chroma components resolutions are rounded up to
> > > + * closest multiple of 8
> > > + */
> > > +#define vic_round_dim(dim, div) (round_up((dim) / (div), 8) * (div))
> > > +
> > > struct fwht_cframe_hdr {
> > > u32 magic1;
> > > u32 magic2;
> > > @@ -95,7 +102,6 @@ struct fwht_cframe_hdr {
> > > };
> > >
> > > struct fwht_cframe {
> > > - unsigned int width, height;
> > > u16 i_frame_qp;
> > > u16 p_frame_qp;
> > > __be16 *rlc_data;
> > > @@ -106,12 +112,12 @@ struct fwht_cframe {
> > > };
> > >
> > > struct fwht_raw_frame {
> > > - unsigned int width, height;
> > > unsigned int width_div;
> > > unsigned int height_div;
> > > unsigned int luma_alpha_step;
> > > unsigned int chroma_step;
> > > unsigned int components_num;
> > > + unsigned int coded_width;
> >
> > Add the 'stride' field here as well. With crop/compose support stride
> > is no
> > longer equal to coded_width.
> >
> > > u8 *luma, *cb, *cr, *alpha;
> > > };
> > >
> > > @@ -125,8 +131,11 @@ struct fwht_raw_frame {
> > > u32 fwht_encode_frame(struct fwht_raw_frame *frm,
> > > struct fwht_raw_frame *ref_frm,
> > > struct fwht_cframe *cf,
> > > - bool is_intra, bool next_is_intra);
> > > + bool is_intra, bool next_is_intra,
> > > + unsigned int width, unsigned int height);
> > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame
> > *ref,
> > > - u32 hdr_flags, unsigned int components_num);
> > > + u32 hdr_flags, unsigned int components_num,
> > > + unsigned int width, unsigned int height,
> > > + unsigned int coded_width);
> > >
> > > #endif
> > > diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> > b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> > > index 8cb0212df67f..19f9d65fc0a9 100644
> > > --- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> > > +++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> > > @@ -56,7 +56,7 @@ const struct v4l2_fwht_pixfmt_info
> > *v4l2_fwht_get_pixfmt(u32 idx)
> > >
> > > int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8
> > *p_out)
> > > {
> > > - unsigned int size = state->width * state->height;
> > > + unsigned int size = state->coded_width * state->coded_height;
> > > const struct v4l2_fwht_pixfmt_info *info = state->info;
> > > struct fwht_cframe_hdr *p_hdr;
> > > struct fwht_cframe cf;
> > > @@ -66,8 +66,8 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state,
> > u8 *p_in, u8 *p_out)
> > >
> > > if (!info)
> > > return -EINVAL;
> > > - rf.width = state->width;
> > > - rf.height = state->height;
> > > +
> > > + rf.coded_width = state->coded_width;
> >
> > You need to set the stride value here as well.
> >
> > > rf.luma = p_in;
> > > rf.width_div = info->width_div;
> > > rf.height_div = info->height_div;
> > > @@ -163,15 +163,14 @@ int v4l2_fwht_encode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > return -EINVAL;
> > > }
> > >
> > > - cf.width = state->width;
> > > - cf.height = state->height;
> > > cf.i_frame_qp = state->i_frame_qp;
> > > cf.p_frame_qp = state->p_frame_qp;
> > > cf.rlc_data = (__be16 *)(p_out + sizeof(*p_hdr));
> > >
> > > encoding = fwht_encode_frame(&rf, &state->ref_frame, &cf,
> > > !state->gop_cnt,
> > > - state->gop_cnt == state->gop_size
> > - 1);
> > > + state->gop_cnt == state->gop_size
> > - 1,
> > > + state->visible_width,
> > state->visible_height);
> > > if (!(encoding & FWHT_FRAME_PCODED))
> > > state->gop_cnt = 0;
> > > if (++state->gop_cnt >= state->gop_size)
> > > @@ -181,8 +180,8 @@ int v4l2_fwht_encode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > p_hdr->magic1 = FWHT_MAGIC1;
> > > p_hdr->magic2 = FWHT_MAGIC2;
> > > p_hdr->version = htonl(FWHT_VERSION);
> > > - p_hdr->width = htonl(cf.width);
> > > - p_hdr->height = htonl(cf.height);
> > > + p_hdr->width = htonl(state->visible_width);
> > > + p_hdr->height = htonl(state->visible_height);
> > > flags |= (info->components_num - 1) <<
> > FWHT_FL_COMPONENTS_NUM_OFFSET;
> > > if (encoding & FWHT_LUMA_UNENCODED)
> > > flags |= FWHT_FL_LUMA_IS_UNCOMPRESSED;
> > > @@ -202,15 +201,13 @@ int v4l2_fwht_encode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > p_hdr->ycbcr_enc = htonl(state->ycbcr_enc);
> > > p_hdr->quantization = htonl(state->quantization);
> > > p_hdr->size = htonl(cf.size);
> > > - state->ref_frame.width = cf.width;
> > > - state->ref_frame.height = cf.height;
> > > return cf.size + sizeof(*p_hdr);
> > > }
> > >
> > > int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8
> > *p_out)
> > > {
> > > - unsigned int size = state->width * state->height;
> > > - unsigned int chroma_size = size;
> > > + unsigned int size;
> > > + unsigned int chroma_size;
> > > unsigned int i;
> > > u32 flags;
> > > struct fwht_cframe_hdr *p_hdr;
> > > @@ -218,13 +215,15 @@ int v4l2_fwht_decode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > u8 *p;
> > > unsigned int components_num = 3;
> > > unsigned int version;
> > > + const struct v4l2_fwht_pixfmt_info *info;
> > >
> > > if (!state->info)
> > > return -EINVAL;
> > >
> > > + info = state->info;
> > > + size = state->coded_width * state->coded_height;
> > > + chroma_size = size;
> > > p_hdr = (struct fwht_cframe_hdr *)p_in;
> > > - cf.width = ntohl(p_hdr->width);
> > > - cf.height = ntohl(p_hdr->height);
> > >
> > > version = ntohl(p_hdr->version);
> > > if (!version || version > FWHT_VERSION) {
> > > @@ -234,12 +233,12 @@ int v4l2_fwht_decode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > }
> > >
> > > if (p_hdr->magic1 != FWHT_MAGIC1 ||
> > > - p_hdr->magic2 != FWHT_MAGIC2 ||
> > > - (cf.width & 7) || (cf.height & 7))
> > > + p_hdr->magic2 != FWHT_MAGIC2)
> > > return -EINVAL;
> > >
> > > /* TODO: support resolution changes */
> > > - if (cf.width != state->width || cf.height != state->height)
> > > + if (ntohl(p_hdr->width) != state->visible_width ||
> > > + ntohl(p_hdr->height) != state->visible_height)
> > > return -EINVAL;
> > >
> > > flags = ntohl(p_hdr->flags);
> > > @@ -260,7 +259,8 @@ int v4l2_fwht_decode(struct v4l2_fwht_state
> > *state, u8 *p_in, u8 *p_out)
> > > if (!(flags & FWHT_FL_CHROMA_FULL_HEIGHT))
> > > chroma_size /= 2;
> > >
> > > - fwht_decode_frame(&cf, &state->ref_frame, flags,
> > components_num);
> > > + fwht_decode_frame(&cf, &state->ref_frame, flags, components_num,
> > > + state->visible_width, state->visible_height,
> > state->coded_width);
> > >
> > > /*
> > > * TODO - handle the case where the compressed stream encodes a
> >
> > When full composition support is added for the decoder, then we need to
> > use the stride value
> > and the code after the fwht_decode_frame() call has to be rewritten to
> > a for loop going
> > over each output line.
> >
> > > diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.h
> > b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
> > > index ed53e28d4f9c..2a09ad13ddd6 100644
> > > --- a/drivers/media/platform/vicodec/codec-v4l2-fwht.h
> > > +++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
> > > @@ -23,8 +23,10 @@ struct v4l2_fwht_pixfmt_info {
> > >
> > > struct v4l2_fwht_state {
> > > const struct v4l2_fwht_pixfmt_info *info;
> > > - unsigned int width;
> > > - unsigned int height;
> > > + unsigned int visible_width;
> > > + unsigned int visible_height;
> > > + unsigned int coded_width;
> > > + unsigned int coded_height;
> > > unsigned int gop_size;
> > > unsigned int gop_cnt;
> > > u16 i_frame_qp;
> > > diff --git a/drivers/media/platform/vicodec/vicodec-core.c
> > b/drivers/media/platform/vicodec/vicodec-core.c
> > > index 0d7876f5acf0..bbcce826c440 100644
> > > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > > @@ -75,8 +75,10 @@ static struct platform_device vicodec_pdev = {
> > >
> > > /* Per-queue, driver-specific private data */
> > > struct vicodec_q_data {
> > > - unsigned int width;
> > > - unsigned int height;
> > > + unsigned int coded_width;
> > > + unsigned int coded_height;
> > > + unsigned int visible_width;
> > > + unsigned int visible_height;
> > > unsigned int sizeimage;
> > > unsigned int sequence;
> > > const struct v4l2_fwht_pixfmt_info *info;
> > > @@ -464,11 +466,11 @@ static int vidioc_g_fmt(struct vicodec_ctx
> > *ctx, struct v4l2_format *f)
> > > if (multiplanar)
> > > return -EINVAL;
> > > pix = &f->fmt.pix;
> > > - pix->width = q_data->width;
> > > - pix->height = q_data->height;
> > > + pix->width = q_data->coded_width;
> > > + pix->height = q_data->coded_height;
> > > pix->field = V4L2_FIELD_NONE;
> > > pix->pixelformat = info->id;
> > > - pix->bytesperline = q_data->width *
> > info->bytesperline_mult;
> > > + pix->bytesperline = q_data->coded_width *
> > info->bytesperline_mult;
> > > pix->sizeimage = q_data->sizeimage;
> > > pix->colorspace = ctx->state.colorspace;
> > > pix->xfer_func = ctx->state.xfer_func;
> > > @@ -481,13 +483,13 @@ static int vidioc_g_fmt(struct vicodec_ctx
> > *ctx, struct v4l2_format *f)
> > > if (!multiplanar)
> > > return -EINVAL;
> > > pix_mp = &f->fmt.pix_mp;
> > > - pix_mp->width = q_data->width;
> > > - pix_mp->height = q_data->height;
> > > + pix_mp->width = q_data->coded_width;
> > > + pix_mp->height = q_data->coded_height;
> > > pix_mp->field = V4L2_FIELD_NONE;
> > > pix_mp->pixelformat = info->id;
> > > pix_mp->num_planes = 1;
> > > pix_mp->plane_fmt[0].bytesperline =
> > > - q_data->width * info->bytesperline_mult;
> > > + q_data->coded_width *
> > info->bytesperline_mult;
> > > pix_mp->plane_fmt[0].sizeimage = q_data->sizeimage;
> > > pix_mp->colorspace = ctx->state.colorspace;
> > > pix_mp->xfer_func = ctx->state.xfer_func;
> > > @@ -528,8 +530,8 @@ static int vidioc_try_fmt(struct vicodec_ctx
> > *ctx, struct v4l2_format *f)
> > > pix = &f->fmt.pix;
> > > if (pix->pixelformat != V4L2_PIX_FMT_FWHT)
> > > info = find_fmt(pix->pixelformat);
> > > - pix->width = clamp(pix->width, MIN_WIDTH, MAX_WIDTH) &
> > ~7;
> > > - pix->height = clamp(pix->height, MIN_HEIGHT,
> > MAX_HEIGHT) & ~7;
> > > + pix->width = vic_round_dim(clamp(pix->width, MIN_WIDTH,
> > MAX_WIDTH), info->width_div);
> > > + pix->height = vic_round_dim(clamp(pix->height,
> > MIN_HEIGHT, MAX_HEIGHT), info->height_div);
> > > pix->field = V4L2_FIELD_NONE;
> > > pix->bytesperline =
> > > pix->width * info->bytesperline_mult;
> > > @@ -545,9 +547,8 @@ static int vidioc_try_fmt(struct vicodec_ctx
> > *ctx, struct v4l2_format *f)
> > > if (pix_mp->pixelformat != V4L2_PIX_FMT_FWHT)
> > > info = find_fmt(pix_mp->pixelformat);
> > > pix_mp->num_planes = 1;
> > > - pix_mp->width = clamp(pix_mp->width, MIN_WIDTH,
> > MAX_WIDTH) & ~7;
> > > - pix_mp->height =
> > > - clamp(pix_mp->height, MIN_HEIGHT, MAX_HEIGHT) &
> > ~7;
> > > + pix_mp->width = vic_round_dim(clamp(pix_mp->width,
> > MIN_WIDTH, MAX_WIDTH), info->width_div);
> > > + pix_mp->height = vic_round_dim(clamp(pix->height,
> > MIN_HEIGHT, MAX_HEIGHT), info->height_div);
> > > pix_mp->field = V4L2_FIELD_NONE;
> > > plane->bytesperline =
> > > pix_mp->width * info->bytesperline_mult;
> > > @@ -658,8 +659,8 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx,
> > struct v4l2_format *f)
> > > if (ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type))
> > > fmt_changed =
> > > q_data->info->id != pix->pixelformat ||
> > > - q_data->width != pix->width ||
> > > - q_data->height != pix->height;
> > > + q_data->coded_width != pix->width ||
> > > + q_data->coded_height != pix->height;
> > >
> > > if (vb2_is_busy(vq) && fmt_changed)
> > > return -EBUSY;
> > > @@ -668,8 +669,8 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx,
> > struct v4l2_format *f)
> > > q_data->info = &pixfmt_fwht;
> > > else
> > > q_data->info = find_fmt(pix->pixelformat);
> > > - q_data->width = pix->width;
> > > - q_data->height = pix->height;
> > > + q_data->coded_width = pix->width;
> > > + q_data->coded_height = pix->height;
> > > q_data->sizeimage = pix->sizeimage;
> > > break;
> > > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > @@ -678,8 +679,8 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx,
> > struct v4l2_format *f)
> > > if (ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type))
> > > fmt_changed =
> > > q_data->info->id != pix_mp->pixelformat
> > ||
> > > - q_data->width != pix_mp->width ||
> > > - q_data->height != pix_mp->height;
> > > + q_data->coded_width != pix_mp->width ||
> > > + q_data->coded_height != pix_mp->height;
> > >
> > > if (vb2_is_busy(vq) && fmt_changed)
> > > return -EBUSY;
> > > @@ -688,17 +689,24 @@ static int vidioc_s_fmt(struct vicodec_ctx
> > *ctx, struct v4l2_format *f)
> > > q_data->info = &pixfmt_fwht;
> > > else
> > > q_data->info = find_fmt(pix_mp->pixelformat);
> > > - q_data->width = pix_mp->width;
> > > - q_data->height = pix_mp->height;
> > > + q_data->coded_width = pix_mp->width;
> > > + q_data->coded_height = pix_mp->height;
> > > q_data->sizeimage = pix_mp->plane_fmt[0].sizeimage;
> > > break;
> > > default:
> > > return -EINVAL;
> > > }
> > > + if (q_data->visible_width > q_data->coded_width)
> > > + q_data->visible_width = q_data->coded_width;
> > > + if (q_data->visible_height > q_data->coded_height)
> > > + q_data->visible_height = q_data->coded_height;
> > > +
> > >
> > > dprintk(ctx->dev,
> > > - "Setting format for type %d, wxh: %dx%d, fourcc:
> > %08x\n",
> > > - f->type, q_data->width, q_data->height,
> > q_data->info->id);
> > > + "Setting format for type %d, coded wxh: %dx%d, visible
> > wxh: %dx%d, fourcc: %08x\n",
> > > + f->type, q_data->coded_width, q_data->coded_height,
> > > + q_data->visible_width, q_data->visible_height,
> > > + q_data->info->id);
> > >
> > > return 0;
> > > }
> > > @@ -753,6 +761,89 @@ static int vidioc_s_fmt_vid_out(struct file
> > *file, void *priv,
> > > return ret;
> > > }
> > >
> > > +static int vidioc_g_selection(struct file *file, void *priv,
> > > + struct v4l2_selection *s)
> > > +{
> > > + struct vicodec_ctx *ctx = file2ctx(file);
> > > + struct vicodec_q_data *q_data;
> > > + enum v4l2_buf_type valid_cap_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > + enum v4l2_buf_type valid_out_type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > +
> > > + if (multiplanar) {
> > > + valid_cap_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > > + valid_out_type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > > + }
> > > +
> > > + q_data = get_q_data(ctx, s->type);
> > > + if (!q_data)
> > > + return -EINVAL;
> > > + /*
> > > + * encoder supports only cropping on the OUTPUT buffer
> > > + * decoder supports only composing on the CAPTURE buffer
> > > + */
> > > + if ((ctx->is_enc && s->type == valid_out_type) ||
> > > + (!ctx->is_enc && s->type == valid_cap_type)) {
> > > + switch (s->target) {
> > > + case V4L2_SEL_TGT_COMPOSE:
> > > + case V4L2_SEL_TGT_CROP:
> > > + s->r.left = 0;
> > > + s->r.top = 0;
> > > + s->r.width = q_data->visible_width;
> > > + s->r.height = q_data->visible_height;
> > > + return 0;
> > > + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > > + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + s->r.left = 0;
> > > + s->r.top = 0;
> > > + s->r.width = q_data->coded_width;
> > > + s->r.height = q_data->coded_height;
> > > + return 0;
> > > + }
> > > + }
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int vidioc_s_selection(struct file *file, void *priv,
> > > + struct v4l2_selection *s)
> > > +{
> > > + struct vicodec_ctx *ctx = file2ctx(file);
> > > + struct vicodec_q_data *q_data;
> > > + bool is_out_crop_on_enc;
> > > + bool is_cap_compose_on_dec;
> > > + enum v4l2_buf_type valid_cap_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > + enum v4l2_buf_type valid_out_type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > +
> > > + if (multiplanar) {
> > > + valid_cap_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > > + valid_out_type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > > + }
> > > +
> > > + q_data = get_q_data(ctx, s->type);
> > > + if (!q_data)
> > > + return -EINVAL;
> > > +
> > > + is_out_crop_on_enc = ctx->is_enc &&
> > > + s->type == valid_out_type &&
> > > + s->target == V4L2_SEL_TGT_CROP;
> > > +
> > > + is_cap_compose_on_dec = !ctx->is_enc &&
> > > + s->type == valid_cap_type &&
> > > + s->target == V4L2_SEL_TGT_COMPOSE;
> > > +
> > > + if (!is_out_crop_on_enc && !is_cap_compose_on_dec)
> > > + return -EINVAL;
> > > +
> > > + s->r.left = 0;
> > > + s->r.top = 0;
> > > + q_data->visible_width = clamp(s->r.width, MIN_WIDTH,
> > q_data->coded_width);
> > > + s->r.width = q_data->visible_width;
> > > + q_data->visible_height = clamp(s->r.height, MIN_HEIGHT,
> > q_data->coded_height);
> > > + s->r.height = q_data->visible_height;
> >
> > This is OK for the encoder, but for the decoder this doesn't work.
> >
> > The main reason is that this requires that, as mentioned above, the
> > v4l2_fwht_decode()
> > function can handle a stride. To implement that the code writing to the
> > capture buffer
> > needs to be rewritten to write line-by-line to the buffer, taking the
> > stride into account.
> >
> > Test what happens when creating a capture buffer of 1920x1080, but
> > decoding to a
> > 640x480 compose target inside that buffer (assuming you are decoding a
> > 640x480 compressed
> > stream, of course).
> >
> > Not sure I understand, isn't the stride still available from by
> > "coded_width * step" for each plane just like in the encoder case ?
>
> See my explanation above. Right now the two are equal, but they won't be in
> the future.
>
> Regards,
>
> Hans
>
> >
> > > + return 0;
> > > +}
> > > +
> > > static void vicodec_mark_last_buf(struct vicodec_ctx *ctx)
> > > {
> > > static const struct v4l2_event eos_event = {
> > > @@ -895,6 +986,9 @@ static const struct v4l2_ioctl_ops
> > vicodec_ioctl_ops = {
> > > .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > > .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > >
> > > + .vidioc_g_selection = vidioc_g_selection,
> > > + .vidioc_s_selection = vidioc_s_selection,
> > > +
> > > .vidioc_try_encoder_cmd = vicodec_try_encoder_cmd,
> > > .vidioc_encoder_cmd = vicodec_encoder_cmd,
> > > .vidioc_try_decoder_cmd = vicodec_try_decoder_cmd,
> > > @@ -988,8 +1082,8 @@ static int vicodec_start_streaming(struct
> > vb2_queue *q,
> > > struct vicodec_ctx *ctx = vb2_get_drv_priv(q);
> > > struct vicodec_q_data *q_data = get_q_data(ctx, q->type);
> > > struct v4l2_fwht_state *state = &ctx->state;
> > > - unsigned int size = q_data->width * q_data->height;
> > > const struct v4l2_fwht_pixfmt_info *info = q_data->info;
> > > + unsigned int size = q_data->coded_width * q_data->coded_height;
> > > unsigned int chroma_div = info->width_div * info->height_div;
> > > unsigned int total_planes_size;
> > >
> > > @@ -1008,17 +1102,20 @@ static int vicodec_start_streaming(struct
> > vb2_queue *q,
> > >
> > > if (!V4L2_TYPE_IS_OUTPUT(q->type)) {
> > > if (!ctx->is_enc) {
> > > - state->width = q_data->width;
> > > - state->height = q_data->height;
> > > + state->visible_width = q_data->visible_width;
> > > + state->visible_height = q_data->visible_height;
> > > + state->coded_width = q_data->coded_width;
> > > + state->coded_height = q_data->coded_height;
> > > }
> > > return 0;
> > > }
> > >
> > > if (ctx->is_enc) {
> > > - state->width = q_data->width;
> > > - state->height = q_data->height;
> > > + state->visible_width = q_data->visible_width;
> > > + state->visible_height = q_data->visible_height;
> > > + state->coded_width = q_data->coded_width;
> > > + state->coded_height = q_data->coded_height;
> > > }
> > > - state->ref_frame.width = state->ref_frame.height = 0;
> > > state->ref_frame.luma = kvmalloc(total_planes_size, GFP_KERNEL);
> > > ctx->comp_max_size = total_planes_size + sizeof(struct
> > fwht_cframe_hdr);
> > > state->compressed_frame = kvmalloc(ctx->comp_max_size,
> > GFP_KERNEL);
> > > @@ -1204,8 +1301,10 @@ static int vicodec_open(struct file *file)
> > >
> > > ctx->q_data[V4L2_M2M_SRC].info =
> > > ctx->is_enc ? v4l2_fwht_get_pixfmt(0) : &pixfmt_fwht;
> > > - ctx->q_data[V4L2_M2M_SRC].width = 1280;
> > > - ctx->q_data[V4L2_M2M_SRC].height = 720;
> > > + ctx->q_data[V4L2_M2M_SRC].coded_width = 1280;
> > > + ctx->q_data[V4L2_M2M_SRC].coded_height = 720;
> > > + ctx->q_data[V4L2_M2M_SRC].visible_width = 1280;
> > > + ctx->q_data[V4L2_M2M_SRC].visible_height = 720;
> > > size = 1280 * 720 *
> > ctx->q_data[V4L2_M2M_SRC].info->sizeimage_mult /
> > > ctx->q_data[V4L2_M2M_SRC].info->sizeimage_div;
> > > if (ctx->is_enc)
> > >
> >
> > Thanks,
> > Dafna
> >
> >
> > Regards,
> >
> > Hans
> >
Dafna
