From: Ludovic Desroches <[email protected]> [ Upstream commit 06682206e2a1883354ed758c09efeb51f435adbd ]
Don’t reject the commit when the source rectangle has fractional parts. This can occur due to scaling: drm_atomic_helper_check_plane_state() calls drm_rect_clip_scaled(), which may introduce fractional parts while computing the clipped source rectangle. This does not imply the commit is invalid, so we should accept it instead of discarding it. Signed-off-by: Ludovic Desroches <[email protected]> Reviewed-by: Manikandan Muralidharan <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Manikandan Muralidharan <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: ## Analysis of drm/atmel-hlcdc: don't reject the commit if the src rect has fractional parts ### 1. COMMIT MESSAGE ANALYSIS The commit message clearly describes a bug fix: the driver was incorrectly rejecting valid plane configurations when the source rectangle had fractional (subpixel) parts. The fractional parts are legitimately introduced by `drm_rect_clip_scaled()` during the atomic check process, but the driver was treating them as invalid input and returning `-EINVAL`. Keywords: "don't reject" — this is fixing a false rejection of valid display configurations. ### 2. CODE CHANGE ANALYSIS The change is straightforward and well-contained: **What was happening before:** 1. `hstate->src_x/y/w/h` were assigned the raw 16.16 fixed-point values from `s->src` 2. A check was performed: if any of these values had bits set in the lower 16 bits (fractional part, via `SUBPIXEL_MASK = 0xffff`), the function returned `-EINVAL` 3. Only after passing that check were the values right-shifted by 16 to get integer pixel coordinates **What happens now:** 1. `hstate->src_x/y/w/h` are assigned the values right-shifted by 16 immediately (converting to integer pixels) 2. The subpixel mask check is completely removed 3. The `SUBPIXEL_MASK` macro definition is removed as it's no longer needed **The bug:** `drm_atomic_helper_check_plane_state()` calls `drm_rect_clip_scaled()`, which can introduce fractional parts when computing clipped source rectangles during scaling operations. This is normal DRM behavior — the fractional parts represent subpixel precision in the scaling calculation. The Atmel HLCDC hardware doesn't support subpixel addressing, so the correct behavior is to truncate (>> 16) and use integer coordinates, not reject the entire configuration. **Impact of the bug:** When scaling is involved and clipping produces fractional coordinates, display operations would fail with `-EINVAL`. This means users would see display failures (plane updates rejected) in legitimate scaling scenarios. This is a real user-visible bug — display output fails when it shouldn't. ### 3. CLASSIFICATION This is a **bug fix**. The driver was incorrectly rejecting valid atomic commits, causing display failures during scaling operations. No new features are added, no new APIs introduced. ### 4. SCOPE AND RISK ASSESSMENT - **Lines changed:** ~15 lines removed, ~4 lines modified — very small - **Files touched:** 1 file (`atmel_hlcdc_plane.c`) - **Subsystem:** DRM driver for Atmel HLCDC (embedded ARM display controller) - **Risk:** Very low. The change simply truncates subpixel precision instead of rejecting it. The hardware can only address whole pixels anyway, so truncation is the correct behavior. The integer parts of the coordinates are unchanged. - **Could it break something?** Extremely unlikely. The only behavioral change is accepting configurations that were previously rejected. The pixel coordinates used are identical (same >> 16 operation, just done earlier). ### 5. USER IMPACT This affects users of Atmel/Microchip SAM9/SAMA5 SoC-based embedded systems that use the HLCDC display controller with plane scaling. When scaling is active and clipping occurs, display updates would fail. This is a real-world scenario for embedded display applications. ### 6. STABILITY INDICATORS - **Reviewed-by:** Manikandan Muralidharan (subsystem maintainer) - **Author:** Ludovic Desroches (Microchip engineer, familiar with the hardware) - The fix is logically sound — truncating subpixel coordinates is standard practice in DRM drivers that don't support subpixel precision ### 7. DEPENDENCY CHECK The commit is self-contained. It doesn't depend on any other changes. The code it modifies (`atmel_hlcdc_plane_atomic_check`) has been present in stable trees for a long time. The functions it interacts with (`drm_atomic_helper_check_plane_state`, `drm_rect_width/height`) are standard DRM helpers available in all stable trees. ### Summary This is a small, well-understood bug fix for an incorrect rejection of valid display configurations in the Atmel HLCDC driver. The fix is surgical (removes an overly strict validation check and shifts the coordinate conversion earlier), has been reviewed by the subsystem maintainer, and carries minimal risk of regression. It fixes a real user-visible bug (display failures during scaling with clipping) that affects embedded systems using this display controller. **YES** .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index c0075894dc422..ec1fb5f9549a2 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -79,8 +79,6 @@ drm_plane_state_to_atmel_hlcdc_plane_state(struct drm_plane_state *s) return container_of(s, struct atmel_hlcdc_plane_state, base); } -#define SUBPIXEL_MASK 0xffff - static uint32_t rgb_formats[] = { DRM_FORMAT_C8, DRM_FORMAT_XRGB4444, @@ -745,24 +743,15 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, if (ret || !s->visible) return ret; - hstate->src_x = s->src.x1; - hstate->src_y = s->src.y1; - hstate->src_w = drm_rect_width(&s->src); - hstate->src_h = drm_rect_height(&s->src); + hstate->src_x = s->src.x1 >> 16; + hstate->src_y = s->src.y1 >> 16; + hstate->src_w = drm_rect_width(&s->src) >> 16; + hstate->src_h = drm_rect_height(&s->src) >> 16; hstate->crtc_x = s->dst.x1; hstate->crtc_y = s->dst.y1; hstate->crtc_w = drm_rect_width(&s->dst); hstate->crtc_h = drm_rect_height(&s->dst); - if ((hstate->src_x | hstate->src_y | hstate->src_w | hstate->src_h) & - SUBPIXEL_MASK) - return -EINVAL; - - hstate->src_x >>= 16; - hstate->src_y >>= 16; - hstate->src_w >>= 16; - hstate->src_h >>= 16; - hstate->nplanes = fb->format->num_planes; if (hstate->nplanes > ATMEL_HLCDC_LAYER_MAX_PLANES) return -EINVAL; -- 2.51.0
