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

media: bcm2835-isp: fix bytes per line calculations for some image fo… #3645

Merged
merged 1 commit into from
May 29, 2020
Merged

media: bcm2835-isp: fix bytes per line calculations for some image fo… #3645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion drivers/staging/vc04_services/bcm2835-isp/bcm2835-v4l2-isp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,11 @@ struct bcm2835_isp_fmt *get_default_format(struct bcm2835_isp_node *node)
static inline unsigned int get_bytesperline(int width,
const struct bcm2835_isp_fmt *fmt)
{
return ALIGN((width * fmt->depth) >> 3, fmt->bytesperline_align);
/* GPU aligns 24bpp images to a multiple of 32 pixels (not bytes). */
if (fmt->depth == 24)
return ALIGN(width, 32) * 3;
else
return ALIGN((width * fmt->depth) >> 3, fmt->bytesperline_align);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure, but I would guess the upstream folks might object to using align_minus_1 variable over an explicit (align - 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Given that I was using it several times I thought I'd make it explicit that I expected the value to be re-used, but if we think (a) the compiler would do it anyway and (b) upstream maintainers might object, then I'll happily change it. Is that the general feeling?

Copy link
Contributor

Choose a reason for hiding this comment

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

They would probably prefer you to use the existing ALIGN and IS_ALIGNED macros.

Copy link
Contributor

@6by9 6by9 May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Personally I don't think we need the complication with manually working out if it's a bitmask or not.
24bpp is the only format that has quirks, and you have a near match with https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c#L994

if (fmt->depth == 24)
   return <whatever the correct align is here, and it doesn't need to be 96>
else
  return ((width * fmt->depth) >> 3, fmt->bytesperline_align);

Admittedly it's not a performance critical path, but manually computing whether we need to do the modulo seems nuts.

MMAL sets the stride via a multiple of width * bpp. 32pixels * 24bpp is 96 bytes, which fulfils the ISP requirement for a multiple of 32 bytes. So I think we can do

  return ((ALIGN(width, fmt->bytesperline_align) * fmt->depth) >> 3);

and leave the bytesperline_align at 32 (or hardcode it here).

(edited - it helps to actually call the ALIGN macro)


static inline unsigned int get_sizeimage(int bpl, int width, int height,
Expand Down
10 changes: 5 additions & 5 deletions drivers/staging/vc04_services/bcm2835-isp/bcm2835_isp_fmts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static const struct bcm2835_isp_fmt supported_formats[] = {
}, {
.fourcc = V4L2_PIX_FMT_YUYV,
.depth = 16,
.bytesperline_align = 32,
.bytesperline_align = 64,
.flags = 0,
.mmal_fmt = MMAL_ENCODING_YUYV,
.size_multiplier_x2 = 2,
Expand All @@ -80,7 +80,7 @@ static const struct bcm2835_isp_fmt supported_formats[] = {
}, {
.fourcc = V4L2_PIX_FMT_UYVY,
.depth = 16,
.bytesperline_align = 32,
.bytesperline_align = 64,
.flags = 0,
.mmal_fmt = MMAL_ENCODING_UYVY,
.size_multiplier_x2 = 2,
Expand All @@ -89,7 +89,7 @@ static const struct bcm2835_isp_fmt supported_formats[] = {
}, {
.fourcc = V4L2_PIX_FMT_YVYU,
.depth = 16,
.bytesperline_align = 32,
.bytesperline_align = 64,
.flags = 0,
.mmal_fmt = MMAL_ENCODING_YVYU,
.size_multiplier_x2 = 2,
Expand All @@ -98,7 +98,7 @@ static const struct bcm2835_isp_fmt supported_formats[] = {
}, {
.fourcc = V4L2_PIX_FMT_VYUY,
.depth = 16,
.bytesperline_align = 32,
.bytesperline_align = 64,
.flags = 0,
.mmal_fmt = MMAL_ENCODING_VYUY,
.size_multiplier_x2 = 2,
Expand Down Expand Up @@ -135,7 +135,7 @@ static const struct bcm2835_isp_fmt supported_formats[] = {
}, {
.fourcc = V4L2_PIX_FMT_ABGR32,
.depth = 32,
.bytesperline_align = 32,
.bytesperline_align = 64,
.flags = 0,
.mmal_fmt = MMAL_ENCODING_BGRA,
.size_multiplier_x2 = 2,
Expand Down