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.

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

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

Merged
merged 1 commit into from
May 29, 2020

Conversation

davidplowman
Copy link
Contributor

…rmats

The bytes per line numbers calculated by get_bytesperline was not
matching the equivalent calculation being performed by the VideoCore
(mostly by the calculate_pitch function there), resulting in failures
to set the image format with some image width values. This patches up
the RGB24 and YUYV type formats to match the VideoCore calculation.

Signed-off-by: David Plowman david.plowman@raspberrypi.com

@davidplowman
Copy link
Contributor Author

@naushir @6by9
(I think the RGB565 format is OK. On the VideoCore side it's aligned to 16 pixels before doubling, so 32 bytes per line on the V4L2 side looks correct.)

}
else
bpl = (bpl + align_minus_1) & ~align_minus_1;
return bpl;
}
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

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)

…rmats

The bytes per line numbers calculated by get_bytesperline was not
matching the equivalent calculation being performed by the VideoCore
(mostly by the calculate_pitch function there), resulting in failures
to set the image format with some image width values. This patches up
the RGB24 and YUYV type formats to match the VideoCore calculation.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
@davidplowman
Copy link
Contributor Author

New version that hardcodes the special case.

@6by9
Copy link
Contributor

6by9 commented May 29, 2020

I'm happy with that.

@pelwell pelwell merged commit a180c1c into raspberrypi:rpi-5.4.y May 29, 2020
@davidplowman davidplowman deleted the isp_align_fix branch May 30, 2020 11:32
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jun 3, 2020
kernel: vc4: Set driver_name for card
See: raspberrypi/linux#3656

kernel: configs: Add SND_SOC_MAX98357A=m
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=275919

kernel: Add Micro Crystal RV-1805 to i2c-rtc overlays
See: raspberrypi/linux#3651

kernel: media: bcm2835-isp: fix bytes per line calculations for some image formats
See: raspberrypi/linux#3645

kernel: media: bcm2835-unicam: change minimum number of vb2_queue buffers to 1
See: raspberrypi/linux#3638

kernel: media: i2c: imx477: Return correct result on sensor id verification
See: raspberrypi/linux#3630

kernel: Clean up the VCHIQ 2711 DMA support
See: raspberrypi/linux#3629

kernel: overlays: i2c-rtc-gpio: Fix trickle-resistor-ohms param
See: raspberrypi/linux#3642

kernel: Enable hwmon for thermal zones
See: raspberrypi/linux#3307
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jun 3, 2020
kernel: vc4: Set driver_name for card
See: raspberrypi/linux#3656

kernel: configs: Add SND_SOC_MAX98357A=m
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=275919

kernel: Add Micro Crystal RV-1805 to i2c-rtc overlays
See: raspberrypi/linux#3651

kernel: media: bcm2835-isp: fix bytes per line calculations for some image formats
See: raspberrypi/linux#3645

kernel: media: bcm2835-unicam: change minimum number of vb2_queue buffers to 1
See: raspberrypi/linux#3638

kernel: media: i2c: imx477: Return correct result on sensor id verification
See: raspberrypi/linux#3630

kernel: Clean up the VCHIQ 2711 DMA support
See: raspberrypi/linux#3629

kernel: overlays: i2c-rtc-gpio: Fix trickle-resistor-ohms param
See: raspberrypi/linux#3642

kernel: Enable hwmon for thermal zones
See: raspberrypi/linux#3307
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants