Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-22 Thread Daniel Vetter
On Wed, Jul 21, 2021 at 9:42 PM Souza, Jose  wrote:
> On Wed, 2021-07-21 at 09:56 +0200, Daniel Vetter wrote:
> > On Wed, Jul 21, 2021 at 9:50 AM Daniel Vetter  wrote:
> > > On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose  wrote:
> > > > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > > > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > > > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  
> > > > > > wrote:
> > > > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > Now using plane damage clips property to calcualte the damaged 
> > > > > > > > area.
> > > > > > > > Selective fetch only supports one region to be fetched so 
> > > > > > > > software
> > > > > > > > needs to calculate a bounding box around all damage clips.
> > > > > > > >
> > > > > > > > Now that we are not complete fetching each plane, there is 
> > > > > > > > another
> > > > > > > > loop needed as all the plane areas that intersect with the pipe
> > > > > > > > damaged area needs to be fetched from memory so the complete 
> > > > > > > > blending
> > > > > > > > of all planes can happen.
> > > > > > > >
> > > > > > > > v2:
> > > > > > > > - do not shifting new_plane_state->uapi.dst only src is in 
> > > > > > > > 16.16 format
> > > > > > > >
> > > > > > > > v4:
> > > > > > > > - setting plane selective fetch area using the whole pipe 
> > > > > > > > damage area
> > > > > > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > > > > > changed
> > > > > > > >
> > > > > > > > v5:
> > > > > > > > - taking in consideration src.y1 in the damage coordinates
> > > > > > > > - adding to the pipe damaged area planes that were visible but 
> > > > > > > > are
> > > > > > > > invisible in the new state
> > > > > > > >
> > > > > > > > v6:
> > > > > > > > - consider old state plane coordinates when visibility changes 
> > > > > > > > or it
> > > > > > > > moved to calculate damaged area
> > > > > > > > - remove from damaged area the portion not in src clip
> > > > > > > >
> > > > > > > > v7:
> > > > > > > > - intersec every damage clip with src to minimize damaged area
> > > > > > > >
> > > > > > > > v8:
> > > > > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > > > > framebuffer coordinates that plane will start to fetch from
> > > > > > > >
> > > > > > > > v9:
> > > > > > > > - Only add plane dst or src to damaged_area if visible
> > > > > > > > - Early skip plane damage calculation if it was not visible in 
> > > > > > > > old and
> > > > > > > > new state
> > > > > > > >
> > > > > > > > Cc: Ville Syrjälä 
> > > > > > > > Cc: Gwan-gyeong Mun 
> > > > > > > > Reviewed-by: Gwan-gyeong Mun 
> > > > > > > > Signed-off-by: José Roberto de Souza 
> > > > > > >
> > > > > > > Why is this not using drm_atomic_helper_damage_merged? I just 
> > > > > > > stumbled
> > > > > > > over this, and this is one of the only two drivers that directly 
> > > > > > > digs
> > > > > > > around in the damage area, and seems to reinvent a bunch of the 
> > > > > > > stuff
> > > > > > > here.
> > > >
> > > > We can use drm_atomic_helper_damage_merged() but it would only save us 
> > > > one for loop.
> > >
> > > Yes please. The trouble with rolling our own copies for everything is
> > > that it does add up.
> > >
> > > > > > Also, did we merge the igts for this stuff? They unfortunately never
> > > > > > landed, when vmwgfx team did all this work, but for i915 we really
> > > > > > shouldn't even land new support without tests.
> > > > >
> > > > > Lo and behold, we merge the uapi enabling way earlier than this patch 
> > > > > here:
> > > > >
> > > > > commit 093a3a3926b8bda9eef773e4ed5079053350
> > > > > Author: José Roberto de Souza 
> > > > > Date:   Thu Jun 25 18:01:47 2020 -0700
> > > > >
> > > > >drm/i915: Add plane damage clips property
> > > > >
> > > > > And the igts are nowhere near to be seen, at least the stuff from
> > > > > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > > > > so this gets sorted out asap.
> > > >
> > > > Here the IGT: 
> > > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c
> > >
> > > There was some igts that were cross-driver, not intel specific. The
> > > thing here is supposed to be a cross-vendor interface, so would be
> > > really good if you resurrect those from vmwgfx folks and land them
> > > too:
> > >
> > > https://patchwork.freedesktop.org/series/51087/
> >
> > Also from a very quick look at the kms_psr2_sf tests there's really
> > not a whole lot specific about our psr2 implementation in there. The
> > test should work on any plane with a FB_DAMAGE_CLIPS rect available,
> > so there's no reason to make this specific to intel, much less to
> > psr2. I think we need to:
> > - make this a generic kms test, it should work
> > - merge with the testcases from the folks w

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-21 Thread Souza, Jose
On Wed, 2021-07-21 at 09:56 +0200, Daniel Vetter wrote:
> On Wed, Jul 21, 2021 at 9:50 AM Daniel Vetter  wrote:
> > On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose  wrote:
> > > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > > > >  wrote:
> > > > > > > 
> > > > > > > Now using plane damage clips property to calcualte the damaged 
> > > > > > > area.
> > > > > > > Selective fetch only supports one region to be fetched so software
> > > > > > > needs to calculate a bounding box around all damage clips.
> > > > > > > 
> > > > > > > Now that we are not complete fetching each plane, there is another
> > > > > > > loop needed as all the plane areas that intersect with the pipe
> > > > > > > damaged area needs to be fetched from memory so the complete 
> > > > > > > blending
> > > > > > > of all planes can happen.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 
> > > > > > > format
> > > > > > > 
> > > > > > > v4:
> > > > > > > - setting plane selective fetch area using the whole pipe damage 
> > > > > > > area
> > > > > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > > > > changed
> > > > > > > 
> > > > > > > v5:
> > > > > > > - taking in consideration src.y1 in the damage coordinates
> > > > > > > - adding to the pipe damaged area planes that were visible but are
> > > > > > > invisible in the new state
> > > > > > > 
> > > > > > > v6:
> > > > > > > - consider old state plane coordinates when visibility changes or 
> > > > > > > it
> > > > > > > moved to calculate damaged area
> > > > > > > - remove from damaged area the portion not in src clip
> > > > > > > 
> > > > > > > v7:
> > > > > > > - intersec every damage clip with src to minimize damaged area
> > > > > > > 
> > > > > > > v8:
> > > > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > > > framebuffer coordinates that plane will start to fetch from
> > > > > > > 
> > > > > > > v9:
> > > > > > > - Only add plane dst or src to damaged_area if visible
> > > > > > > - Early skip plane damage calculation if it was not visible in 
> > > > > > > old and
> > > > > > > new state
> > > > > > > 
> > > > > > > Cc: Ville Syrjälä 
> > > > > > > Cc: Gwan-gyeong Mun 
> > > > > > > Reviewed-by: Gwan-gyeong Mun 
> > > > > > > Signed-off-by: José Roberto de Souza 
> > > > > > 
> > > > > > Why is this not using drm_atomic_helper_damage_merged? I just 
> > > > > > stumbled
> > > > > > over this, and this is one of the only two drivers that directly 
> > > > > > digs
> > > > > > around in the damage area, and seems to reinvent a bunch of the 
> > > > > > stuff
> > > > > > here.
> > > 
> > > We can use drm_atomic_helper_damage_merged() but it would only save us 
> > > one for loop.
> > 
> > Yes please. The trouble with rolling our own copies for everything is
> > that it does add up.
> > 
> > > > > Also, did we merge the igts for this stuff? They unfortunately never
> > > > > landed, when vmwgfx team did all this work, but for i915 we really
> > > > > shouldn't even land new support without tests.
> > > > 
> > > > Lo and behold, we merge the uapi enabling way earlier than this patch 
> > > > here:
> > > > 
> > > > commit 093a3a3926b8bda9eef773e4ed5079053350
> > > > Author: José Roberto de Souza 
> > > > Date:   Thu Jun 25 18:01:47 2020 -0700
> > > > 
> > > >drm/i915: Add plane damage clips property
> > > > 
> > > > And the igts are nowhere near to be seen, at least the stuff from
> > > > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > > > so this gets sorted out asap.
> > > 
> > > Here the IGT: 
> > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c
> > 
> > There was some igts that were cross-driver, not intel specific. The
> > thing here is supposed to be a cross-vendor interface, so would be
> > really good if you resurrect those from vmwgfx folks and land them
> > too:
> > 
> > https://patchwork.freedesktop.org/series/51087/
> 
> Also from a very quick look at the kms_psr2_sf tests there's really
> not a whole lot specific about our psr2 implementation in there. The
> test should work on any plane with a FB_DAMAGE_CLIPS rect available,
> so there's no reason to make this specific to intel, much less to
> psr2. I think we need to:
> - make this a generic kms test, it should work
> - merge with the testcases from the folks who merged FB_DAMAGE_CLIPS
> originally to make sure all drivers follow the same contract.
> 
> KMS properties are generic, intel-specific tests for when there's
> nothing intel-specific isn't good. Also adding Pankaj.

