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-12 at 09:54 +0800, CK Hu wrote:
> 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.

Sorry, I find that crop left, right, top, bottom all has only 8 bits.
The hardware maximum crop could be 255 pixel, but the actual crop may be larger than 255.
So we still need block-based crop with this pixel-based crop.
Please add this information to describe why we need this mixed crop mechanism.

Regards,
CK

> 
> Regards,
> CK
> 
> 
>