| Message ID | 1381362589-32237-4-git-send-email-sheu@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi John, Thanks for the patches! It's nice to see fixes/improvements like this being upstreamed. Unfortunately I have to NACK this patch, but fortunately it is not difficult to fix. The main problem is that you use the wrong API: you need to use G/S_SELECTION instead of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your reaction is 'Huh?', then you're not alone. Which is why the selection API was added. The selection API can crop and compose for both capture and output nodes, and it does what you expect. You need to implement TGT_CROP, TGT_CROP_DEFAULT and TGT_CROP_BOUNDS. The latter two are read-only and just return the current format's width and height. Applications that today use S_CROP to set the crop rectangle for this driver's output need to be adapted to using the S_SELECTION API since S_CROP really doesn't do what they think it does. Regards, Hans On 10/10/2013 01:49 AM, John Sheu wrote: > Allow userspace to set the crop rect of the input image buffer to > encode. > > Signed-off-by: John Sheu <sheu@google.com> > --- > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 6 ++- > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 7 ++-- > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 54 ++++++++++++++++++++++++- > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 +++++--- > 4 files changed, 70 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > index 6920b54..48f706f 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > @@ -428,8 +428,10 @@ struct s5p_mfc_vp8_enc_params { > * struct s5p_mfc_enc_params - general encoding parameters > */ > struct s5p_mfc_enc_params { > - u16 width; > - u16 height; > + u16 crop_left_offset; > + u16 crop_right_offset; > + u16 crop_top_offset; > + u16 crop_bottom_offset; > > u16 gop_size; > enum v4l2_mpeg_video_multi_slice_mode slice_mode; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > index 8faf969..e99bcb8 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > @@ -334,10 +334,9 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f) > ctx->state >= MFCINST_HEAD_PARSED && > ctx->state < MFCINST_ABORT) { > /* This is run on CAPTURE (decode output) */ > - /* Width and height are set to the dimensions > - of the movie, the buffer is bigger and > - further processing stages should crop to this > - rectangle. */ > + /* Width and height are set to the dimensions of the buffer, > + The movie's dimensions may be smaller; the cropping rectangle > + required should be queried with VIDIOC_G_CROP. */ > pix_mp->width = ctx->buf_width; > pix_mp->height = ctx->buf_height; > pix_mp->field = V4L2_FIELD_NONE; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > index 8b24829..4ad9349 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > @@ -1599,7 +1599,57 @@ static int vidioc_g_parm(struct file *file, void *priv, > a->parm.output.timeperframe.numerator = > ctx->enc_params.rc_framerate_denom; > } else { > - mfc_err("Setting FPS is only possible for the output queue\n"); > + mfc_err("Getting FPS is only possible for the output queue\n"); > + return -EINVAL; > + } > + return 0; > +} > + > +static int vidioc_g_crop(struct file *file, void *priv, struct v4l2_crop *a) > +{ > + struct s5p_mfc_ctx *ctx = fh_to_ctx(priv); > + struct s5p_mfc_enc_params *p = &ctx->enc_params; > + > + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { > + a->c.left = p->crop_left_offset; > + a->c.top = p->crop_top_offset; > + a->c.width = ctx->img_width - > + (p->crop_left_offset + p->crop_right_offset); > + a->c.height = ctx->img_height - > + (p->crop_top_offset + p->crop_bottom_offset); > + } else { > + mfc_err("Getting crop is only possible for the output queue\n"); > + return -EINVAL; > + } > + return 0; > +} > + > +static int vidioc_s_crop(struct file *file, void *priv, > + const struct v4l2_crop *a) > +{ > + struct s5p_mfc_ctx *ctx = fh_to_ctx(priv); > + struct s5p_mfc_enc_params *p = &ctx->enc_params; > + > + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { > + int left, right, top, bottom; > + left = round_down(a->c.left, 16); > + right = ctx->img_width - (left + a->c.width); > + top = round_down(a->c.top, 16); > + bottom = ctx->img_height - (top + a->c.height); > + if (left > ctx->img_width) > + left = ctx->img_width; > + if (right < 0) > + right = 0; > + if (top > ctx->img_height) > + top = ctx->img_height; > + if (bottom < 0) > + bottom = 0; > + p->crop_left_offset = left; > + p->crop_right_offset = right; > + p->crop_top_offset = top; > + p->crop_bottom_offset = bottom; > + } else { > + mfc_err("Setting crop is only possible for the output queue\n"); > return -EINVAL; > } > return 0; > @@ -1679,6 +1729,8 @@ static const struct v4l2_ioctl_ops s5p_mfc_enc_ioctl_ops = { > .vidioc_streamoff = vidioc_streamoff, > .vidioc_s_parm = vidioc_s_parm, > .vidioc_g_parm = vidioc_g_parm, > + .vidioc_g_crop = vidioc_g_crop, > + .vidioc_s_crop = vidioc_s_crop, > .vidioc_encoder_cmd = vidioc_encoder_cmd, > .vidioc_subscribe_event = vidioc_subscribe_event, > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > index 5bf6efd..1bb487c 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > @@ -600,12 +600,16 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx) > /* height */ > WRITEL(ctx->img_height, S5P_FIMV_E_FRAME_HEIGHT_V6); /* 16 align */ > > - /* cropped width */ > - WRITEL(ctx->img_width, S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6); > - /* cropped height */ > - WRITEL(ctx->img_height, S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6); > - /* cropped offset */ > - WRITEL(0x0, S5P_FIMV_E_FRAME_CROP_OFFSET_V6); > + /* cropped width, pixels */ > + WRITEL(ctx->img_width - (p->crop_left_offset + p->crop_right_offset), > + S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6); > + /* cropped height, pixels */ > + WRITEL(ctx->img_height - (p->crop_top_offset + p->crop_bottom_offset), > + S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6); > + /* cropped offset, macroblocks */ > + WRITEL(((p->crop_left_offset / 16) & 0x2FF) | > + (((p->crop_top_offset / 16) & 0x2FF) << 10), > + S5P_FIMV_E_FRAME_CROP_OFFSET_V6); > > /* pictype : IDR period */ > reg = 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > The main problem is that you use the wrong API: you need to use G/S_SELECTION instead > of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your > reaction is 'Huh?', then you're not alone. Which is why the selection API was added. > > The selection API can crop and compose for both capture and output nodes, and it > does what you expect. Happy to fix up the patch. I'll just need some clarification on the terminology here. So, as I understand it: (I'll use "source"/"sink" to refer to the device's inputs/outputs, since "output" collides with the V4L2 concept of an OUTPUT device or OUTPUT queue). In all cases, the crop boundary refers to the area in the source image; for a CAPTURE device, this is the (presumably analog) sensor, and for an OUTPUT device, this is the memory buffer. My particular case is a memory-to-memory device, with both CAPTURE and OUTPUT queues. In this case, {G,S}_CROP on either the CAPTURE or OUTPUT queues should effect exactly the same operation: cropping on the source image, i.e. whatever image buffer I'm providing to the OUTPUT queue. The addition of {G,S}_SELECTION is to allow this same operation, except on the sink side this time. So, {G,S}_SELECTION setting the compose bounds on either the CAPTURE or OUTPUT queues should also effect exactly the same operation; cropping on the sink image, i.e. whatever memory buffer I'm providing to the CAPTURE queue. Not sure what you mean by "S_CROP on an output video node doesn't crop, it composes", though. Thanks, -John Sheu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/2013 01:48 AM, John Sheu wrote: > On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> The main problem is that you use the wrong API: you need to use G/S_SELECTION instead >> of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your >> reaction is 'Huh?', then you're not alone. Which is why the selection API was added. >> >> The selection API can crop and compose for both capture and output nodes, and it >> does what you expect. > > > Happy to fix up the patch. I'll just need some clarification on the > terminology here. So, as I understand it: > > (I'll use "source"/"sink" to refer to the device's inputs/outputs, > since "output" collides with the V4L2 concept of an OUTPUT device or > OUTPUT queue). > > In all cases, the crop boundary refers to the area in the source > image; for a CAPTURE device, this is the (presumably analog) sensor, Correct. > and for an OUTPUT device, this is the memory buffer. Correct. > My particular > case is a memory-to-memory device, with both CAPTURE and OUTPUT > queues. In this case, {G,S}_CROP on either the CAPTURE or OUTPUT > queues should effect exactly the same operation: cropping on the > source image, i.e. whatever image buffer I'm providing to the OUTPUT > queue. Incorrect. S_CROP on an OUTPUT queue does the inverse: it refers to the area in the sink image. When the S_CROP API was designed a long time ago output devices were very rare. Those that existed would output video to a S-Video or Composite connector and often could scale the source image down to a particular rectangle on the screen with the area outside of that rectangle either having a fixed color or being a graphical picture. The rectangle for the video on the output image was set by S_CROP, thus effectively composing instead of cropping. As the spec says (http://hverkuil.home.xs4all.nl/spec/media.html#crop): "On a video output device the source are the images passed in by the application, and their size is again negotiated with the VIDIOC_G/S_FMT ioctls, or may be encoded in a compressed video stream. The target is the video signal, and the cropping ioctls determine the area where the images are inserted." This was always confusing, but since there were so few output drivers it was never that important. But when we started seeing many more output and m2m drivers this became a real issue, in particular since S_CROP did only half the operation (cropping for capture, composing for output) and was missing composing for capture and "real" cropping for output. > > The addition of {G,S}_SELECTION is to allow this same operation, > except on the sink side this time. No, it adds the compose operation for capture and the crop operation for output, and it uses the terms 'cropping' and 'composing' correctly without the inversion that S_CROP introduced on the output side. > So, {G,S}_SELECTION setting the > compose bounds on either the CAPTURE or OUTPUT queues should also > effect exactly the same operation; cropping on the sink image, i.e. > whatever memory buffer I'm providing to the CAPTURE queue. > > Not sure what you mean by "S_CROP on an output video node doesn't > crop, it composes", though. I hope I explained it better this time. Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP). S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE). Highly counter-intuitive, blame the original designers of the V4L2 API. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I thought you were not making sense for a bit. Then I walked away, came back, and I think you're making sense now. So: * Crop always refers to the source image * Compose always refers to the destination image On Sat, Oct 12, 2013 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 10/12/2013 01:48 AM, John Sheu wrote: >> On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> In all cases, the crop boundary refers to the area in the source >> image; for a CAPTURE device, this is the (presumably analog) sensor, > > Correct. > >> and for an OUTPUT device, this is the memory buffer. > > Correct. Here you are referring to the crop boundary, which is _not_ always what {G,S}_CROP refers to. (Confusing part). {G,S}_CROP refers to the crop boundary only for a CAPTURE queue. >> My particular >> case is a memory-to-memory device, with both CAPTURE and OUTPUT >> queues. In this case, {G,S}_CROP on either the CAPTURE or OUTPUT >> queues should effect exactly the same operation: cropping on the >> source image, i.e. whatever image buffer I'm providing to the OUTPUT >> queue. > > Incorrect. > > S_CROP on an OUTPUT queue does the inverse: it refers to the area in > the sink image. This confused me for a bit (seeming contradiction with the above), until I realized that you're referring to the S_CROP ioctl here, which is _not_ the "crop boundary"; on an OUTPUT queue it refers to the compose boundary. > No, it adds the compose operation for capture and the crop operation for > output, and it uses the terms 'cropping' and 'composing' correctly > without the inversion that S_CROP introduced on the output side. > > Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP). > S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE). So yes. By adding the {G,S}_SELECTION ioctls we can now refer to the compose boundary for CAPTURE, and crop boundary for OUTPUT. Now, here's a question. It seems that for a mem2mem device, since {G,S}_CROP on the CAPTURE queue covers the crop boundary, and {G,S}_CROP on the OUTPUT queue capture the compose boundary, is there any missing functionality that {G,S}_SELECTION is covering here. In other words: for a mem2mem device, the crop and compose boundaries should be identical for the CAPTURE and OUTPUT queues? -John Sheu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello John, I am a designer of original selection API. Maybe I could clarify what does what in VIDIOC_S_CROP ioctl. > I thought you were not making sense for a bit. Then I walked away, > came back, and I think you're making sense now. So: > > * Crop always refers to the source image > * Compose always refers to the destination image > Not exactly. Look below. There are three basic cases in V4L2 that deal with crop/compose features. CASE 1. Capture devices like sensors There is only queue, the capture queue. The only valid buffer types are V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE. OLD API: The operation ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } ) selects an area on a sensor array that is processed by a device. The memory buffers is filled totally with data produced by the device. It is NOT possible to partially fill a buffer. In selection API the old API call: ioctl(capture, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE }) becomes ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, .target = V4L2_SEL_TGT_CROP }) Setting a area in the memory buffer (not supported in old API) is done by: ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, .target = V4L2_SEL_TGT_COMPOSE }) CASE 2. Output devices like TV encoders. There is only output queue. The only valid buffer types are V4L2_BUF_TYPE_VIDEO_OUTPUT or V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE. OLD API: The operation ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } ) selects an area on a display where buffer's content is inserted. So technically VIDIOC_S_CROP is used to configure composing. It was a quick and effective hack to utilize good old VIDIOC_S_CROP for a new type of device in V4L2, the output device. It is NOT possible to chose which part of a memory buffer is going to be displayed. One has to insert the whole buffer. With selection API old API call becomes ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT, .target = V4L2_SEL_TGT_COMPOSE }) Setting a area in the memory buffer (not supported in old API) is done by: ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT, .target = V4L2_SEL_TGT_CROP }) CASE 3. The memory-to-memory devices There are two queues capture and output. The only valid buffer types are V4L2_BUF_TYPE_VIDEO_OUTPUT or V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE or V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE. OLD API: The operation ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } ) selects an area in a destination buffer that is filled by a device. Notice that it is something completely different from definition of VIDIOC_S_CROP on only-capture devices. In case of M2M, crop means composing actually. The operation ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } ) selects an area in source buffer that is processed by hardware. So it is actually cropping contradictory to what VIDIOC_S_CROP does on only-output devices. All describe non-consistencies seams to a result of some misunderstanding that happened in early days of m2m API. Currently, there are applications use M2M devices and assume the mentioned behavior. I am afraid that it is now an unintentional part V4L2 API :(. That is why I strongly recommend to migrate to selection API. In selection API ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } ) becomes ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT, .target = V4L2_SEL_TGT_CROP }) Moreover, the old API call ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } ) becomes ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, .target = V4L2_SEL_TGT_COMPOSE }) So in all cases cropping is cropping, composing is composing. The other advantage of selection API are constraint flags that can be used co control for policy of rounding rectangle's coordinates. Moreover, the selection API provides additional methods to extract bounds for rectangles, default areas, and padding areas. It is important to know that currently the behavior of two operation: ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, .target = V4L2_SEL_TGT_CROP }) ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT, .target = V4L2_SEL_TGT_COMPOSE }) is still undefined and application should not use them. I hope you find this information useful. Regards, Tomasz Stanislawski On 10/12/2013 11:08 AM, John Sheu wrote: > On Sat, Oct 12, 2013 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 10/12/2013 01:48 AM, John Sheu wrote: >>> On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> In all cases, the crop boundary refers to the area in the source >>> image; for a CAPTURE device, this is the (presumably analog) sensor, >> >> Correct. >> >>> and for an OUTPUT device, this is the memory buffer. >> >> Correct. > > Here you are referring to the crop boundary, which is _not_ always > what {G,S}_CROP refers to. (Confusing part). {G,S}_CROP refers to > the crop boundary only for a CAPTURE queue. > >>> My particular >>> case is a memory-to-memory device, with both CAPTURE and OUTPUT >>> queues. In this case, {G,S}_CROP on either the CAPTURE or OUTPUT >>> queues should effect exactly the same operation: cropping on the >>> source image, i.e. whatever image buffer I'm providing to the OUTPUT >>> queue. >> >> Incorrect. >> >> S_CROP on an OUTPUT queue does the inverse: it refers to the area in >> the sink image. > > This confused me for a bit (seeming contradiction with the above), > until I realized that you're referring to the S_CROP ioctl here, which > is _not_ the "crop boundary"; on an OUTPUT queue it refers to the > compose boundary. > >> No, it adds the compose operation for capture and the crop operation for >> output, and it uses the terms 'cropping' and 'composing' correctly >> without the inversion that S_CROP introduced on the output side. >> >> Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP). >> S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE). > > So yes. By adding the {G,S}_SELECTION ioctls we can now refer to the > compose boundary for CAPTURE, and crop boundary for OUTPUT. > > > Now, here's a question. It seems that for a mem2mem device, since > {G,S}_CROP on the CAPTURE queue covers the crop boundary, and > {G,S}_CROP on the OUTPUT queue capture the compose boundary, is there > any missing functionality that {G,S}_SELECTION is covering here. In > other words: for a mem2mem device, the crop and compose boundaries > should be identical for the CAPTURE and OUTPUT queues? > > -John Sheu > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 17, 2013 at 8:27 AM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > > Hello John, > > I am a designer of original selection API. Maybe I could clarify what > does what in VIDIOC_S_CROP ioctl. > > <snip> > Sweet. Thanks for spelling things out explicitly like this. The fact that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" when used in a m2m device is definitely all sorts of confusing. I'll have an updated patch shortly. -John Sheu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 17, 2013 at 2:46 PM, John Sheu <sheu@google.com> wrote: > Sweet. Thanks for spelling things out explicitly like this. The fact > that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" > when used in a m2m device is definitely all sorts of confusing. Just to double-check: this means that we have another bug. In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, we "simulate" a G_CROP or S_CROP, if the entry point is not defined for that device, by doing the appropriate S_SELECTION or G_SELECTION. Unfortunately then, for M2M this is incorrect then. Am I reading this right? -John Sheu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/18/2013 12:25 AM, John Sheu wrote: > On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com> wrote: >> > Sweet. Thanks for spelling things out explicitly like this. The fact >> > that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >> > when used in a m2m device is definitely all sorts of confusing. > > Just to double-check: this means that we have another bug. > > In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, > we "simulate" a G_CROP or S_CROP, if the entry point is not defined > for that device, by doing the appropriate S_SELECTION or G_SELECTION. > Unfortunately then, for M2M this is incorrect then. > > Am I reading this right? You are right, John. Firstly a clear specification needs to be written, something along the lines of Tomasz's explanation in this thread, once all agree to that the ioctl code should be corrected if needed. It seems this [1] RFC is an answer exactly to your question. Exact meaning of the selection ioctl is only part of the problem, also interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. [1] http://www.spinics.net/lists/linux-media/msg56078.html Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > On 10/18/2013 12:25 AM, John Sheu wrote: >> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com> wrote: >>> > Sweet. Thanks for spelling things out explicitly like this. The fact >>> > that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>> > when used in a m2m device is definitely all sorts of confusing. >> >> Just to double-check: this means that we have another bug. >> >> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >> Unfortunately then, for M2M this is incorrect then. >> >> Am I reading this right? > > You are right, John. Firstly a clear specification needs to be written, > something along the lines of Tomasz's explanation in this thread, once > all agree to that the ioctl code should be corrected if needed. > > It seems this [1] RFC is an answer exactly to your question. > > Exact meaning of the selection ioctl is only part of the problem, also > interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. > > [1] http://www.spinics.net/lists/linux-media/msg56078.html I think the "inversion" behavior is confusing and we should remove it if at all possible. I took a look through all the drivers in linux-media which implement S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of those that aren't: * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. No functional difference. * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver with both OUTPUT and CAPTURE queues. It has uninverted behavior: S_CROP(CAPTURE) -> source S_CROP(OUTPUT) -> destination * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both OUTPUT and CAPTURE queues. It has inverted behavior: S_CROP(CAPTURE) -> destination S_CROP(OUTPUT) -> source The last two points above are the most relevant. So we already have at least one broken driver, regardless of whether we allow inversion or not; I'd think this grants us a certain freedom to redefine the specification to be more logical. Can we do this please? -John Sheu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi John, On 10/18/2013 02:03 AM, John Sheu wrote: > On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki > <sylvester.nawrocki@gmail.com> wrote: >> On 10/18/2013 12:25 AM, John Sheu wrote: >>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com> wrote: >>>>> Sweet. Thanks for spelling things out explicitly like this. The fact >>>>> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>>>> when used in a m2m device is definitely all sorts of confusing. >>> >>> Just to double-check: this means that we have another bug. >>> >>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >>> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >>> Unfortunately then, for M2M this is incorrect then. >>> >>> Am I reading this right? >> >> You are right, John. Firstly a clear specification needs to be written, >> something along the lines of Tomasz's explanation in this thread, once >> all agree to that the ioctl code should be corrected if needed. I don't understand the problem here. The specification has always been clear: s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). Drivers should only implement the selection API and the v4l2 core will do the correct translation of s_crop. Yes, I know it's weird, but that's the way the crop API was defined way back and that's what should be used. My advise: forget about s_crop and just implement s_selection. >> >> It seems this [1] RFC is an answer exactly to your question. >> >> Exact meaning of the selection ioctl is only part of the problem, also >> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >> >> [1] http://www.spinics.net/lists/linux-media/msg56078.html > > I think the "inversion" behavior is confusing and we should remove it > if at all possible. > > I took a look through all the drivers in linux-media which implement > S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of > those that aren't: > > * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both > OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. > No functional difference. Yeah, I guess that's a driver bug. This is a very old driver that originally used a custom API for these things, and since no selection API existed at the time it was just mapped to the crop API. Eventually it should use the selection API as well and do it correctly. But to be honest, nobody cares about this driver :-) It is however on my TODO list of drivers that need to be converted to the latest frameworks, so I might fix this eventually. > * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify > the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. Yes, that's a driver bug. It should check the buffer type. > * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver > with both OUTPUT and CAPTURE queues. It has uninverted behavior: > S_CROP(CAPTURE) -> source > S_CROP(OUTPUT) -> destination This is the wrong behavior. > * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both > OUTPUT and CAPTURE queues. It has inverted behavior: > S_CROP(CAPTURE) -> destination > S_CROP(OUTPUT) -> source This is the correct behavior. > > The last two points above are the most relevant. So we already have > at least one broken driver, regardless of whether we allow inversion > or not; I'd think this grants us a certain freedom to redefine the > specification to be more logical. Can we do this please? No. The fimc-m2m.c driver needs to be fixed. That's the broken one. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry, I missed to reply to this e-mail. On 04/11/13 11:57, Hans Verkuil wrote: > Hi John, > > On 10/18/2013 02:03 AM, John Sheu wrote: >> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki >> <sylvester.nawrocki@gmail.com> wrote: >>> On 10/18/2013 12:25 AM, John Sheu wrote: >>>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com> wrote: >>>>>> Sweet. Thanks for spelling things out explicitly like this. The fact >>>>>> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>>>>> when used in a m2m device is definitely all sorts of confusing. >>>> >>>> Just to double-check: this means that we have another bug. >>>> >>>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >>>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >>>> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >>>> Unfortunately then, for M2M this is incorrect then. >>>> >>>> Am I reading this right? >>> >>> You are right, John. Firstly a clear specification needs to be written, >>> something along the lines of Tomasz's explanation in this thread, once >>> all agree to that the ioctl code should be corrected if needed. > > I don't understand the problem here. The specification has always been clear: > s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). > > Drivers should only implement the selection API and the v4l2 core will do the > correct translation of s_crop. > > Yes, I know it's weird, but that's the way the crop API was defined way back > and that's what should be used. > > My advise: forget about s_crop and just implement s_selection. > >>> >>> It seems this [1] RFC is an answer exactly to your question. >>> >>> Exact meaning of the selection ioctl is only part of the problem, also >>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >>> >>> [1] http://www.spinics.net/lists/linux-media/msg56078.html >> >> I think the "inversion" behavior is confusing and we should remove it >> if at all possible. >> >> I took a look through all the drivers in linux-media which implement >> S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of >> those that aren't: >> >> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both >> OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. >> No functional difference. > > Yeah, I guess that's a driver bug. This is a very old driver that originally > used a custom API for these things, and since no selection API existed at the > time it was just mapped to the crop API. Eventually it should use the selection > API as well and do it correctly. But to be honest, nobody cares about this driver :-) > > It is however on my TODO list of drivers that need to be converted to the latest > frameworks, so I might fix this eventually. > >> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify >> the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. > > Yes, that's a driver bug. It should check the buffer type. > >> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver >> with both OUTPUT and CAPTURE queues. It has uninverted behavior: >> S_CROP(CAPTURE) -> source >> S_CROP(OUTPUT) -> destination No, that's not true. It seems you got it wrong, cropping in case of this m2m driver works like this: S_CROP(OUTPUT) -> source (from HW POV) S_CROP(CAPTURE) -> destination I.e. exactly same way as for s5p-g2d, for which it somehow was a reference implementation. > This is the wrong behavior. > >> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both >> OUTPUT and CAPTURE queues. It has inverted behavior: >> S_CROP(CAPTURE) -> destination >> S_CROP(OUTPUT) -> source > > This is the correct behavior. > >> >> The last two points above are the most relevant. So we already have >> at least one broken driver, regardless of whether we allow inversion >> or not; I'd think this grants us a certain freedom to redefine the >> specification to be more logical. Can we do this please? > > No. The fimc-m2m.c driver needs to be fixed. That's the broken one. It's not broken, it's easy to figure out if you actually look at the code, e.g. fimc_m2m_try_crop() function. -- Regards, Sylwester
On 11/04/2013 12:29 PM, Sylwester Nawrocki wrote: > Sorry, I missed to reply to this e-mail. > > On 04/11/13 11:57, Hans Verkuil wrote: >> Hi John, >> >> On 10/18/2013 02:03 AM, John Sheu wrote: >>> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki >>> <sylvester.nawrocki@gmail.com> wrote: >>>> On 10/18/2013 12:25 AM, John Sheu wrote: >>>>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com> wrote: >>>>>>> Sweet. Thanks for spelling things out explicitly like this. The fact >>>>>>> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>>>>>> when used in a m2m device is definitely all sorts of confusing. >>>>> >>>>> Just to double-check: this means that we have another bug. >>>>> >>>>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >>>>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >>>>> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >>>>> Unfortunately then, for M2M this is incorrect then. >>>>> >>>>> Am I reading this right? >>>> >>>> You are right, John. Firstly a clear specification needs to be written, >>>> something along the lines of Tomasz's explanation in this thread, once >>>> all agree to that the ioctl code should be corrected if needed. >> >> I don't understand the problem here. The specification has always been clear: >> s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). >> >> Drivers should only implement the selection API and the v4l2 core will do the >> correct translation of s_crop. >> >> Yes, I know it's weird, but that's the way the crop API was defined way back >> and that's what should be used. >> >> My advise: forget about s_crop and just implement s_selection. >> >>>> >>>> It seems this [1] RFC is an answer exactly to your question. >>>> >>>> Exact meaning of the selection ioctl is only part of the problem, also >>>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >>>> >>>> [1] http://www.spinics.net/lists/linux-media/msg56078.html >>> >>> I think the "inversion" behavior is confusing and we should remove it >>> if at all possible. >>> >>> I took a look through all the drivers in linux-media which implement >>> S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of >>> those that aren't: >>> >>> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both >>> OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. >>> No functional difference. >> >> Yeah, I guess that's a driver bug. This is a very old driver that originally >> used a custom API for these things, and since no selection API existed at the >> time it was just mapped to the crop API. Eventually it should use the selection >> API as well and do it correctly. But to be honest, nobody cares about this driver :-) >> >> It is however on my TODO list of drivers that need to be converted to the latest >> frameworks, so I might fix this eventually. >> >>> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify >>> the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. >> >> Yes, that's a driver bug. It should check the buffer type. >> >>> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver >>> with both OUTPUT and CAPTURE queues. It has uninverted behavior: >>> S_CROP(CAPTURE) -> source >>> S_CROP(OUTPUT) -> destination > > No, that's not true. It seems you got it wrong, cropping in case of this m2m > driver works like this: > > S_CROP(OUTPUT) -> source (from HW POV) > S_CROP(CAPTURE) -> destination > > I.e. exactly same way as for s5p-g2d, for which it somehow was a reference > implementation. > >> This is the wrong behavior. >> >>> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both >>> OUTPUT and CAPTURE queues. It has inverted behavior: >>> S_CROP(CAPTURE) -> destination >>> S_CROP(OUTPUT) -> source >> >> This is the correct behavior. >> >>> >>> The last two points above are the most relevant. So we already have >>> at least one broken driver, regardless of whether we allow inversion >>> or not; I'd think this grants us a certain freedom to redefine the >>> specification to be more logical. Can we do this please? >> >> No. The fimc-m2m.c driver needs to be fixed. That's the broken one. > > It's not broken, it's easy to figure out if you actually look at the > code, e.g. fimc_m2m_try_crop() function. I did look at the code, but it is quite possible I got confused. Let me be precise as to what should happen, and you can check whether that's what is actually done in the fimc and g2d drivers. For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120, then the DMA engine will only transfer the center 320x240 rectangle to memory. This means that S_FMT needs to provide a buffer size large enough to accomodate a 320x240 image. So: VIDIOC_S_CROP(CAPTURE) == S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP). For V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: Say that the image in memory is a 640x480 picture. If VIDIOC_S_CROP was called for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE with a rectangle of 320x240@160x120 then this would mean that the full 640x480 image is DMAed to the hardware, is scaled down to 320x240 and composed at position (160x120) in a canvas of at least 480x360. In other words, S_CROP behaves as composition for output devices: VIDIOC_S_CROP(OUTPUT) == S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE). The last operation in particular is almost certainly not what you want for m2m devices. Instead, you want to select (crop) part of the image in memory and DMA that to the device. This is S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) and cannot be translated to an S_CROP ioctl. What's more: in order to implement S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE) you would need some way of setting the 'canvas' size of the m2m device, and there is no API today to do this (this was discussed during the v4l/dvb mini-summit). Regarding the capture side of an m2m device: it is not clear to me what these drivers implement: S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP) or S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). If it is the latter, then again S_CROP cannot be used and you have to use S_SELECTION. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On 11/04/2013 01:07 PM, Hans Verkuil wrote: > Let me be precise as to what should happen, and you can check whether that's > what is actually done in the fimc and g2d drivers. > > For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > > Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was > called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120, > then the DMA engine will only transfer the center 320x240 rectangle to memory. > This means that S_FMT needs to provide a buffer size large enough to accomodate > a 320x240 image. > > So: VIDIOC_S_CROP(CAPTURE) == S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP). Unfortunately it's not how it currently works at these drivers. For VIDIOC_S_CROP called with V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE and a rectangle of 320x240@160x120 the hardware would scale and compose full 640x480 image onto 320x240 rectangle in the output memory buffer at position 160x120. IIRC the g2d device cannot scale so it would not allow to select DMA output rectangle smaller than 640x480. But looking at the code it doesn't do any crop parameters validation... So VIDIOC_S_CROP(CAPTURE) is actually being abused on m2m as S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). > For V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > > Say that the image in memory is a 640x480 picture. If VIDIOC_S_CROP was called > for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE with a rectangle of 320x240@160x120 then > this would mean that the full 640x480 image is DMAed to the hardware, is scaled > down to 320x240 and composed at position (160x120) in a canvas of at least 480x360. > > In other words, S_CROP behaves as composition for output devices: > VIDIOC_S_CROP(OUTPUT) == S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE). No, in case of these devices VIDIOC_S_CROP(OUTPUT) does what it actually means - the DMA would read only 320x240 rectangle at position 160x120. > The last operation in particular is almost certainly not what you want for > m2m devices. Instead, you want to select (crop) part of the image in memory and > DMA that to the device. This is S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) and cannot > be translated to an S_CROP ioctl. Yeah, I didn't come yet across a video mem-to-mem hardware that would support steps as in your first description of crop on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE. S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) seems to have been redefined on mem-to-mem devices to do what it actually says. But it's not written anywhere in the spec yet, so I guess we could keep the crop ioctls in those drivers, in order to not break existing user space, and implement the selection API ioctls after documenting its semantics for mem-to-mem devices. > What's more: in order to implement S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE) you > would need some way of setting the 'canvas' size of the m2m device, and there is > no API today to do this (this was discussed during the v4l/dvb mini-summit). > > Regarding the capture side of an m2m device: it is not clear to me what these > drivers implement: S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP) or >S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). > > If it is the latter, then again S_CROP cannot be used and you have to use >S_SELECTION. It's equivalent of S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). Note that the fimc and mfc drivers were written long before the selection API was introduced. Presumably the crop ioctls should just be deprecated (however handlers left for backward compatibility) in those drivers, once there is complete definition of the selections API for the m2m video devices. It's probably worth to avoid adding any translation layer of such behaviour (which doesn't match your definitions above) to the v4l2-core. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h index 6920b54..48f706f 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h @@ -428,8 +428,10 @@ struct s5p_mfc_vp8_enc_params { * struct s5p_mfc_enc_params - general encoding parameters */ struct s5p_mfc_enc_params { - u16 width; - u16 height; + u16 crop_left_offset; + u16 crop_right_offset; + u16 crop_top_offset; + u16 crop_bottom_offset; u16 gop_size; enum v4l2_mpeg_video_multi_slice_mode slice_mode; diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c index 8faf969..e99bcb8 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c @@ -334,10 +334,9 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f) ctx->state >= MFCINST_HEAD_PARSED && ctx->state < MFCINST_ABORT) { /* This is run on CAPTURE (decode output) */ - /* Width and height are set to the dimensions - of the movie, the buffer is bigger and - further processing stages should crop to this - rectangle. */ + /* Width and height are set to the dimensions of the buffer, + The movie's dimensions may be smaller; the cropping rectangle + required should be queried with VIDIOC_G_CROP. */ pix_mp->width = ctx->buf_width; pix_mp->height = ctx->buf_height; pix_mp->field = V4L2_FIELD_NONE; diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c index 8b24829..4ad9349 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c @@ -1599,7 +1599,57 @@ static int vidioc_g_parm(struct file *file, void *priv, a->parm.output.timeperframe.numerator = ctx->enc_params.rc_framerate_denom; } else { - mfc_err("Setting FPS is only possible for the output queue\n"); + mfc_err("Getting FPS is only possible for the output queue\n"); + return -EINVAL; + } + return 0; +} + +static int vidioc_g_crop(struct file *file, void *priv, struct v4l2_crop *a) +{ + struct s5p_mfc_ctx *ctx = fh_to_ctx(priv); + struct s5p_mfc_enc_params *p = &ctx->enc_params; + + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { + a->c.left = p->crop_left_offset; + a->c.top = p->crop_top_offset; + a->c.width = ctx->img_width - + (p->crop_left_offset + p->crop_right_offset); + a->c.height = ctx->img_height - + (p->crop_top_offset + p->crop_bottom_offset); + } else { + mfc_err("Getting crop is only possible for the output queue\n"); + return -EINVAL; + } + return 0; +} + +static int vidioc_s_crop(struct file *file, void *priv, + const struct v4l2_crop *a) +{ + struct s5p_mfc_ctx *ctx = fh_to_ctx(priv); + struct s5p_mfc_enc_params *p = &ctx->enc_params; + + if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { + int left, right, top, bottom; + left = round_down(a->c.left, 16); + right = ctx->img_width - (left + a->c.width); + top = round_down(a->c.top, 16); + bottom = ctx->img_height - (top + a->c.height); + if (left > ctx->img_width) + left = ctx->img_width; + if (right < 0) + right = 0; + if (top > ctx->img_height) + top = ctx->img_height; + if (bottom < 0) + bottom = 0; + p->crop_left_offset = left; + p->crop_right_offset = right; + p->crop_top_offset = top; + p->crop_bottom_offset = bottom; + } else { + mfc_err("Setting crop is only possible for the output queue\n"); return -EINVAL; } return 0; @@ -1679,6 +1729,8 @@ static const struct v4l2_ioctl_ops s5p_mfc_enc_ioctl_ops = { .vidioc_streamoff = vidioc_streamoff, .vidioc_s_parm = vidioc_s_parm, .vidioc_g_parm = vidioc_g_parm, + .vidioc_g_crop = vidioc_g_crop, + .vidioc_s_crop = vidioc_s_crop, .vidioc_encoder_cmd = vidioc_encoder_cmd, .vidioc_subscribe_event = vidioc_subscribe_event, .vidioc_unsubscribe_event = v4l2_event_unsubscribe, diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c index 5bf6efd..1bb487c 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c @@ -600,12 +600,16 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx) /* height */ WRITEL(ctx->img_height, S5P_FIMV_E_FRAME_HEIGHT_V6); /* 16 align */ - /* cropped width */ - WRITEL(ctx->img_width, S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6); - /* cropped height */ - WRITEL(ctx->img_height, S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6); - /* cropped offset */ - WRITEL(0x0, S5P_FIMV_E_FRAME_CROP_OFFSET_V6); + /* cropped width, pixels */ + WRITEL(ctx->img_width - (p->crop_left_offset + p->crop_right_offset), + S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6); + /* cropped height, pixels */ + WRITEL(ctx->img_height - (p->crop_top_offset + p->crop_bottom_offset), + S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6); + /* cropped offset, macroblocks */ + WRITEL(((p->crop_left_offset / 16) & 0x2FF) | + (((p->crop_top_offset / 16) & 0x2FF) << 10), + S5P_FIMV_E_FRAME_CROP_OFFSET_V6); /* pictype : IDR period */ reg = 0;
Allow userspace to set the crop rect of the input image buffer to encode. Signed-off-by: John Sheu <sheu@google.com> --- drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 6 ++- drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 7 ++-- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 54 ++++++++++++++++++++++++- drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 +++++--- 4 files changed, 70 insertions(+), 13 deletions(-)