For Intel the only use that we have for damage clips is PSR2 selective fetch, 
we needed th

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-21 Thread Daniel Vetter
On Wed, Jul 21, 2021 at 9:56 AM Daniel Vetter  wrote:
> On Wed, Jul 21, 2021 at 9:50 AM Daniel Vetter  wrote:
> > On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose  wrote:
> > > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > > > >  wrote:
> > > > > > >
> > > > > > > Now using plane damage clips property to calcualte the damaged 
> > > > > > > area.
> > > > > > > Selective fetch only supports one region to be fetched so software
> > > > > > > needs to calculate a bounding box around all damage clips.
> > > > > > >
> > > > > > > Now that we are not complete fetching each plane, there is another
> > > > > > > loop needed as all the plane areas that intersect with the pipe
> > > > > > > damaged area needs to be fetched from memory so the complete 
> > > > > > > blending
> > > > > > > of all planes can happen.
> > > > > > >
> > > > > > > v2:
> > > > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 
> > > > > > > format
> > > > > > >
> > > > > > > v4:
> > > > > > > - setting plane selective fetch area using the whole pipe damage 
> > > > > > > area
> > > > > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > > > > changed
> > > > > > >
> > > > > > > v5:
> > > > > > > - taking in consideration src.y1 in the damage coordinates
> > > > > > > - adding to the pipe damaged area planes that were visible but are
> > > > > > > invisible in the new state
> > > > > > >
> > > > > > > v6:
> > > > > > > - consider old state plane coordinates when visibility changes or 
> > > > > > > it
> > > > > > > moved to calculate damaged area
> > > > > > > - remove from damaged area the portion not in src clip
> > > > > > >
> > > > > > > v7:
> > > > > > > - intersec every damage clip with src to minimize damaged area
> > > > > > >
> > > > > > > v8:
> > > > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > > > framebuffer coordinates that plane will start to fetch from
> > > > > > >
> > > > > > > v9:
> > > > > > > - Only add plane dst or src to damaged_area if visible
> > > > > > > - Early skip plane damage calculation if it was not visible in 
> > > > > > > old and
> > > > > > > new state
> > > > > > >
> > > > > > > Cc: Ville Syrjälä 
> > > > > > > Cc: Gwan-gyeong Mun 
> > > > > > > Reviewed-by: Gwan-gyeong Mun 
> > > > > > > Signed-off-by: José Roberto de Souza 
> > > > > >
> > > > > > Why is this not using drm_atomic_helper_damage_merged? I just 
> > > > > > stumbled
> > > > > > over this, and this is one of the only two drivers that directly 
> > > > > > digs
> > > > > > around in the damage area, and seems to reinvent a bunch of the 
> > > > > > stuff
> > > > > > here.
> > >
> > > We can use drm_atomic_helper_damage_merged() but it would only save us 
> > > one for loop.
> >
> > Yes please. The trouble with rolling our own copies for everything is
> > that it does add up.
> >
> > > > > Also, did we merge the igts for this stuff? They unfortunately never
> > > > > landed, when vmwgfx team did all this work, but for i915 we really
> > > > > shouldn't even land new support without tests.
> > > >
> > > > Lo and behold, we merge the uapi enabling way earlier than this patch 
> > > > here:
> > > >
> > > > commit 093a3a3926b8bda9eef773e4ed5079053350
> > > > Author: José Roberto de Souza 
> > > > Date:   Thu Jun 25 18:01:47 2020 -0700
> > > >
> > > >drm/i915: Add plane damage clips property
> > > >
> > > > And the igts are nowhere near to be seen, at least the stuff from
> > > > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > > > so this gets sorted out asap.
> > >
> > > Here the IGT: 
> > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c
> >
> > There was some igts that were cross-driver, not intel specific. The
> > thing here is supposed to be a cross-vendor interface, so would be
> > really good if you resurrect those from vmwgfx folks and land them
> > too:
> >
> > https://patchwork.freedesktop.org/series/51087/
>
> Also from a very quick look at the kms_psr2_sf tests there's really
> not a whole lot specific about our psr2 implementation in there. The
> test should work on any plane with a FB_DAMAGE_CLIPS rect available,
> so there's no reason to make this specific to intel, much less to
> psr2. I think we need to:
> - make this a generic kms test, it should work
> - merge with the testcases from the folks who merged FB_DAMAGE_CLIPS
> originally to make sure all drivers follow the same contract.
>
> KMS properties are generic, intel-specific tests for when there's
> nothing intel-specific isn't good. Also adding Pankaj.

