Re: [PATCH RFC 3/6] drm/mediatek: ovl: Fix misaligned layer source size on AFBC mode

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Thu, 2026-02-05 at 14:13 -0500, Nícolas F. R. A. Prado wrote:
> On Tue, 2026-02-03 at 02:01 +0000, CK Hu (胡俊光) wrote:
> > On Tue, 2025-12-30 at 11:03 -0300, Nícolas F. R. A. Prado wrote:
> > > From: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> > > 
> > > In AFBC mode, OVL_SRC_SIZE must be block aligned. Due to this
> > > limitation
> > > of the AFBC format, OVL_CLIP needs to be used to achieve the
> > > desired
> > > output size of the layer while still meeting the alignment
> > > constraints.
> > > Failure to do this will result in vblank timeouts and no rendered
> > > output
> > > when the AFBC data source isn't aligned to the AFBC block (32x8).
> > > 
> > > Configure OVL_CLIP so unaligned AFBC layers can be displayed.
> > > 
> > > The following illustrates how the alignment is achieved through the
> > > clip
> > > settings for the horizontal coordinates, the vertical coordinates
> > > are
> > > analogous:
> > > 
> > > /------------------------------------------------\
> > > >                                                |
> > > >            ........................            |
> > > >            ........................            |
> > > >            ........................            |
> > > >            ........................            |
> > > >                                                |
> > > \------------------------------------------------/
> > >      |       |                      |       |
> > >      |       src.x1                 src.x2  |
> > >      |       |                      |       |
> > >      |       |<-------------------->|       |
> > >      |              src_width               |
> > >      |                                      |
> > >      N * AFBC_DATA_BLOCK_WIDTH              M *
> > > AFBC_DATA_BLOCK_WIDTH
> > >      |                                      |
> > >      |<----->|                      |<----->|
> > >       clip_left                      clip_right
> > 
> > As I know, crop is used to drop pixel data.
> > From the name of 'clip_left', I think it would drop the left part of
> > this image.
> > But usually the image is aligned to the left (start from axis 0) and
> > append garbage data in right part.
> > If so, clip_left should be zero and all the clip would be clip_right.
> > This is the normal behavior.
> > If OVL_CROP does behave as this, add comment to describe that
> > clip_left does not drop pixel data.
> 
> Both clip_left and clip_right work in the same way, by discarding that
> many pixels, on the left and right, respectively, of the plane's
> framebuffer when compositing the plane on the final image.
> 
> In the simplest case, when the image to be displayed is left-aligned,
> ie src.x1 = 0, then yes, only clip_right will be used to make sure that
> the plane's width aligns with the AFBC_DATA_BLOCK_WIDTH.
> 
> However if only a sub-region of the image is to be displayed, then
> src.x1 will be non-zero. If that x offset coordinate aligns with the
> AFBC_DATA_BLOCK_WIDTH, then again clip_left will be 0, and we're back
> at the simplest case.
> 
> But if it doesn't align, then clip_left will need to be used to ensure
> only the intended sub-region is displayed, even though it starts in the
> middle of an AFBC data block.
> 
> This is because not only the width and height in DISP_REG_OVL_SRC_SIZE
> need to be aligned to the AFBC data block, but also the starting
> address in DISP_REG_OVL_ADDR, while src.x1 and src.x2 supplied by
> userspace are arbitrary and won't necessarily align, hence we use both
> clips as needed to achieve the intended display outcome respecting the
> hardware constraints.

OK, in mtk_plane_update_new_state(), x_offset_in_blocks has done the block-based crop.
And this patch introduce clip_left for pixel-based crop.
I think pixel-based crop could replace block-based crop.
So drop x_offset_in_blocks and let clip_left = src.x1.

In addition, the clip_right is also not necessary.
The pitch already tell hardware the buffer width.
I think hardware would not access data over src_width,
so drop clip_right and it could be simplified as

|<---------------------------------------------------->|
                    pitch
|<---------------------------------------------->|       
                 valid image data   
             |                      |       
             src.x1                 src.x2  
             |                      |       
|<---------->|                      |
   clip_left |                      |
             |                      |  
|<--------------------------------->|       
                    src_width               


But conceptually, src_width means valid image data, it's better that

|<---------------------------------------------------->|
                    pitch
|<---------------------------------------------->|       
                 valid image data  (fb->width)   |
             |                      |            |
             src.x1                 src.x2       |
             |                      |            |
|<---------->|                      |<---------->|
   clip_left |                      | clip_right |
             |                      |            |
|<---------------------------------------------->|       
                    src_width          

For the old SoC, we just tell hardware buffer start address is in x1 , and src_width is (x2 - x1).
This is because old SoC has no crop function.
When we have crop function. I would like to configure to fit the meaning.

Regards,
CK