On Mon, Oct 29, 2018 at 11:23 AM Hans Verkuil wrote: > Hi Dafna, > > Looks good overall, but I do have some - mostly minor - comments: > > On 10/24/2018 08:41 PM, Dafna Hirschfeld wrote: > > Add support for single plane greyscale format V4L2_PIX_FMT_GREY. > > Also change the header of the encoded file to include the number > > of components. > > > > Signed-off-by: Dafna Hirschfeld > > --- > > drivers/media/platform/vicodec/codec-fwht.c | 56 +++++++++++-------- > > drivers/media/platform/vicodec/codec-fwht.h | 5 +- > > .../media/platform/vicodec/codec-v4l2-fwht.c | 12 +++- > > drivers/media/platform/vicodec/vicodec-core.c | 21 +++++-- > > 4 files changed, 61 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/media/platform/vicodec/codec-fwht.c > b/drivers/media/platform/vicodec/codec-fwht.c > > index 36656031b295..91bfd1d9182b 100644 > > --- a/drivers/media/platform/vicodec/codec-fwht.c > > +++ b/drivers/media/platform/vicodec/codec-fwht.c > > @@ -753,9 +753,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > __be16 *rlco = cf->rlc_data; > > __be16 *rlco_max; > > u32 encoding; > > - u32 chroma_h = frm->height / frm->height_div; > > - u32 chroma_w = frm->width / frm->width_div; > > - unsigned int chroma_size = chroma_h * chroma_w; > > > > rlco_max = rlco + size / 2 - 256; > > encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, > cf, > > @@ -764,20 +761,27 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > if (encoding & FWHT_FRAME_UNENCODED) > > encoding |= FWHT_LUMA_UNENCODED; > > encoding &= ~FWHT_FRAME_UNENCODED; > > - rlco_max = rlco + chroma_size / 2 - 256; > > - encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, cf, > > + > > + if (frm->components_num > 1) { > > Just to be on the safe side, change this to: > > frm->components_num >= 3 > > since the code inside the if statements assumes that there are at least > three components and it would fail if components_num == 2. > > > + u32 chroma_h = frm->height / frm->height_div; > > + u32 chroma_w = frm->width / frm->width_div; > > + 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, is_intra, > next_is_intra); > > - if (encoding & FWHT_FRAME_UNENCODED) > > - encoding |= FWHT_CB_UNENCODED; > > - encoding &= ~FWHT_FRAME_UNENCODED; > > - rlco_max = rlco + chroma_size / 2 - 256; > > - encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, cf, > > + if (encoding & FWHT_FRAME_UNENCODED) > > + encoding |= FWHT_CB_UNENCODED; > > + encoding &= ~FWHT_FRAME_UNENCODED; > > + 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, is_intra, > next_is_intra); > > - if (encoding & FWHT_FRAME_UNENCODED) > > - encoding |= FWHT_CR_UNENCODED; > > - encoding &= ~FWHT_FRAME_UNENCODED; > > + if (encoding & FWHT_FRAME_UNENCODED) > > + encoding |= FWHT_CR_UNENCODED; > > + encoding &= ~FWHT_FRAME_UNENCODED; > > + } > > cf->size = (rlco - cf->rlc_data) * sizeof(*rlco); > > return encoding; > > } > > @@ -836,20 +840,24 @@ static void decode_plane(struct fwht_cframe *cf, > const __be16 **rlco, u8 *ref, > > } > > > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame > *ref, > > - u32 hdr_flags) > > + u32 hdr_flags, unsigned int components_num) > > { > > const __be16 *rlco = cf->rlc_data; > > - u32 h = cf->height / 2; > > - u32 w = cf->width / 2; > > > > - if (hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT) > > - h *= 2; > > - if (hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH) > > - w *= 2; > > decode_plane(cf, &rlco, ref->luma, cf->height, cf->width, > > hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED); > > - decode_plane(cf, &rlco, ref->cb, h, w, > > - hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED); > > - decode_plane(cf, &rlco, ref->cr, h, w, > > - hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED); > > + > > + if (components_num > 1) { > > Ditto: >= 3 > > > + u32 h = cf->height; > > + u32 w = cf->width; > > + > > + if (!(hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT)) > > + h /= 2; > > + if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)) > > + w /= 2; > > + decode_plane(cf, &rlco, ref->cb, h, w, > > + hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED); > > + decode_plane(cf, &rlco, ref->cr, h, w, > > + hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED); > > + } > > } > > diff --git a/drivers/media/platform/vicodec/codec-fwht.h > b/drivers/media/platform/vicodec/codec-fwht.h > > index 5750e21f16ac..5e6813aeaa96 100644 > > --- a/drivers/media/platform/vicodec/codec-fwht.h > > +++ b/drivers/media/platform/vicodec/codec-fwht.h > > @@ -56,7 +56,7 @@ > > #define FWHT_MAGIC1 0x4f4f4f4f > > #define FWHT_MAGIC2 0xffffffff > > > > -#define FWHT_VERSION 1 > > +#define FWHT_VERSION 2 > > > > /* Set if this is an interlaced format */ > > #define FWHT_FL_IS_INTERLACED BIT(0) > > @@ -81,6 +81,7 @@ struct fwht_cframe_hdr { > > u32 magic2; > > __be32 version; > > __be32 width, height; > > + u8 components_num; > > Rather than adding a new field, I prefer that it is encoded in the flags > field. > Take two bits (say bits 16 and 17) where you store the number of > components - 1. > > This simplifies decoders since they don't have to deal with different > structure > sizes. > > > __be32 flags; > > __be32 colorspace; > > __be32 xfer_func; > > @@ -122,6 +123,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > struct fwht_cframe *cf, > > bool is_intra, bool next_is_intra); > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame > *ref, > > - u32 hdr_flags); > > + u32 hdr_flags, unsigned int components_num); > > > > #endif > > diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c > b/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > index 64ad01e99bad..cc9275f3c6cb 100644 > > --- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > +++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > @@ -11,6 +11,7 @@ > > #include "codec-v4l2-fwht.h" > > > > static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = { > > + { V4L2_PIX_FMT_GREY, 1, 1, 1, 1, 0, 1, 1, 1}, > > { V4L2_PIX_FMT_YUV420, 1, 3, 2, 1, 1, 2, 2, 3}, > > { V4L2_PIX_FMT_YVU420, 1, 3, 2, 1, 1, 2, 2, 3}, > > { V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1, 3}, > > @@ -76,6 +77,9 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 > *p_in, u8 *p_out) > > rf.components_num = info->components_num; > > > > switch (info->id) { > > + case V4L2_PIX_FMT_GREY: > > + rf.cb = rf.cr = NULL; > > + break; > > case V4L2_PIX_FMT_YUV420: > > rf.cb = rf.luma + size; > > rf.cr = rf.cb + size / 4; > > @@ -166,6 +170,7 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, > u8 *p_in, u8 *p_out) > > p_hdr->version = htonl(FWHT_VERSION); > > p_hdr->width = htonl(cf.width); > > p_hdr->height = htonl(cf.height); > > + p_hdr->components_num = info->components_num; > > if (encoding & FWHT_LUMA_UNENCODED) > > flags |= FWHT_FL_LUMA_IS_UNCOMPRESSED; > > if (encoding & FWHT_CB_UNENCODED) > > @@ -196,6 +201,7 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, > u8 *p_in, u8 *p_out) > > struct fwht_cframe_hdr *p_hdr; > > struct fwht_cframe cf; > > u8 *p; > > + unsigned int components_num; > > This should default to 3. > > > > > if (!state->info) > > return -EINVAL; > > @@ -204,6 +210,7 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, > u8 *p_in, u8 *p_out) > > cf.width = ntohl(p_hdr->width); > > cf.height = ntohl(p_hdr->height); > > flags = ntohl(p_hdr->flags); > > + components_num = p_hdr->components_num; > > Only read this if the version is 2. If the version is 1, then it defaults > to 3. > And of course it should read the flags for the number of components as > mentioned > above. > > > state->colorspace = ntohl(p_hdr->colorspace); > > state->xfer_func = ntohl(p_hdr->xfer_func); > > state->ycbcr_enc = ntohl(p_hdr->ycbcr_enc); > > @@ -225,9 +232,12 @@ 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); > > + fwht_decode_frame(&cf, &state->ref_frame, flags, components_num); > > > > switch (state->info->id) { > > + case V4L2_PIX_FMT_GREY: > > + memcpy(p_out, state->ref_frame.luma, size); > > + break; > > case V4L2_PIX_FMT_YUV420: > > case V4L2_PIX_FMT_YUV422P: > > memcpy(p_out, state->ref_frame.luma, size); > > diff --git a/drivers/media/platform/vicodec/vicodec-core.c > b/drivers/media/platform/vicodec/vicodec-core.c > > index fb6d5e9a06ab..945b54efd4bd 100644 > > --- a/drivers/media/platform/vicodec/vicodec-core.c > > +++ b/drivers/media/platform/vicodec/vicodec-core.c > > @@ -993,6 +993,12 @@ static int vicodec_start_streaming(struct vb2_queue > *q, > > unsigned int size = q_data->width * q_data->height; > > const struct v4l2_fwht_pixfmt_info *info = q_data->info; > > unsigned int chroma_div = info->width_div * info->height_div; > > + unsigned int total_planes_size; > > + > > + if (info->components_num == 1) > > Use < 3 > Hi, But this case handles the specific case where the number of components is 1. > > + total_planes_size = size; > > + else > > + total_planes_size = size + 2 * (size / chroma_div); > > > > q_data->sequence = 0; > > > > @@ -1002,10 +1008,8 @@ static int vicodec_start_streaming(struct > vb2_queue *q, > > state->width = q_data->width; > > state->height = q_data->height; > > state->ref_frame.width = state->ref_frame.height = 0; > > - state->ref_frame.luma = kvmalloc(size + 2 * size / chroma_div, > > - GFP_KERNEL); > > - ctx->comp_max_size = size + 2 * size / chroma_div + > > - sizeof(struct fwht_cframe_hdr); > > + 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); > > if (!state->ref_frame.luma || !state->compressed_frame) { > > kvfree(state->ref_frame.luma); > > @@ -1013,8 +1017,13 @@ static int vicodec_start_streaming(struct > vb2_queue *q, > > vicodec_return_bufs(q, VB2_BUF_STATE_QUEUED); > > return -ENOMEM; > > } > > - state->ref_frame.cb = state->ref_frame.luma + size; > > - state->ref_frame.cr = state->ref_frame.cb + size / chroma_div; > > + if (info->id == V4L2_PIX_FMT_FWHT || info->components_num > 1) { > > >= 3 > > > + state->ref_frame.cb = state->ref_frame.luma + size; > > + state->ref_frame.cr = state->ref_frame.cb + size / > chroma_div; > > + } else { > > + state->ref_frame.cb = NULL; > > + state->ref_frame.cr = NULL; > > + } > > ctx->last_src_buf = NULL; > > ctx->last_dst_buf = NULL; > > state->gop_cnt = 0; > > > > Regards, > > Hans >