Looks like Pankaj no longer working at intel.

Another one is this here:

igt_debug_manual_check("all", expected

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-21 Thread Daniel Vetter
On Wed, Jul 21, 2021 at 9:50 AM Daniel Vetter  wrote:
> On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose  wrote:
> > On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > > >  wrote:
> > > > > >
> > > > > > Now using plane damage clips property to calcualte the damaged area.
> > > > > > Selective fetch only supports one region to be fetched so software
> > > > > > needs to calculate a bounding box around all damage clips.
> > > > > >
> > > > > > Now that we are not complete fetching each plane, there is another
> > > > > > loop needed as all the plane areas that intersect with the pipe
> > > > > > damaged area needs to be fetched from memory so the complete 
> > > > > > blending
> > > > > > of all planes can happen.
> > > > > >
> > > > > > v2:
> > > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 
> > > > > > format
> > > > > >
> > > > > > v4:
> > > > > > - setting plane selective fetch area using the whole pipe damage 
> > > > > > area
> > > > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > > > changed
> > > > > >
> > > > > > v5:
> > > > > > - taking in consideration src.y1 in the damage coordinates
> > > > > > - adding to the pipe damaged area planes that were visible but are
> > > > > > invisible in the new state
> > > > > >
> > > > > > v6:
> > > > > > - consider old state plane coordinates when visibility changes or it
> > > > > > moved to calculate damaged area
> > > > > > - remove from damaged area the portion not in src clip
> > > > > >
> > > > > > v7:
> > > > > > - intersec every damage clip with src to minimize damaged area
> > > > > >
> > > > > > v8:
> > > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > > framebuffer coordinates that plane will start to fetch from
> > > > > >
> > > > > > v9:
> > > > > > - Only add plane dst or src to damaged_area if visible
> > > > > > - Early skip plane damage calculation if it was not visible in old 
> > > > > > and
> > > > > > new state
> > > > > >
> > > > > > Cc: Ville Syrjälä 
> > > > > > Cc: Gwan-gyeong Mun 
> > > > > > Reviewed-by: Gwan-gyeong Mun 
> > > > > > Signed-off-by: José Roberto de Souza 
> > > > >
> > > > > Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> > > > > over this, and this is one of the only two drivers that directly digs
> > > > > around in the damage area, and seems to reinvent a bunch of the stuff
> > > > > here.
> >
> > We can use drm_atomic_helper_damage_merged() but it would only save us one 
> > for loop.
>
> Yes please. The trouble with rolling our own copies for everything is
> that it does add up.
>
> > > > Also, did we merge the igts for this stuff? They unfortunately never
> > > > landed, when vmwgfx team did all this work, but for i915 we really
> > > > shouldn't even land new support without tests.
> > >
> > > Lo and behold, we merge the uapi enabling way earlier than this patch 
> > > here:
> > >
> > > commit 093a3a3926b8bda9eef773e4ed5079053350
> > > Author: José Roberto de Souza 
> > > Date:   Thu Jun 25 18:01:47 2020 -0700
> > >
> > >drm/i915: Add plane damage clips property
> > >
> > > And the igts are nowhere near to be seen, at least the stuff from
> > > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > > so this gets sorted out asap.
> >
> > Here the IGT: 
> > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c
>
> There was some igts that were cross-driver, not intel specific. The
> thing here is supposed to be a cross-vendor interface, so would be
> really good if you resurrect those from vmwgfx folks and land them
> too:
>
> https://patchwork.freedesktop.org/series/51087/

Also from a very quick look at the kms_psr2_sf tests there's really
not a whole lot specific about our psr2 implementation in there. The
test should work on any plane with a FB_DAMAGE_CLIPS rect available,
so there's no reason to make this specific to intel, much less to
psr2. I think we need to:
- make this a generic kms test, it should work
- merge with the testcases from the folks who merged FB_DAMAGE_CLIPS
originally to make sure all drivers follow the same contract.

KMS properties are generic, intel-specific tests for when there's
nothing intel-specific isn't good. Also adding Pankaj.

Cheers, Daniel

> >
> > >
> > > Thanks, Daniel
> > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 113 
> > > > > > ---
> > > > > >  1 file changed, 99 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index d9a395c486d3..f5b9519b3756 100644
> > > > > > --- a/drivers/gpu/drm/

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-21 Thread Daniel Vetter
On Tue, Jul 20, 2021 at 6:55 PM Souza, Jose  wrote:
> On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> > On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > > >  wrote:
> > > > >
> > > > > Now using plane damage clips property to calcualte the damaged area.
> > > > > Selective fetch only supports one region to be fetched so software
> > > > > needs to calculate a bounding box around all damage clips.
> > > > >
> > > > > Now that we are not complete fetching each plane, there is another
> > > > > loop needed as all the plane areas that intersect with the pipe
> > > > > damaged area needs to be fetched from memory so the complete blending
> > > > > of all planes can happen.
> > > > >
> > > > > v2:
> > > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 
> > > > > format
> > > > >
> > > > > v4:
> > > > > - setting plane selective fetch area using the whole pipe damage area
> > > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > > changed
> > > > >
> > > > > v5:
> > > > > - taking in consideration src.y1 in the damage coordinates
> > > > > - adding to the pipe damaged area planes that were visible but are
> > > > > invisible in the new state
> > > > >
> > > > > v6:
> > > > > - consider old state plane coordinates when visibility changes or it
> > > > > moved to calculate damaged area
> > > > > - remove from damaged area the portion not in src clip
> > > > >
> > > > > v7:
> > > > > - intersec every damage clip with src to minimize damaged area
> > > > >
> > > > > v8:
> > > > > - adjust pipe_damaged area to 4 lines grouping
> > > > > - adjust calculation now that is understood that uapi.src is the
> > > > > framebuffer coordinates that plane will start to fetch from
> > > > >
> > > > > v9:
> > > > > - Only add plane dst or src to damaged_area if visible
> > > > > - Early skip plane damage calculation if it was not visible in old and
> > > > > new state
> > > > >
> > > > > Cc: Ville Syrjälä 
> > > > > Cc: Gwan-gyeong Mun 
> > > > > Reviewed-by: Gwan-gyeong Mun 
> > > > > Signed-off-by: José Roberto de Souza 
> > > >
> > > > Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> > > > over this, and this is one of the only two drivers that directly digs
> > > > around in the damage area, and seems to reinvent a bunch of the stuff
> > > > here.
>
> We can use drm_atomic_helper_damage_merged() but it would only save us one 
> for loop.

Yes please. The trouble with rolling our own copies for everything is
that it does add up.

> > > Also, did we merge the igts for this stuff? They unfortunately never
> > > landed, when vmwgfx team did all this work, but for i915 we really
> > > shouldn't even land new support without tests.
> >
> > Lo and behold, we merge the uapi enabling way earlier than this patch here:
> >
> > commit 093a3a3926b8bda9eef773e4ed5079053350
> > Author: José Roberto de Souza 
> > Date:   Thu Jun 25 18:01:47 2020 -0700
> >
> >drm/i915: Add plane damage clips property
> >
> > And the igts are nowhere near to be seen, at least the stuff from
> > vmwgfx didn't land. Please file a JIRA internally and ping me on that
> > so this gets sorted out asap.
>
> Here the IGT: 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c

There was some igts that were cross-driver, not intel specific. The
thing here is supposed to be a cross-vendor interface, so would be
really good if you resurrect those from vmwgfx folks and land them
too:

https://patchwork.freedesktop.org/series/51087/

Cheers, Daniel



>
> >
> > Thanks, Daniel
> >
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 113 
> > > > > ---
> > > > >  1 file changed, 99 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index d9a395c486d3..f5b9519b3756 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
> > > > > intel_crtc_state *crtc_state,
> > > > > if (clip->y1 == -1)
> > > > > goto exit;
> > > > >
> > > > > +   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || 
> > > > > clip->y2 % 4);
> > > > > +
> > > > > val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > > > val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 
> > > > > 1);
> > > > > -   val |= 
> > > > > PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) + 1);
> > > > > +   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > > > >  exit:
> > > > > crtc_state->psr2_man_track_ctl = val;
> > > > >  }
> > > > > @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct 
>

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-20 Thread Souza, Jose
On Tue, 2021-07-20 at 17:31 +0200, Daniel Vetter wrote:
> On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> > On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> > >  wrote:
> > > > 
> > > > Now using plane damage clips property to calcualte the damaged area.
> > > > Selective fetch only supports one region to be fetched so software
> > > > needs to calculate a bounding box around all damage clips.
> > > > 
> > > > Now that we are not complete fetching each plane, there is another
> > > > loop needed as all the plane areas that intersect with the pipe
> > > > damaged area needs to be fetched from memory so the complete blending
> > > > of all planes can happen.
> > > > 
> > > > v2:
> > > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 format
> > > > 
> > > > v4:
> > > > - setting plane selective fetch area using the whole pipe damage area
> > > > - mark the whole plane area damaged if plane visibility or alpha
> > > > changed
> > > > 
> > > > v5:
> > > > - taking in consideration src.y1 in the damage coordinates
> > > > - adding to the pipe damaged area planes that were visible but are
> > > > invisible in the new state
> > > > 
> > > > v6:
> > > > - consider old state plane coordinates when visibility changes or it
> > > > moved to calculate damaged area
> > > > - remove from damaged area the portion not in src clip
> > > > 
> > > > v7:
> > > > - intersec every damage clip with src to minimize damaged area
> > > > 
> > > > v8:
> > > > - adjust pipe_damaged area to 4 lines grouping
> > > > - adjust calculation now that is understood that uapi.src is the
> > > > framebuffer coordinates that plane will start to fetch from
> > > > 
> > > > v9:
> > > > - Only add plane dst or src to damaged_area if visible
> > > > - Early skip plane damage calculation if it was not visible in old and
> > > > new state
> > > > 
> > > > Cc: Ville Syrjälä 
> > > > Cc: Gwan-gyeong Mun 
> > > > Reviewed-by: Gwan-gyeong Mun 
> > > > Signed-off-by: José Roberto de Souza 
> > > 
> > > Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> > > over this, and this is one of the only two drivers that directly digs
> > > around in the damage area, and seems to reinvent a bunch of the stuff
> > > here.

