Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bcm2835-codec tweaks #4113

Merged
merged 5 commits into from
Mar 12, 2021
Merged

bcm2835-codec tweaks #4113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea13e57
media/v4l2_m2m: In buffered mode run jobs if either port is streaming
6by9 Feb 1, 2021
ff583bb
staging/bcm2835-codec: Correct logging of size_t to %zu
6by9 Feb 1, 2021
bf6d809
staging/bcm2835-codec: Add support for pixel aspect ratio
6by9 Feb 2, 2021
420578c
staging/bcm2835-codec: Implement additional g_selection calls for decode
6by9 Feb 2, 2021
084084d
staging/bcm2835-codec: Add VC-1 support.
6by9 Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions drivers/media/v4l2-core/v4l2-mem2mem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,10 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,

dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);

if (!m2m_ctx->out_q_ctx.q.streaming
|| !m2m_ctx->cap_q_ctx.q.streaming) {
dprintk("Streaming needs to be on for both queues\n");
if (!(m2m_ctx->out_q_ctx.q.streaming &&
m2m_ctx->cap_q_ctx.q.streaming) &&
!(m2m_ctx->out_q_ctx.buffered && m2m_ctx->out_q_ctx.q.streaming)) {
Copy link
Contributor

@pelwell pelwell Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just spent a minute or two writing out your condition as you described it:

	if (!((m2m_ctx->out_q_ctx.q.streaming &&
	       m2m_ctx->cap_q_ctx.q.streaming) ||
	      (m2m_ctx->out_q_ctx.buffered && m2m_ctx->out_q_ctx.q.streaming))) {

and then using De Morgan's laws to transform it into what you have written. But I shouldn't have to - what's wrong with the literal version above?

Copy link

@blazczak blazczak Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OP's version is just an evolution of pre-existing (and thus assumed stable to some degree through testing/deployment) code using a minimal set of changes.

The check could also be written as

	if (!(m2m_ctx->out_q_ctx.q.streaming && m2m_ctx-cap_q_ctx.q.streaming) && 
	       !(m2m_ctx->out_q_ctx.buffered && m2m_ctx->out_q_ctx.q.streaming)) {

They are all logically equivalent but having OR inside the clause and top-level NOT in the proposed transformed version depending on the conditions could make it a slower performing check if no short-circuiting happens inside and the result is not final until after the negation is performed at the end.

The changes suggested in c54a951 still employ a short-circuit, so they look ok to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They aren't wrong, but in (especially) kernel work clarity trumps pretty much everything, especially when the compiler will likely generate the same code in both cases.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on clarity, thus my suggestion above. Of course, as more conditions get added, the clause may have to be rewritten again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just retaining the format of the original term. I'll rework as requested.

dprintk("Streaming needs to be on for both queues, or buffered and OUTPUT streaming\n");
return;
}

Expand Down
50 changes: 47 additions & 3 deletions drivers/staging/vc04_services/bcm2835-codec/bcm2835-v4l2-codec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,12 @@ static const struct bcm2835_codec_fmt supported_formats[] = {
.depth = 0,
.flags = V4L2_FMT_FLAG_COMPRESSED,
.mmal_fmt = MMAL_ENCODING_VP8,
},
}, {
.fourcc = V4L2_PIX_FMT_VC1_ANNEX_G,
.depth = 0,
.flags = V4L2_FMT_FLAG_COMPRESSED,
.mmal_fmt = MMAL_ENCODING_WVC1,
}
};

struct bcm2835_codec_fmt_list {
Expand All @@ -602,6 +607,7 @@ struct bcm2835_codec_q_data {
unsigned int crop_width;
unsigned int crop_height;
bool selection_set;
struct v4l2_fract aspect_ratio;

unsigned int sizeimage;
unsigned int sequence;
Expand Down Expand Up @@ -981,6 +987,9 @@ static void handle_fmt_changed(struct bcm2835_codec_ctx *ctx,
if (format->es.video.color_space)
color_mmal2v4l(ctx, format->es.video.color_space);

q_data->aspect_ratio.numerator = format->es.video.par.num;
q_data->aspect_ratio.denominator = format->es.video.par.den;

queue_res_chg_event(ctx);
}

Expand Down Expand Up @@ -1513,6 +1522,14 @@ static int vidioc_g_selection(struct file *file, void *priv,
s->r.width = q_data->crop_width;
s->r.height = q_data->crop_height;
break;
case V4L2_SEL_TGT_CROP_BOUNDS:
pelwell marked this conversation as resolved.
Show resolved Hide resolved
case V4L2_SEL_TGT_CROP_DEFAULT:
s->r.left = 0;
s->r.top = 0;
s->r.width = (q_data->bytesperline << 3) /
q_data->fmt->depth;
s->r.height = q_data->height;
break;
default:
return -EINVAL;
}
Expand Down Expand Up @@ -1657,6 +1674,29 @@ static int vidioc_g_parm(struct file *file, void *priv,
return 0;
}

static int vidioc_g_pixelaspect(struct file *file, void *fh, int type,
struct v4l2_fract *f)
{
struct bcm2835_codec_ctx *ctx = file2ctx(file);

/*
* The selection API takes V4L2_BUF_TYPE_VIDEO_CAPTURE and
* V4L2_BUF_TYPE_VIDEO_OUTPUT, even if the device implements the MPLANE
* API. The V4L2 core will have converted the MPLANE variants to
* non-MPLANE.
* Open code this instead of using get_q_data in this case.
*/
if (ctx->dev->role != DECODE)
return -ENOIOCTLCMD;

if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;

*f = ctx->q_data[V4L2_M2M_DST].aspect_ratio;

return 0;
}

static int vidioc_subscribe_evt(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub)
{
Expand Down Expand Up @@ -2082,6 +2122,8 @@ static const struct v4l2_ioctl_ops bcm2835_codec_ioctl_ops = {
.vidioc_g_parm = vidioc_g_parm,
.vidioc_s_parm = vidioc_s_parm,

.vidioc_g_pixelaspect = vidioc_g_pixelaspect,

.vidioc_subscribe_event = vidioc_subscribe_evt,
.vidioc_unsubscribe_event = v4l2_event_unsubscribe,

Expand Down Expand Up @@ -2640,6 +2682,8 @@ static int bcm2835_codec_open(struct file *file)
ctx->q_data[V4L2_M2M_DST].crop_width,
ctx->q_data[V4L2_M2M_DST].height,
ctx->q_data[V4L2_M2M_DST].fmt);
ctx->q_data[V4L2_M2M_DST].aspect_ratio.numerator = 1;
ctx->q_data[V4L2_M2M_DST].aspect_ratio.denominator = 1;

ctx->colorspace = V4L2_COLORSPACE_REC709;
ctx->bitrate = 10 * 1000 * 1000;
Expand Down Expand Up @@ -2837,7 +2881,7 @@ static int bcm2835_codec_get_supported_fmts(struct bcm2835_codec_dev *dev)
if (ret) {
if (ret == MMAL_MSG_STATUS_ENOSPC) {
v4l2_err(&dev->v4l2_dev,
"%s: port has more encodings than we provided space for. Some are dropped (%u vs %u).\n",
"%s: port has more encodings than we provided space for. Some are dropped (%zu vs %u).\n",
__func__, param_size / sizeof(u32),
MAX_SUPPORTED_ENCODINGS);
num_encodings = MAX_SUPPORTED_ENCODINGS;
Expand Down Expand Up @@ -2883,7 +2927,7 @@ static int bcm2835_codec_get_supported_fmts(struct bcm2835_codec_dev *dev)
if (ret) {
if (ret == MMAL_MSG_STATUS_ENOSPC) {
v4l2_err(&dev->v4l2_dev,
"%s: port has more encodings than we provided space for. Some are dropped (%u vs %u).\n",
"%s: port has more encodings than we provided space for. Some are dropped (%zu vs %u).\n",
__func__, param_size / sizeof(u32),
MAX_SUPPORTED_ENCODINGS);
num_encodings = MAX_SUPPORTED_ENCODINGS;
Expand Down