Re: [PATCH 1/7] drm/amd/display: Store tiling_flags and tmz_surface on dm_plane_state

2020-08-07 Thread daniel 
On Thu, Jul 30, 2020 at 04:36:36PM -0400, Nicholas Kazlauskas wrote:
> [Why]
> Store these in advance so we can reuse them later in commit_tail without
> having to reserve the fbo again.
> 
> These will also be used for checking for tiling changes when deciding
> to reset the plane or not.

I've also dropped some comments on Bas' series for adding modifiers which
might be relevant for shuffling all this. But yeah stuff this into plane
state sounds like a good idea.
-Daniel

> 
> [How]
> This change should mostly be a refactor. Only commit check is affected
> for now and I'll drop the get_fb_info calls in prepare_planes and
> commit_tail after.
> 
> This runs a prepass loop once we think that all planes have been added
> to the context and replaces the get_fb_info calls with accessing the
> dm_plane_state instead.
> 
> Cc: Bhawanpreet Lakha 
> Cc: Rodrigo Siqueira 
> Signed-off-by: Nicholas Kazlauskas 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 60 +++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>  2 files changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 8d64f5fde7e2..7cc5ab90ce13 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3700,8 +3700,17 @@ static int fill_dc_scaling_info(const struct 
> drm_plane_state *state,
>  static int get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb,
>  uint64_t *tiling_flags, bool *tmz_surface)
>  {
> - struct amdgpu_bo *rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
> - int r = amdgpu_bo_reserve(rbo, false);
> + struct amdgpu_bo *rbo;
> + int r;
> +
> + if (!amdgpu_fb) {
> + *tiling_flags = 0;
> + *tmz_surface = false;
> + return 0;
> + }
> +
> + rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
> + r = amdgpu_bo_reserve(rbo, false);
>  
>   if (unlikely(r)) {
>   /* Don't show error message when returning -ERESTARTSYS */
> @@ -4124,13 +4133,10 @@ static int fill_dc_plane_attributes(struct 
> amdgpu_device *adev,
>   struct drm_crtc_state *crtc_state)
>  {
>   struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(crtc_state);
> - const struct amdgpu_framebuffer *amdgpu_fb =
> - to_amdgpu_framebuffer(plane_state->fb);
> + struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
>   struct dc_scaling_info scaling_info;
>   struct dc_plane_info plane_info;
> - uint64_t tiling_flags;
>   int ret;
> - bool tmz_surface = false;
>   bool force_disable_dcc = false;
>  
>   ret = fill_dc_scaling_info(plane_state, &scaling_info);
> @@ -4142,15 +4148,12 @@ static int fill_dc_plane_attributes(struct 
> amdgpu_device *adev,
>   dc_plane_state->clip_rect = scaling_info.clip_rect;
>   dc_plane_state->scaling_quality = scaling_info.scaling_quality;
>  
> - ret = get_fb_info(amdgpu_fb, &tiling_flags, &tmz_surface);
> - if (ret)
> - return ret;
> -
>   force_disable_dcc = adev->asic_type == CHIP_RAVEN && adev->in_suspend;
> - ret = fill_dc_plane_info_and_addr(adev, plane_state, tiling_flags,
> + ret = fill_dc_plane_info_and_addr(adev, plane_state,
> +   dm_plane_state->tiling_flags,
> &plane_info,
> &dc_plane_state->address,
> -   tmz_surface,
> +   dm_plane_state->tmz_surface,
> force_disable_dcc);
>   if (ret)
>   return ret;
> @@ -5753,6 +5756,10 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane)
>   dc_plane_state_retain(dm_plane_state->dc_state);
>   }
>  
> + /* Framebuffer hasn't been updated yet, so retain old flags. */
> + dm_plane_state->tiling_flags = old_dm_plane_state->tiling_flags;
> + dm_plane_state->tmz_surface = old_dm_plane_state->tmz_surface;
> +
>   return &dm_plane_state->base;
>  }
>  
> @@ -8557,13 +8564,9 @@ dm_determine_update_type_for_commit(struct 
> amdgpu_display_manager *dm,
>   continue;
>  
>   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, j) {
> - const struct amdgpu_framebuffer *amdgpu_fb =
> - to_amdgpu_framebuffer(new_plane_state->fb);
>   struct dc_plane_info *plane_info = 
> &bundle->plane_infos[num_plane];
>   struct dc_flip_addrs *flip_addr = 
> &bundle->flip_addrs[num_plane];
>   struct dc_scaling_info *scaling_info = 
> &bundle->scaling_infos[num_plane];
> - uint64_t tiling_flags;
> -

Re: [PATCH 1/7] drm/amd/display: Store tiling_flags and tmz_surface on dm_plane_state

2020-08-05 Thread Rodrigo Siqueira 
Reviewed-by: Rodrigo Siqueira 

On 07/30, Nicholas Kazlauskas wrote:
> [Why]
> Store these in advance so we can reuse them later in commit_tail without
> having to reserve the fbo again.
> 
> These will also be used for checking for tiling changes when deciding
> to reset the plane or not.
> 
> [How]
> This change should mostly be a refactor. Only commit check is affected
> for now and I'll drop the get_fb_info calls in prepare_planes and
> commit_tail after.
> 
> This runs a prepass loop once we think that all planes have been added
> to the context and replaces the get_fb_info calls with accessing the
> dm_plane_state instead.
> 
> Cc: Bhawanpreet Lakha 
> Cc: Rodrigo Siqueira 
> Signed-off-by: Nicholas Kazlauskas 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 60 +++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>  2 files changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 8d64f5fde7e2..7cc5ab90ce13 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3700,8 +3700,17 @@ static int fill_dc_scaling_info(const struct 
> drm_plane_state *state,
>  static int get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb,
>  uint64_t *tiling_flags, bool *tmz_surface)
>  {
> - struct amdgpu_bo *rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
> - int r = amdgpu_bo_reserve(rbo, false);
> + struct amdgpu_bo *rbo;
> + int r;
> +
> + if (!amdgpu_fb) {
> + *tiling_flags = 0;
> + *tmz_surface = false;
> + return 0;
> + }
> +
> + rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
> + r = amdgpu_bo_reserve(rbo, false);
>  
>   if (unlikely(r)) {
>   /* Don't show error message when returning -ERESTARTSYS */
> @@ -4124,13 +4133,10 @@ static int fill_dc_plane_attributes(struct 
> amdgpu_device *adev,
>   struct drm_crtc_state *crtc_state)
>  {
>   struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(crtc_state);
> - const struct amdgpu_framebuffer *amdgpu_fb =
> - to_amdgpu_framebuffer(plane_state->fb);
> + struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
>   struct dc_scaling_info scaling_info;
>   struct dc_plane_info plane_info;
> - uint64_t tiling_flags;
>   int ret;
> - bool tmz_surface = false;
>   bool force_disable_dcc = false;
>  
>   ret = fill_dc_scaling_info(plane_state, &scaling_info);
> @@ -4142,15 +4148,12 @@ static int fill_dc_plane_attributes(struct 
> amdgpu_device *adev,
>   dc_plane_state->clip_rect = scaling_info.clip_rect;
>   dc_plane_state->scaling_quality = scaling_info.scaling_quality;
>  
> - ret = get_fb_info(amdgpu_fb, &tiling_flags, &tmz_surface);
> - if (ret)
> - return ret;
> -
>   force_disable_dcc = adev->asic_type == CHIP_RAVEN && adev->in_suspend;
> - ret = fill_dc_plane_info_and_addr(adev, plane_state, tiling_flags,
> + ret = fill_dc_plane_info_and_addr(adev, plane_state,
> +   dm_plane_state->tiling_flags,
> &plane_info,
> &dc_plane_state->address,
> -   tmz_surface,
> +   dm_plane_state->tmz_surface,
> force_disable_dcc);
>   if (ret)
>   return ret;
> @@ -5753,6 +5756,10 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane)
>   dc_plane_state_retain(dm_plane_state->dc_state);
>   }
>  
> + /* Framebuffer hasn't been updated yet, so retain old flags. */
> + dm_plane_state->tiling_flags = old_dm_plane_state->tiling_flags;
> + dm_plane_state->tmz_surface = old_dm_plane_state->tmz_surface;
> +
>   return &dm_plane_state->base;
>  }
>  
> @@ -8557,13 +8564,9 @@ dm_determine_update_type_for_commit(struct 
> amdgpu_display_manager *dm,
>   continue;
>  
>   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, j) {
> - const struct amdgpu_framebuffer *amdgpu_fb =
> - to_amdgpu_framebuffer(new_plane_state->fb);
>   struct dc_plane_info *plane_info = 
> &bundle->plane_infos[num_plane];
>   struct dc_flip_addrs *flip_addr = 
> &bundle->flip_addrs[num_plane];
>   struct dc_scaling_info *scaling_info = 
> &bundle->scaling_infos[num_plane];
> - uint64_t tiling_flags;
> - bool tmz_surface = false;
>  
>   new_plane_crtc = new_plane_state->crtc;
>   new_dm_plane_state = to_dm_plane_state(new_plane_state);
> @@

[PATCH 1/7] drm/amd/display: Store tiling_flags and tmz_surface on dm_plane_state

2020-07-30 Thread Nicholas Kazlauskas 
[Why]
Store these in advance so we can reuse them later in commit_tail without
having to reserve the fbo again.

These will also be used for checking for tiling changes when deciding
to reset the plane or not.

[How]
This change should mostly be a refactor. Only commit check is affected
for now and I'll drop the get_fb_info calls in prepare_planes and
commit_tail after.

This runs a prepass loop once we think that all planes have been added
to the context and replaces the get_fb_info calls with accessing the
dm_plane_state instead.

Cc: Bhawanpreet Lakha 
Cc: Rodrigo Siqueira 
Signed-off-by: Nicholas Kazlauskas 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 60 +++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 8d64f5fde7e2..7cc5ab90ce13 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3700,8 +3700,17 @@ static int fill_dc_scaling_info(const struct 
drm_plane_state *state,
 static int get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb,
   uint64_t *tiling_flags, bool *tmz_surface)
 {
-   struct amdgpu_bo *rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
-   int r = amdgpu_bo_reserve(rbo, false);
+   struct amdgpu_bo *rbo;
+   int r;
+
+   if (!amdgpu_fb) {
+   *tiling_flags = 0;
+   *tmz_surface = false;
+   return 0;
+   }
+
+   rbo = gem_to_amdgpu_bo(amdgpu_fb->base.obj[0]);
+   r = amdgpu_bo_reserve(rbo, false);
 
if (unlikely(r)) {
/* Don't show error message when returning -ERESTARTSYS */
@@ -4124,13 +4133,10 @@ static int fill_dc_plane_attributes(struct 
amdgpu_device *adev,
struct drm_crtc_state *crtc_state)
 {
struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(crtc_state);
-   const struct amdgpu_framebuffer *amdgpu_fb =
-   to_amdgpu_framebuffer(plane_state->fb);
+   struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
struct dc_scaling_info scaling_info;
struct dc_plane_info plane_info;
-   uint64_t tiling_flags;
int ret;
-   bool tmz_surface = false;
bool force_disable_dcc = false;
 
ret = fill_dc_scaling_info(plane_state, &scaling_info);
@@ -4142,15 +4148,12 @@ static int fill_dc_plane_attributes(struct 
amdgpu_device *adev,
dc_plane_state->clip_rect = scaling_info.clip_rect;
dc_plane_state->scaling_quality = scaling_info.scaling_quality;
 
-   ret = get_fb_info(amdgpu_fb, &tiling_flags, &tmz_surface);
-   if (ret)
-   return ret;
-
force_disable_dcc = adev->asic_type == CHIP_RAVEN && adev->in_suspend;
-   ret = fill_dc_plane_info_and_addr(adev, plane_state, tiling_flags,
+   ret = fill_dc_plane_info_and_addr(adev, plane_state,
+ dm_plane_state->tiling_flags,
  &plane_info,
  &dc_plane_state->address,
- tmz_surface,
+ dm_plane_state->tmz_surface,
  force_disable_dcc);
if (ret)
return ret;
@@ -5753,6 +5756,10 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane)
dc_plane_state_retain(dm_plane_state->dc_state);
}
 
+   /* Framebuffer hasn't been updated yet, so retain old flags. */
+   dm_plane_state->tiling_flags = old_dm_plane_state->tiling_flags;
+   dm_plane_state->tmz_surface = old_dm_plane_state->tmz_surface;
+
return &dm_plane_state->base;
 }
 
@@ -8557,13 +8564,9 @@ dm_determine_update_type_for_commit(struct 
amdgpu_display_manager *dm,
continue;
 
for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, j) {
-   const struct amdgpu_framebuffer *amdgpu_fb =
-   to_amdgpu_framebuffer(new_plane_state->fb);
struct dc_plane_info *plane_info = 
&bundle->plane_infos[num_plane];
struct dc_flip_addrs *flip_addr = 
&bundle->flip_addrs[num_plane];
struct dc_scaling_info *scaling_info = 
&bundle->scaling_infos[num_plane];
-   uint64_t tiling_flags;
-   bool tmz_surface = false;
 
new_plane_crtc = new_plane_state->crtc;
new_dm_plane_state = to_dm_plane_state(new_plane_state);
@@ -8610,16 +8613,12 @@ dm_determine_update_type_for_commit(struct 
amdgpu_display_manager *dm,
 
bundle->surface_updates[num_plane].scaling_info = 
scaling_info;