We can use drm_atomic_helper_damage_merged() but it would only save us one for 
loop.

> > 
> > Also, did we merge the igts for this stuff? They unfortunately never
> > landed, when vmwgfx team did all this work, but for i915 we really
> > shouldn't even land new support without tests.
> 
> Lo and behold, we merge the uapi enabling way earlier than this patch here:
> 
> commit 093a3a3926b8bda9eef773e4ed5079053350
> Author: José Roberto de Souza 
> Date:   Thu Jun 25 18:01:47 2020 -0700
> 
>drm/i915: Add plane damage clips property
> 
> And the igts are nowhere near to be seen, at least the stuff from
> vmwgfx didn't land. Please file a JIRA internally and ping me on that
> so this gets sorted out asap.

Here the IGT: 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_psr2_sf.c

> 
> Thanks, Daniel
> 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 113 ---
> > > >  1 file changed, 99 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index d9a395c486d3..f5b9519b3756 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
> > > > intel_crtc_state *crtc_state,
> > > > if (clip->y1 == -1)
> > > > goto exit;
> > > > 
> > > > +   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || 
> > > > clip->y2 % 4);
> > > > +
> > > > val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > > val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > > > -   val |= 
> > > > PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) + 1);
> > > > +   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > > >  exit:
> > > > crtc_state->psr2_man_track_ctl = val;
> > > >  }
> > > > @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct 
> > > > intel_atomic_state *state,
> > > > struct intel_crtc *crtc)
> > > >  {
> > > > struct intel_crtc_state *crtc_state = 
> > > > intel_atomic_get_new_crtc_state(state, crtc);
> > > > +   struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, 
> > > > .y2 = -1 };
> > > > struct intel_plane_state *new_plane_state, *old_plane_state;
> > > > -   struct drm_rect pipe_clip = { .y1 = -1 };
> > > > struct intel_plane *plane;
> > > > bool full_update = false;
> > > > int i, ret;
> > > > @@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-20 Thread Daniel Vetter
On Tue, Jul 20, 2021 at 5:16 PM Daniel Vetter  wrote:
> On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
> > On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
> >  wrote:
> > >
> > > Now using plane damage clips property to calcualte the damaged area.
> > > Selective fetch only supports one region to be fetched so software
> > > needs to calculate a bounding box around all damage clips.
> > >
> > > Now that we are not complete fetching each plane, there is another
> > > loop needed as all the plane areas that intersect with the pipe
> > > damaged area needs to be fetched from memory so the complete blending
> > > of all planes can happen.
> > >
> > > v2:
> > > - do not shifting new_plane_state->uapi.dst only src is in 16.16 format
> > >
> > > v4:
> > > - setting plane selective fetch area using the whole pipe damage area
> > > - mark the whole plane area damaged if plane visibility or alpha
> > > changed
> > >
> > > v5:
> > > - taking in consideration src.y1 in the damage coordinates
> > > - adding to the pipe damaged area planes that were visible but are
> > > invisible in the new state
> > >
> > > v6:
> > > - consider old state plane coordinates when visibility changes or it
> > > moved to calculate damaged area
> > > - remove from damaged area the portion not in src clip
> > >
> > > v7:
> > > - intersec every damage clip with src to minimize damaged area
> > >
> > > v8:
> > > - adjust pipe_damaged area to 4 lines grouping
> > > - adjust calculation now that is understood that uapi.src is the
> > > framebuffer coordinates that plane will start to fetch from
> > >
> > > v9:
> > > - Only add plane dst or src to damaged_area if visible
> > > - Early skip plane damage calculation if it was not visible in old and
> > > new state
> > >
> > > Cc: Ville Syrjälä 
> > > Cc: Gwan-gyeong Mun 
> > > Reviewed-by: Gwan-gyeong Mun 
> > > Signed-off-by: José Roberto de Souza 
> >
> > Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> > over this, and this is one of the only two drivers that directly digs
> > around in the damage area, and seems to reinvent a bunch of the stuff
> > here.
>
> Also, did we merge the igts for this stuff? They unfortunately never
> landed, when vmwgfx team did all this work, but for i915 we really
> shouldn't even land new support without tests.

