Repository navigation
grainmedia: GM8135S, GM8136 and GM8136S machines - #208
Merged
Merged
Conversation
Three machines in hw/arm/grainmedia.c: a Faraday FA626TE (arm926 with the FA626TE's MIDR) among Faraday peripherals, Grain Media's own devices as hw/*/gm-*.c: * gm-intc (FTINTC030), gm-timer (FTTMR010 on the PMU's PCLK), gm-wdt, gm-gpio, gm-i2c (with an OmniVision sensor ID for sensor=ov2715 / ov9715), gm-pmu (chip IDs, strap, PLLs, frequency change interrupt), gm-ddrc, gm-dmac (FTDMAC020, hardware handshake, link lists, abort), gm-spi020 (FTSPI020 with its RX FIFO), gm-otg (FOTG210 host); the FTGMAC100 and FTSDC021 on QEMU's ftgmac100 / generic-sdhci; * a mask ROM stand-in: a camera's NOR dump (flash-file=, written back) boots nsboot in SRAM, the vendor U-Boot and the 3.3 kernel; -kernel boots ATAGs, machine 758; * the video pipeline the vendor graph runs: gm-vcap (VCAP300 link lists, painting the OpenIPC logo), gm-3dnr, gm-scaler, gm-lcd (the graph's frame clock), gm-h264e (MCP280: real I_16x16 slices behind the driver's own headers, CAVLC and CABAC, P slices coding changed macroblocks I_PCM against the reference in the REC buffer), gm-isp (ISP328 frames, the DMA tables and AE / AWB / AF statistics), gm-2d (GM2D command lists: lines, fills, keyed / rotated / scaled blits across 14 formats), gm-mcp100 (MJPEG through the embedded CPU's mailbox, with restart markers); * audio: gm-ssp (FTSSP010 I2S + the ADDA308 codec), capturing a tone or playing to -audiodev; gm-saradc, gm-rtc, gm-pwm, gm-irdet (keys from qom-set), gm-pmonitor, and the FTAES020 on fh-aes (new "advance"). Shared code: hantro-h264.c gains I16 DC / P_Skip / I_PCM slice coders for CAVLC and CABAC; nvt_pixel_jpeg_scan_rst() adds restart markers and separate chroma planes. A Longse GM8136 dump boots from flash to its application, whose RTSP server streams decodable 1080p / D1 H.264 (qemu-boot/test-grainmedia- rtsp.sh) and, with audio enabled in its config, the tone as PCMU. The OpenIPC gm8136 / gm8135 releases boot to a login with a DHCP lease and a JFFS2 overlay (qemu-boot/test-grainmedia-boot.sh, in CI); qemu/tests/test-grainmedia.py checks the devices register by register (in CI), decoding the H.264 and JPEG output with ffmpeg when present.
PR Summary by QodoAdd Grain Media GM8135S, GM8136, and GM8136S camera machines
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
Code Review by Qodo
1. Flash boot loses access to its stack
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+474
to
+475
| create_unimplemented_device("grainmedia.periph", GM_PERIPH_BASE, | ||
| GM_PERIPH_SIZE); |
There was a problem hiding this comment.
1. Flash boot loses access to its stack 🐞 Bug ≡ Correctness
grainmedia_common_init() maps SRAM and then adds an unimplemented peripheral region that covers the same address range. During flash boot, the nsboot image occupies only the first 24 KiB of SRAM, while its stack uses the overlapping range above it.
Agent Prompt
## Issue description
The broad unimplemented peripheral mapping overlaps boot SRAM and its stack.
## Fix Focus Areas
- qemu/hw/arm/grainmedia.c[468-475]
## Recommended Fix
Split the unimplemented peripheral range around SRAM, or give SRAM an explicitly higher-priority mapping, and verify reads and writes throughout SRAM during flash boot.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+369
to
+373
| } else { /* B: nothing changes */ | ||
| if (cabac) { | ||
| n = h264_slice_pskip_cabac(qp, idc, nmb, out, cap); | ||
| } else { | ||
| n = h264_slice_pskip_cavlc(nmb, off, out, cap); |
There was a problem hiding this comment.
5. Cabac b slices decode incorrectly 🐞 Bug ≡ Correctness
gm_h264e_encode_slice() sends B slices to h264_slice_pskip_cabac(), which initializes and emits P-slice skip flags using context 11. When the configured slice uses CABAC and is neither I nor P, its B-slice header and P-slice skip coding disagree.
Agent Prompt
## Issue description
CABAC B slices are encoded with P-slice skip contexts.
## Fix Focus Areas
- qemu/hw/misc/gm-h264e.c[369-373]
- qemu/hw/misc/hantro-h264.c[883-906]
## Recommended Fix
Implement and call a B-slice CABAC skip encoder using the B-slice contexts, or reject unsupported B-slice requests rather than emitting invalid slices.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+383
to
+386
| if (ry && rec_y && rec_y != GM_H264E_NO_REC) { | ||
| dma_memory_write(&address_space_memory, rec_y, ry, (size_t)w * h, | ||
| MEMTXATTRS_UNSPECIFIED); | ||
| dma_memory_write(&address_space_memory, rec_uv, ruv, |
There was a problem hiding this comment.
11. Multi-slice pictures overwrite the reference top 🐞 Bug ≡ Correctness
For P pictures, gm_h264e_encode_slice reads the reference at ME_REF + first*w*16 (luma) and `+ first*w*8 (chroma), but writes the reconstruction to rec_y / rec_uv with no first` offset. In a picture coded as several slices, every slice after the first writes its rows over the top of the REC frame. The next P picture then compares against the wrong reference, so its skip/PCM decisions and the decoded output are wrong.
Agent Prompt
## Issue description
The REC write ignores the slice's first macroblock row, while the ME_REF read honours it.
## Fix Focus Areas
- qemu/hw/misc/gm-h264e.c[383-387]
## Recommended Fix
Write to `rec_y + (hwaddr)first * w * 16` and `rec_uv + (hwaddr)first * w * 8`, matching the ME_REF read offsets.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
* gm-h264e: refuse a source stride narrower than the picture (read past the row buffer); log CABAC B slices as unimplemented; note that the driver gives REC per slice (slice.c), ME_REF per picture. * gm-scaler, gm-2d: pad the source copy so an odd width's last UYVY pair stays inside it. * gm-2d: order reversed rectangle corners before clipping. * gm-gpio: drive a pin low when it turns input, and release every line at reset. * gm-timer: a zero LOAD written while running turns the channel one-shot. * gm-dmac: at most 256 descriptors per channel per bottom half, so a looping chain lets the main loop run. * grainmedia: accept only the DDR sizes the controller can describe (64-512 MiB, powers of two). * test-grainmedia-rtsp.sh: the exit trap no longer trips on an unset QPID under set -u.
Collaborator
Author
|
Qodo findings, addressed in 4808b72:
Re-run: test-grainmedia.py 0 failures, the OpenIPC gm8136 / gm8135 boots, and a camera dump's RTSP stream decodes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
grainmedia: GM8135S, GM8136 and GM8136S machines
Three machines in hw/arm/grainmedia.c: a Faraday FA626TE (arm926 with
the FA626TE's MIDR) among Faraday peripherals, Grain Media's own devices
as hw//gm-.c:
gm-gpio, gm-i2c (with an OmniVision sensor ID for sensor=ov2715 /
ov9715), gm-pmu (chip IDs, strap, PLLs, frequency change interrupt),
gm-ddrc, gm-dmac (FTDMAC020, hardware handshake, link lists, abort),
gm-spi020 (FTSPI020 with its RX FIFO), gm-otg (FOTG210 host); the
FTGMAC100 and FTSDC021 on QEMU's ftgmac100 / generic-sdhci;
boots nsboot in SRAM, the vendor U-Boot and the 3.3 kernel; -kernel
boots ATAGs, machine 758;
painting the OpenIPC logo), gm-3dnr, gm-scaler, gm-lcd (the graph's
frame clock), gm-h264e (MCP280: real I_16x16 slices behind the driver's
own headers, CAVLC and CABAC, P slices coding changed macroblocks I_PCM
against the reference in the REC buffer), gm-isp (ISP328 frames, the
DMA tables and AE / AWB / AF statistics), gm-2d (GM2D command lists:
lines, fills, keyed / rotated / scaled blits across 14 formats),
gm-mcp100 (MJPEG through the embedded CPU's mailbox, with restart
markers);
playing to -audiodev; gm-saradc, gm-rtc, gm-pwm, gm-irdet (keys from
qom-set), gm-pmonitor, and the FTAES020 on fh-aes (new "advance").
Shared code: hantro-h264.c gains I16 DC / P_Skip / I_PCM slice coders for
CAVLC and CABAC; nvt_pixel_jpeg_scan_rst() adds restart markers and
separate chroma planes.
A Longse GM8136 dump boots from flash to its application, whose RTSP
server streams decodable 1080p / D1 H.264 (qemu-boot/test-grainmedia-
rtsp.sh) and, with audio enabled in its config, the tone as PCMU. The
OpenIPC gm8136 / gm8135 releases boot to a login with a DHCP lease and a
JFFS2 overlay (qemu-boot/test-grainmedia-boot.sh, in CI);
qemu/tests/test-grainmedia.py checks the devices register by register
(in CI), decoding the H.264 and JPEG output with ffmpeg when present.