Lo and behold, we merge the uapi enabling way earlier than this patch here:

commit 093a3a3926b8bda9eef773e4ed5079053350
Author: José Roberto de Souza 
Date:   Thu Jun 25 18:01:47 2020 -0700

   drm/i915: Add plane damage clips property

And the igts are nowhere near to be seen, at least the stuff from
vmwgfx didn't land. Please file a JIRA internally and ping me on that
so this gets sorted out asap.

Thanks, Daniel

> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 113 ---
> > >  1 file changed, 99 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index d9a395c486d3..f5b9519b3756 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
> > > intel_crtc_state *crtc_state,
> > > if (clip->y1 == -1)
> > > goto exit;
> > >
> > > +   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 
> > > % 4);
> > > +
> > > val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > > -   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 
> > > 4) + 1);
> > > +   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> > >  exit:
> > > crtc_state->psr2_man_track_ctl = val;
> > >  }
> > > @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct 
> > > intel_atomic_state *state,
> > > struct intel_crtc *crtc)
> > >  {
> > > struct intel_crtc_state *crtc_state = 
> > > intel_atomic_get_new_crtc_state(state, crtc);
> > > +   struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, 
> > > .y2 = -1 };
> > > struct intel_plane_state *new_plane_state, *old_plane_state;
> > > -   struct drm_rect pipe_clip = { .y1 = -1 };
> > > struct intel_plane *plane;
> > > bool full_update = false;
> > > int i, ret;
> > > @@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct 
> > > intel_atomic_state *state,
> > > if (ret)
> > > return ret;
> > >
> > > +   /*
> > > +* Calculate minimal selective fetch area of each plane and 
> > > calculate
> > > +* the pipe damaged area.
> > > +* In the next loop the plane selective fetch area will actually 
> > > be set
> > > +* using whole pipe damaged area.
> > > +*/
> > > for_each_oldnew_intel_plane_in_state(state, plane, 
> > > old_p

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-20 Thread Daniel Vetter
On Tue, Jul 20, 2021 at 5:09 PM Daniel Vetter  wrote:
>
> On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
>  wrote:
> >
> > Now using plane damage clips property to calcualte the damaged area.
> > Selective fetch only supports one region to be fetched so software
> > needs to calculate a bounding box around all damage clips.
> >
> > Now that we are not complete fetching each plane, there is another
> > loop needed as all the plane areas that intersect with the pipe
> > damaged area needs to be fetched from memory so the complete blending
> > of all planes can happen.
> >
> > v2:
> > - do not shifting new_plane_state->uapi.dst only src is in 16.16 format
> >
> > v4:
> > - setting plane selective fetch area using the whole pipe damage area
> > - mark the whole plane area damaged if plane visibility or alpha
> > changed
> >
> > v5:
> > - taking in consideration src.y1 in the damage coordinates
> > - adding to the pipe damaged area planes that were visible but are
> > invisible in the new state
> >
> > v6:
> > - consider old state plane coordinates when visibility changes or it
> > moved to calculate damaged area
> > - remove from damaged area the portion not in src clip
> >
> > v7:
> > - intersec every damage clip with src to minimize damaged area
> >
> > v8:
> > - adjust pipe_damaged area to 4 lines grouping
> > - adjust calculation now that is understood that uapi.src is the
> > framebuffer coordinates that plane will start to fetch from
> >
> > v9:
> > - Only add plane dst or src to damaged_area if visible
> > - Early skip plane damage calculation if it was not visible in old and
> > new state
> >
> > Cc: Ville Syrjälä 
> > Cc: Gwan-gyeong Mun 
> > Reviewed-by: Gwan-gyeong Mun 
> > Signed-off-by: José Roberto de Souza 
>
> Why is this not using drm_atomic_helper_damage_merged? I just stumbled
> over this, and this is one of the only two drivers that directly digs
> around in the damage area, and seems to reinvent a bunch of the stuff
> here.

Also, did we merge the igts for this stuff? They unfortunately never
landed, when vmwgfx team did all this work, but for i915 we really
shouldn't even land new support without tests.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 113 ---
> >  1 file changed, 99 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index d9a395c486d3..f5b9519b3756 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
> > intel_crtc_state *crtc_state,
> > if (clip->y1 == -1)
> > goto exit;
> >
> > +   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 
> > 4);
> > +
> > val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> > -   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 
> > 4) + 1);
> > +   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
> >  exit:
> > crtc_state->psr2_man_track_ctl = val;
> >  }
> > @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct 
> > intel_atomic_state *state,
> > struct intel_crtc *crtc)
> >  {
> > struct intel_crtc_state *crtc_state = 
> > intel_atomic_get_new_crtc_state(state, crtc);
> > +   struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, .y2 
> > = -1 };
> > struct intel_plane_state *new_plane_state, *old_plane_state;
> > -   struct drm_rect pipe_clip = { .y1 = -1 };
> > struct intel_plane *plane;
> > bool full_update = false;
> > int i, ret;
> > @@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct 
> > intel_atomic_state *state,
> > if (ret)
> > return ret;
> >
> > +   /*
> > +* Calculate minimal selective fetch area of each plane and 
> > calculate
> > +* the pipe damaged area.
> > +* In the next loop the plane selective fetch area will actually be 
> > set
> > +* using whole pipe damaged area.
> > +*/
> > for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
> >  new_plane_state, i) {
> > -   struct drm_rect *sel_fetch_area, temp;
> > +   struct drm_rect src, damaged_area = { .y1 = -1 };
> > +   struct drm_mode_rect *damaged_clips;
> > +   u32 num_clips, j;
> >
> > if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
> > continue;
> >
> > +   if (!new_plane_state->uapi.visible &&
> > +   !old_plane_state->uapi.visible)
> > +   continue;
> > +
> > /*
> >  * TODO: Not clear how to handle planes with negative 
> > pos

Re: [Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-07-20 Thread Daniel Vetter
On Mon, Jan 4, 2021 at 9:56 PM José Roberto de Souza
 wrote:
>
> Now using plane damage clips property to calcualte the damaged area.
> Selective fetch only supports one region to be fetched so software
> needs to calculate a bounding box around all damage clips.
>
> Now that we are not complete fetching each plane, there is another
> loop needed as all the plane areas that intersect with the pipe
> damaged area needs to be fetched from memory so the complete blending
> of all planes can happen.
>
> v2:
> - do not shifting new_plane_state->uapi.dst only src is in 16.16 format
>
> v4:
> - setting plane selective fetch area using the whole pipe damage area
> - mark the whole plane area damaged if plane visibility or alpha
> changed
>
> v5:
> - taking in consideration src.y1 in the damage coordinates
> - adding to the pipe damaged area planes that were visible but are
> invisible in the new state
>
> v6:
> - consider old state plane coordinates when visibility changes or it
> moved to calculate damaged area
> - remove from damaged area the portion not in src clip
>
> v7:
> - intersec every damage clip with src to minimize damaged area
>
> v8:
> - adjust pipe_damaged area to 4 lines grouping
> - adjust calculation now that is understood that uapi.src is the
> framebuffer coordinates that plane will start to fetch from
>
> v9:
> - Only add plane dst or src to damaged_area if visible
> - Early skip plane damage calculation if it was not visible in old and
> new state
>
> Cc: Ville Syrjälä 
> Cc: Gwan-gyeong Mun 
> Reviewed-by: Gwan-gyeong Mun 
> Signed-off-by: José Roberto de Souza 

Why is this not using drm_atomic_helper_damage_merged? I just stumbled
over this, and this is one of the only two drivers that directly digs
around in the damage area, and seems to reinvent a bunch of the stuff
here.
-Daniel

> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 113 ---
>  1 file changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index d9a395c486d3..f5b9519b3756 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
> intel_crtc_state *crtc_state,
> if (clip->y1 == -1)
> goto exit;
>
> +   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4);
> +
> val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
> -   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) 
> + 1);
> +   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
>  exit:
> crtc_state->psr2_man_track_ctl = val;
>  }
> @@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct 
> intel_atomic_state *state,
> struct intel_crtc *crtc)
>  {
> struct intel_crtc_state *crtc_state = 
> intel_atomic_get_new_crtc_state(state, crtc);
> +   struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, .y2 = 
> -1 };
> struct intel_plane_state *new_plane_state, *old_plane_state;
> -   struct drm_rect pipe_clip = { .y1 = -1 };
> struct intel_plane *plane;
> bool full_update = false;
> int i, ret;
> @@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct 
> intel_atomic_state *state,
> if (ret)
> return ret;
>
> +   /*
> +* Calculate minimal selective fetch area of each plane and calculate
> +* the pipe damaged area.
> +* In the next loop the plane selective fetch area will actually be 
> set
> +* using whole pipe damaged area.
> +*/
> for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
>  new_plane_state, i) {
> -   struct drm_rect *sel_fetch_area, temp;
> +   struct drm_rect src, damaged_area = { .y1 = -1 };
> +   struct drm_mode_rect *damaged_clips;
> +   u32 num_clips, j;
>
> if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
> continue;
>
> +   if (!new_plane_state->uapi.visible &&
> +   !old_plane_state->uapi.visible)
> +   continue;
> +
> /*
>  * TODO: Not clear how to handle planes with negative 
> position,
>  * also planes are not updated if they have a negative X
> @@ -1300,23 +1314,94 @@ int intel_psr2_sel_fetch_update(struct 
> intel_atomic_state *state,
> break;
> }
>
> -   if (!new_plane_state->uapi.visible)
> -   continue;
> +   num_clips = 
> drm_plane_get_damage_clips_count(&new_plane_state->uapi);
>
> /*
> -* For now doing a selective fe

[Intel-gfx] [PATCH CI 2/4] drm/i915/display/psr: Use plane damage clips to calculate damaged area

2021-01-04 Thread José Roberto de Souza
Now using plane damage clips property to calcualte the damaged area.
Selective fetch only supports one region to be fetched so software
needs to calculate a bounding box around all damage clips.

Now that we are not complete fetching each plane, there is another
loop needed as all the plane areas that intersect with the pipe
damaged area needs to be fetched from memory so the complete blending
of all planes can happen.

v2:
- do not shifting new_plane_state->uapi.dst only src is in 16.16 format

v4:
- setting plane selective fetch area using the whole pipe damage area
- mark the whole plane area damaged if plane visibility or alpha
changed

v5:
- taking in consideration src.y1 in the damage coordinates
- adding to the pipe damaged area planes that were visible but are
invisible in the new state

v6:
- consider old state plane coordinates when visibility changes or it
moved to calculate damaged area
- remove from damaged area the portion not in src clip

v7:
- intersec every damage clip with src to minimize damaged area

v8:
- adjust pipe_damaged area to 4 lines grouping
- adjust calculation now that is understood that uapi.src is the
framebuffer coordinates that plane will start to fetch from

v9:
- Only add plane dst or src to damaged_area if visible
- Early skip plane damage calculation if it was not visible in old and
new state

Cc: Ville Syrjälä 
Cc: Gwan-gyeong Mun 
Reviewed-by: Gwan-gyeong Mun 
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/display/intel_psr.c | 113 ---
 1 file changed, 99 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
b/drivers/gpu/drm/i915/display/intel_psr.c
index d9a395c486d3..f5b9519b3756 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1242,9 +1242,11 @@ static void psr2_man_trk_ctl_calc(struct 
intel_crtc_state *crtc_state,
if (clip->y1 == -1)
goto exit;
 
+   drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4);
+
val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
-   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(DIV_ROUND_UP(clip->y2, 4) + 
1);
+   val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
 exit:
crtc_state->psr2_man_track_ctl = val;
 }
@@ -1269,8 +1271,8 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state 
*state,
struct intel_crtc *crtc)
 {
struct intel_crtc_state *crtc_state = 
intel_atomic_get_new_crtc_state(state, crtc);
+   struct drm_rect pipe_clip = { .x1 = 0, .y1 = -1, .x2 = INT_MAX, .y2 = 
-1 };
struct intel_plane_state *new_plane_state, *old_plane_state;
-   struct drm_rect pipe_clip = { .y1 = -1 };
struct intel_plane *plane;
bool full_update = false;
int i, ret;
@@ -1282,13 +1284,25 @@ int intel_psr2_sel_fetch_update(struct 
intel_atomic_state *state,
if (ret)
return ret;
 
+   /*
+* Calculate minimal selective fetch area of each plane and calculate
+* the pipe damaged area.
+* In the next loop the plane selective fetch area will actually be set
+* using whole pipe damaged area.
+*/
for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
 new_plane_state, i) {
-   struct drm_rect *sel_fetch_area, temp;
+   struct drm_rect src, damaged_area = { .y1 = -1 };
+   struct drm_mode_rect *damaged_clips;
+   u32 num_clips, j;
 
if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
continue;
 
+   if (!new_plane_state->uapi.visible &&
+   !old_plane_state->uapi.visible)
+   continue;
+
/*
 * TODO: Not clear how to handle planes with negative position,
 * also planes are not updated if they have a negative X
@@ -1300,23 +1314,94 @@ int intel_psr2_sel_fetch_update(struct 
intel_atomic_state *state,
break;
}
 
-   if (!new_plane_state->uapi.visible)
-   continue;
+   num_clips = 
drm_plane_get_damage_clips_count(&new_plane_state->uapi);
 
/*
-* For now doing a selective fetch in the whole plane area,
-* optimizations will come in the future.
+* If visibility or plane moved, mark the whole plane area as
+* damaged as it needs to be complete redraw in the new and old
+* position.
 */
-   sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
-   sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >> 16;
-   sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >> 16;
+