| Message ID | 20191218161105.30638-6-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl: Render/media decompression support | expand |
On Wed, 2019-12-18 at 18:10 +0200, Imre Deak wrote: > Using helpers instead of open coding this to select a CCS plane for a > main plane makes the code cleaner and less error-prone when the > location > of CCS plane can be different based on the format (packed vs. YUV > semiplanar). The same applies to selecting an AUX plane which can be > a > UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++ > ---- > 1 file changed, 50 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 4b8b44c39724..6bda397ae677 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const > struct drm_i915_private *dev_priv) > return IS_GEN(dev_priv, 2) ? 2048 : 4096; > } > > +static bool is_ccs_plane(const struct drm_framebuffer *fb, int > plane) > +{ > + if (!is_ccs_modifier(fb->modifier)) > + return false; > + > + return plane >= fb->format->num_planes / 2; > +} > + > +static bool is_aux_plane(const struct drm_framebuffer *fb, int > plane) > +{ > + if (is_ccs_modifier(fb->modifier)) > + return is_ccs_plane(fb, plane); > + > + return plane == 1; > +} > + > +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int > main_plane) > +{ > + WARN_ON(!is_ccs_modifier(fb->modifier) || > + (main_plane && main_plane >= fb->format->num_planes / > 2)); > + > + return fb->format->num_planes / 2 + main_plane; > +} > + > +/* Return either the main plane's CCS or - if not a CCS FB - UV > plane */ > +static int > +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int > main_plane) > +{ > + if (is_ccs_modifier(fb->modifier)) > + return main_to_ccs_plane(fb, main_plane); > + > + return 1; > +} > + > static unsigned int > intel_tile_width_bytes(const struct drm_framebuffer *fb, int > color_plane) > { > @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct > drm_framebuffer *fb, int color_plane) > else > return 512; > case I915_FORMAT_MOD_Y_TILED_CCS: > - if (color_plane == 1) > + if (is_ccs_plane(fb, color_plane)) > return 128; > /* fall through */ > case I915_FORMAT_MOD_Y_TILED: > @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct > drm_framebuffer *fb, int color_plane) > else > return 512; > case I915_FORMAT_MOD_Yf_TILED_CCS: > - if (color_plane == 1) > + if (is_ccs_plane(fb, color_plane)) > return 128; > /* fall through */ > case I915_FORMAT_MOD_Yf_TILED: > @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const > struct drm_framebuffer *fb, > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > /* AUX_DIST needs only 4K alignment */ > - if (color_plane == 1) > + if (is_aux_plane(fb, color_plane)) > return 4096; > > switch (fb->modifier) { > @@ -3457,10 +3491,11 @@ static bool > skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > const struct drm_framebuffer *fb = plane_state->hw.fb; > int hsub = fb->format->hsub; > int vsub = fb->format->vsub; > - int aux_x = plane_state->color_plane[1].x; > - int aux_y = plane_state->color_plane[1].y; > - u32 aux_offset = plane_state->color_plane[1].offset; > - u32 alignment = intel_surf_alignment(fb, 1); > + int ccs_plane = main_to_ccs_plane(fb, 0); > + int aux_x = plane_state->color_plane[ccs_plane].x; > + int aux_y = plane_state->color_plane[ccs_plane].y; > + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; > + u32 alignment = intel_surf_alignment(fb, ccs_plane); > > while (aux_offset >= main_offset && aux_y <= main_y) { > int x, y; > @@ -3473,7 +3508,7 @@ static bool > skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > x = aux_x / hsub; > y = aux_y / vsub; > - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, > plane_state, 1, > + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, > plane_state, ccs_plane, > aux_offs > et, aux_offset - alignment); > aux_x = x * hsub + aux_x % hsub; > aux_y = y * vsub + aux_y % vsub; > @@ -3482,9 +3517,9 @@ static bool > skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > if (aux_x != main_x || aux_y != main_y) > return false; > > - plane_state->color_plane[1].offset = aux_offset; > - plane_state->color_plane[1].x = aux_x; > - plane_state->color_plane[1].y = aux_y; > + plane_state->color_plane[ccs_plane].offset = aux_offset; > + plane_state->color_plane[ccs_plane].x = aux_x; > + plane_state->color_plane[ccs_plane].y = aux_y; > > return true; > } > @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct > intel_plane_state *plane_state) > int h = drm_rect_height(&plane_state->uapi.src) >> 16; > int max_width; > int max_height; > - u32 alignment, offset, aux_offset = plane_state- > >color_plane[1].offset; > + int aux_plane = intel_main_to_aux_plane(fb, 0); > + u32 alignment, offset, aux_offset = plane_state- > >color_plane[aux_plane].offset; > > if (INTEL_GEN(dev_priv) >= 11) > max_width = icl_max_plane_width(fb, 0, rotation); > @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct > intel_plane_state *plane_state) > offs > et, offset - alignment); > } > > - if (x != plane_state->color_plane[1].x || y != > plane_state->color_plane[1].y) { > + if (x != plane_state->color_plane[aux_plane].x || > + y != plane_state->color_plane[aux_plane].y) { > DRM_DEBUG_KMS("Unable to find suitable display > surface offset due to CCS\n"); > return -EINVAL; > }
On Wed, Dec 18, 2019 at 06:10:55PM +0200, Imre Deak wrote: > Using helpers instead of open coding this to select a CCS plane for a > main plane makes the code cleaner and less error-prone when the location > of CCS plane can be different based on the format (packed vs. YUV > semiplanar). The same applies to selecting an AUX plane which can be a > UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> Looking at this makes me wonder if some of the aux stuff that we're doing for YUV in skl_check_main_surface is actually necessary for gen11+ now that we have separate planes rather than an AUX surface in the same plane. But none of the logic should be impacted by your changes here so, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++---- > 1 file changed, 50 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 4b8b44c39724..6bda397ae677 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) > return IS_GEN(dev_priv, 2) ? 2048 : 4096; > } > > +static bool is_ccs_plane(const struct drm_framebuffer *fb, int plane) > +{ > + if (!is_ccs_modifier(fb->modifier)) > + return false; > + > + return plane >= fb->format->num_planes / 2; > +} > + > +static bool is_aux_plane(const struct drm_framebuffer *fb, int plane) > +{ > + if (is_ccs_modifier(fb->modifier)) > + return is_ccs_plane(fb, plane); > + > + return plane == 1; > +} > + > +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane) > +{ > + WARN_ON(!is_ccs_modifier(fb->modifier) || > + (main_plane && main_plane >= fb->format->num_planes / 2)); > + > + return fb->format->num_planes / 2 + main_plane; > +} > + > +/* Return either the main plane's CCS or - if not a CCS FB - UV plane */ > +static int > +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) > +{ > + if (is_ccs_modifier(fb->modifier)) > + return main_to_ccs_plane(fb, main_plane); > + > + return 1; > +} > + > static unsigned int > intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > { > @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > else > return 512; > case I915_FORMAT_MOD_Y_TILED_CCS: > - if (color_plane == 1) > + if (is_ccs_plane(fb, color_plane)) > return 128; > /* fall through */ > case I915_FORMAT_MOD_Y_TILED: > @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > else > return 512; > case I915_FORMAT_MOD_Yf_TILED_CCS: > - if (color_plane == 1) > + if (is_ccs_plane(fb, color_plane)) > return 128; > /* fall through */ > case I915_FORMAT_MOD_Yf_TILED: > @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > /* AUX_DIST needs only 4K alignment */ > - if (color_plane == 1) > + if (is_aux_plane(fb, color_plane)) > return 4096; > > switch (fb->modifier) { > @@ -3457,10 +3491,11 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > const struct drm_framebuffer *fb = plane_state->hw.fb; > int hsub = fb->format->hsub; > int vsub = fb->format->vsub; > - int aux_x = plane_state->color_plane[1].x; > - int aux_y = plane_state->color_plane[1].y; > - u32 aux_offset = plane_state->color_plane[1].offset; > - u32 alignment = intel_surf_alignment(fb, 1); > + int ccs_plane = main_to_ccs_plane(fb, 0); > + int aux_x = plane_state->color_plane[ccs_plane].x; > + int aux_y = plane_state->color_plane[ccs_plane].y; > + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; > + u32 alignment = intel_surf_alignment(fb, ccs_plane); > > while (aux_offset >= main_offset && aux_y <= main_y) { > int x, y; > @@ -3473,7 +3508,7 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > x = aux_x / hsub; > y = aux_y / vsub; > - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 1, > + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, ccs_plane, > aux_offset, aux_offset - alignment); > aux_x = x * hsub + aux_x % hsub; > aux_y = y * vsub + aux_y % vsub; > @@ -3482,9 +3517,9 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > if (aux_x != main_x || aux_y != main_y) > return false; > > - plane_state->color_plane[1].offset = aux_offset; > - plane_state->color_plane[1].x = aux_x; > - plane_state->color_plane[1].y = aux_y; > + plane_state->color_plane[ccs_plane].offset = aux_offset; > + plane_state->color_plane[ccs_plane].x = aux_x; > + plane_state->color_plane[ccs_plane].y = aux_y; > > return true; > } > @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > int h = drm_rect_height(&plane_state->uapi.src) >> 16; > int max_width; > int max_height; > - u32 alignment, offset, aux_offset = plane_state->color_plane[1].offset; > + int aux_plane = intel_main_to_aux_plane(fb, 0); > + u32 alignment, offset, aux_offset = plane_state->color_plane[aux_plane].offset; > > if (INTEL_GEN(dev_priv) >= 11) > max_width = icl_max_plane_width(fb, 0, rotation); > @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > offset, offset - alignment); > } > > - if (x != plane_state->color_plane[1].x || y != plane_state->color_plane[1].y) { > + if (x != plane_state->color_plane[aux_plane].x || > + y != plane_state->color_plane[aux_plane].y) { > DRM_DEBUG_KMS("Unable to find suitable display surface offset due to CCS\n"); > return -EINVAL; > } > -- > 2.22.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 19, 2019 at 01:04:33PM -0800, Matt Roper wrote: > On Wed, Dec 18, 2019 at 06:10:55PM +0200, Imre Deak wrote: > > Using helpers instead of open coding this to select a CCS plane for a > > main plane makes the code cleaner and less error-prone when the location > > of CCS plane can be different based on the format (packed vs. YUV > > semiplanar). The same applies to selecting an AUX plane which can be a > > UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Looking at this makes me wonder if some of the aux stuff that we're > doing for YUV in skl_check_main_surface is actually necessary for gen11+ > now that we have separate planes rather than an AUX surface in the same > plane. I also wondered if programming the UV surface's offset in the Y plane's AUX_DIST register is necessary or not on GEN11+, however that's what we do atm. > > But none of the logic should be impacted by your changes here so, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++---- > > 1 file changed, 50 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 4b8b44c39724..6bda397ae677 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) > > return IS_GEN(dev_priv, 2) ? 2048 : 4096; > > } > > > > +static bool is_ccs_plane(const struct drm_framebuffer *fb, int plane) > > +{ > > + if (!is_ccs_modifier(fb->modifier)) > > + return false; > > + > > + return plane >= fb->format->num_planes / 2; > > +} > > + > > +static bool is_aux_plane(const struct drm_framebuffer *fb, int plane) > > +{ > > + if (is_ccs_modifier(fb->modifier)) > > + return is_ccs_plane(fb, plane); > > + > > + return plane == 1; > > +} > > + > > +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane) > > +{ > > + WARN_ON(!is_ccs_modifier(fb->modifier) || > > + (main_plane && main_plane >= fb->format->num_planes / 2)); > > + > > + return fb->format->num_planes / 2 + main_plane; > > +} > > + > > +/* Return either the main plane's CCS or - if not a CCS FB - UV plane */ > > +static int > > +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) > > +{ > > + if (is_ccs_modifier(fb->modifier)) > > + return main_to_ccs_plane(fb, main_plane); > > + > > + return 1; > > +} > > + > > static unsigned int > > intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > { > > @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > else > > return 512; > > case I915_FORMAT_MOD_Y_TILED_CCS: > > - if (color_plane == 1) > > + if (is_ccs_plane(fb, color_plane)) > > return 128; > > /* fall through */ > > case I915_FORMAT_MOD_Y_TILED: > > @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > else > > return 512; > > case I915_FORMAT_MOD_Yf_TILED_CCS: > > - if (color_plane == 1) > > + if (is_ccs_plane(fb, color_plane)) > > return 128; > > /* fall through */ > > case I915_FORMAT_MOD_Yf_TILED: > > @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, > > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > > > /* AUX_DIST needs only 4K alignment */ > > - if (color_plane == 1) > > + if (is_aux_plane(fb, color_plane)) > > return 4096; > > > > switch (fb->modifier) { > > @@ -3457,10 +3491,11 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > int hsub = fb->format->hsub; > > int vsub = fb->format->vsub; > > - int aux_x = plane_state->color_plane[1].x; > > - int aux_y = plane_state->color_plane[1].y; > > - u32 aux_offset = plane_state->color_plane[1].offset; > > - u32 alignment = intel_surf_alignment(fb, 1); > > + int ccs_plane = main_to_ccs_plane(fb, 0); > > + int aux_x = plane_state->color_plane[ccs_plane].x; > > + int aux_y = plane_state->color_plane[ccs_plane].y; > > + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; > > + u32 alignment = intel_surf_alignment(fb, ccs_plane); > > > > while (aux_offset >= main_offset && aux_y <= main_y) { > > int x, y; > > @@ -3473,7 +3508,7 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > > x = aux_x / hsub; > > y = aux_y / vsub; > > - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 1, > > + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, ccs_plane, > > aux_offset, aux_offset - alignment); > > aux_x = x * hsub + aux_x % hsub; > > aux_y = y * vsub + aux_y % vsub; > > @@ -3482,9 +3517,9 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > if (aux_x != main_x || aux_y != main_y) > > return false; > > > > - plane_state->color_plane[1].offset = aux_offset; > > - plane_state->color_plane[1].x = aux_x; > > - plane_state->color_plane[1].y = aux_y; > > + plane_state->color_plane[ccs_plane].offset = aux_offset; > > + plane_state->color_plane[ccs_plane].x = aux_x; > > + plane_state->color_plane[ccs_plane].y = aux_y; > > > > return true; > > } > > @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > int h = drm_rect_height(&plane_state->uapi.src) >> 16; > > int max_width; > > int max_height; > > - u32 alignment, offset, aux_offset = plane_state->color_plane[1].offset; > > + int aux_plane = intel_main_to_aux_plane(fb, 0); > > + u32 alignment, offset, aux_offset = plane_state->color_plane[aux_plane].offset; > > > > if (INTEL_GEN(dev_priv) >= 11) > > max_width = icl_max_plane_width(fb, 0, rotation); > > @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > offset, offset - alignment); > > } > > > > - if (x != plane_state->color_plane[1].x || y != plane_state->color_plane[1].y) { > > + if (x != plane_state->color_plane[aux_plane].x || > > + y != plane_state->color_plane[aux_plane].y) { > > DRM_DEBUG_KMS("Unable to find suitable display surface offset due to CCS\n"); > > return -EINVAL; > > } > > -- > > 2.22.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795
On Fri, Dec 20, 2019 at 02:26:07AM +0200, Imre Deak wrote: > On Thu, Dec 19, 2019 at 01:04:33PM -0800, Matt Roper wrote: > > On Wed, Dec 18, 2019 at 06:10:55PM +0200, Imre Deak wrote: > > > Using helpers instead of open coding this to select a CCS plane for a > > > main plane makes the code cleaner and less error-prone when the location > > > of CCS plane can be different based on the format (packed vs. YUV > > > semiplanar). The same applies to selecting an AUX plane which can be a > > > UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > Looking at this makes me wonder if some of the aux stuff that we're > > doing for YUV in skl_check_main_surface is actually necessary for gen11+ > > now that we have separate planes rather than an AUX surface in the same > > plane. > > I also wondered if programming the UV surface's offset in the Y plane's > AUX_DIST register is necessary or not on GEN11+, however that's what we > do atm. Shouldn't be needed. But I presume you're changing that anyway for media compression since you need aux for both Y and UV? > > > > > But none of the logic should be impacted by your changes here so, > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++---- > > > 1 file changed, 50 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 4b8b44c39724..6bda397ae677 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) > > > return IS_GEN(dev_priv, 2) ? 2048 : 4096; > > > } > > > > > > +static bool is_ccs_plane(const struct drm_framebuffer *fb, int plane) > > > +{ > > > + if (!is_ccs_modifier(fb->modifier)) > > > + return false; > > > + > > > + return plane >= fb->format->num_planes / 2; > > > +} > > > + > > > +static bool is_aux_plane(const struct drm_framebuffer *fb, int plane) > > > +{ > > > + if (is_ccs_modifier(fb->modifier)) > > > + return is_ccs_plane(fb, plane); > > > + > > > + return plane == 1; > > > +} > > > + > > > +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane) > > > +{ > > > + WARN_ON(!is_ccs_modifier(fb->modifier) || > > > + (main_plane && main_plane >= fb->format->num_planes / 2)); > > > + > > > + return fb->format->num_planes / 2 + main_plane; > > > +} > > > + > > > +/* Return either the main plane's CCS or - if not a CCS FB - UV plane */ > > > +static int > > > +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) > > > +{ > > > + if (is_ccs_modifier(fb->modifier)) > > > + return main_to_ccs_plane(fb, main_plane); > > > + > > > + return 1; > > > +} > > > + > > > static unsigned int > > > intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > { > > > @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > else > > > return 512; > > > case I915_FORMAT_MOD_Y_TILED_CCS: > > > - if (color_plane == 1) > > > + if (is_ccs_plane(fb, color_plane)) > > > return 128; > > > /* fall through */ > > > case I915_FORMAT_MOD_Y_TILED: > > > @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > else > > > return 512; > > > case I915_FORMAT_MOD_Yf_TILED_CCS: > > > - if (color_plane == 1) > > > + if (is_ccs_plane(fb, color_plane)) > > > return 128; > > > /* fall through */ > > > case I915_FORMAT_MOD_Yf_TILED: > > > @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, > > > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > > > > > /* AUX_DIST needs only 4K alignment */ > > > - if (color_plane == 1) > > > + if (is_aux_plane(fb, color_plane)) > > > return 4096; > > > > > > switch (fb->modifier) { > > > @@ -3457,10 +3491,11 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > > int hsub = fb->format->hsub; > > > int vsub = fb->format->vsub; > > > - int aux_x = plane_state->color_plane[1].x; > > > - int aux_y = plane_state->color_plane[1].y; > > > - u32 aux_offset = plane_state->color_plane[1].offset; > > > - u32 alignment = intel_surf_alignment(fb, 1); > > > + int ccs_plane = main_to_ccs_plane(fb, 0); > > > + int aux_x = plane_state->color_plane[ccs_plane].x; > > > + int aux_y = plane_state->color_plane[ccs_plane].y; > > > + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; > > > + u32 alignment = intel_surf_alignment(fb, ccs_plane); > > > > > > while (aux_offset >= main_offset && aux_y <= main_y) { > > > int x, y; > > > @@ -3473,7 +3508,7 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > > > > x = aux_x / hsub; > > > y = aux_y / vsub; > > > - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 1, > > > + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, ccs_plane, > > > aux_offset, aux_offset - alignment); > > > aux_x = x * hsub + aux_x % hsub; > > > aux_y = y * vsub + aux_y % vsub; > > > @@ -3482,9 +3517,9 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > if (aux_x != main_x || aux_y != main_y) > > > return false; > > > > > > - plane_state->color_plane[1].offset = aux_offset; > > > - plane_state->color_plane[1].x = aux_x; > > > - plane_state->color_plane[1].y = aux_y; > > > + plane_state->color_plane[ccs_plane].offset = aux_offset; > > > + plane_state->color_plane[ccs_plane].x = aux_x; > > > + plane_state->color_plane[ccs_plane].y = aux_y; > > > > > > return true; > > > } > > > @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > > int h = drm_rect_height(&plane_state->uapi.src) >> 16; > > > int max_width; > > > int max_height; > > > - u32 alignment, offset, aux_offset = plane_state->color_plane[1].offset; > > > + int aux_plane = intel_main_to_aux_plane(fb, 0); > > > + u32 alignment, offset, aux_offset = plane_state->color_plane[aux_plane].offset; > > > > > > if (INTEL_GEN(dev_priv) >= 11) > > > max_width = icl_max_plane_width(fb, 0, rotation); > > > @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > > offset, offset - alignment); > > > } > > > > > > - if (x != plane_state->color_plane[1].x || y != plane_state->color_plane[1].y) { > > > + if (x != plane_state->color_plane[aux_plane].x || > > > + y != plane_state->color_plane[aux_plane].y) { > > > DRM_DEBUG_KMS("Unable to find suitable display surface offset due to CCS\n"); > > > return -EINVAL; > > > } > > > -- > > > 2.22.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Matt Roper > > Graphics Software Engineer > > VTT-OSGC Platform Enablement > > Intel Corporation > > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 20, 2019 at 04:03:20PM +0200, Ville Syrjälä wrote: > On Fri, Dec 20, 2019 at 02:26:07AM +0200, Imre Deak wrote: > > On Thu, Dec 19, 2019 at 01:04:33PM -0800, Matt Roper wrote: > > > On Wed, Dec 18, 2019 at 06:10:55PM +0200, Imre Deak wrote: > > > > Using helpers instead of open coding this to select a CCS plane for a > > > > main plane makes the code cleaner and less error-prone when the location > > > > of CCS plane can be different based on the format (packed vs. YUV > > > > semiplanar). The same applies to selecting an AUX plane which can be a > > > > UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > > Looking at this makes me wonder if some of the aux stuff that we're > > > doing for YUV in skl_check_main_surface is actually necessary for gen11+ > > > now that we have separate planes rather than an AUX surface in the same > > > plane. > > > > I also wondered if programming the UV surface's offset in the Y plane's > > AUX_DIST register is necessary or not on GEN11+, however that's what we > > do atm. > > Shouldn't be needed. But I presume you're changing that anyway for media > compression since you need aux for both Y and UV? Right, CCS planes (both for RGB and YUV) will need to be programmed as an AUX plane of their main surface (and for that the main_to_aux_plane() helper came handy since the code is shared since GEN9). What I was pondering after Matt's comment is that for GEN11 we could stop programming the UV AUX plane in skl_program_plane() which means the UV coords need not match the main plane coords, what we require now even on GEN11 (otoh for CCS AUX we need coordinates to match there too). But this is unrelated to the change in this patch, so we can ignore that for now. > > > > > > But none of the logic should be impacted by your changes here so, > > > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++---- > > > > 1 file changed, 50 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 4b8b44c39724..6bda397ae677 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) > > > > return IS_GEN(dev_priv, 2) ? 2048 : 4096; > > > > } > > > > > > > > +static bool is_ccs_plane(const struct drm_framebuffer *fb, int plane) > > > > +{ > > > > + if (!is_ccs_modifier(fb->modifier)) > > > > + return false; > > > > + > > > > + return plane >= fb->format->num_planes / 2; > > > > +} > > > > + > > > > +static bool is_aux_plane(const struct drm_framebuffer *fb, int plane) > > > > +{ > > > > + if (is_ccs_modifier(fb->modifier)) > > > > + return is_ccs_plane(fb, plane); > > > > + > > > > + return plane == 1; > > > > +} > > > > + > > > > +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane) > > > > +{ > > > > + WARN_ON(!is_ccs_modifier(fb->modifier) || > > > > + (main_plane && main_plane >= fb->format->num_planes / 2)); > > > > + > > > > + return fb->format->num_planes / 2 + main_plane; > > > > +} > > > > + > > > > +/* Return either the main plane's CCS or - if not a CCS FB - UV plane */ > > > > +static int > > > > +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) > > > > +{ > > > > + if (is_ccs_modifier(fb->modifier)) > > > > + return main_to_ccs_plane(fb, main_plane); > > > > + > > > > + return 1; > > > > +} > > > > + > > > > static unsigned int > > > > intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > > { > > > > @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > > else > > > > return 512; > > > > case I915_FORMAT_MOD_Y_TILED_CCS: > > > > - if (color_plane == 1) > > > > + if (is_ccs_plane(fb, color_plane)) > > > > return 128; > > > > /* fall through */ > > > > case I915_FORMAT_MOD_Y_TILED: > > > > @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) > > > > else > > > > return 512; > > > > case I915_FORMAT_MOD_Yf_TILED_CCS: > > > > - if (color_plane == 1) > > > > + if (is_ccs_plane(fb, color_plane)) > > > > return 128; > > > > /* fall through */ > > > > case I915_FORMAT_MOD_Yf_TILED: > > > > @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, > > > > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > > > > > > > /* AUX_DIST needs only 4K alignment */ > > > > - if (color_plane == 1) > > > > + if (is_aux_plane(fb, color_plane)) > > > > return 4096; > > > > > > > > switch (fb->modifier) { > > > > @@ -3457,10 +3491,11 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > > > int hsub = fb->format->hsub; > > > > int vsub = fb->format->vsub; > > > > - int aux_x = plane_state->color_plane[1].x; > > > > - int aux_y = plane_state->color_plane[1].y; > > > > - u32 aux_offset = plane_state->color_plane[1].offset; > > > > - u32 alignment = intel_surf_alignment(fb, 1); > > > > + int ccs_plane = main_to_ccs_plane(fb, 0); > > > > + int aux_x = plane_state->color_plane[ccs_plane].x; > > > > + int aux_y = plane_state->color_plane[ccs_plane].y; > > > > + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; > > > > + u32 alignment = intel_surf_alignment(fb, ccs_plane); > > > > > > > > while (aux_offset >= main_offset && aux_y <= main_y) { > > > > int x, y; > > > > @@ -3473,7 +3508,7 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > > > > > > x = aux_x / hsub; > > > > y = aux_y / vsub; > > > > - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 1, > > > > + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, ccs_plane, > > > > aux_offset, aux_offset - alignment); > > > > aux_x = x * hsub + aux_x % hsub; > > > > aux_y = y * vsub + aux_y % vsub; > > > > @@ -3482,9 +3517,9 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state > > > > if (aux_x != main_x || aux_y != main_y) > > > > return false; > > > > > > > > - plane_state->color_plane[1].offset = aux_offset; > > > > - plane_state->color_plane[1].x = aux_x; > > > > - plane_state->color_plane[1].y = aux_y; > > > > + plane_state->color_plane[ccs_plane].offset = aux_offset; > > > > + plane_state->color_plane[ccs_plane].x = aux_x; > > > > + plane_state->color_plane[ccs_plane].y = aux_y; > > > > > > > > return true; > > > > } > > > > @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > > > int h = drm_rect_height(&plane_state->uapi.src) >> 16; > > > > int max_width; > > > > int max_height; > > > > - u32 alignment, offset, aux_offset = plane_state->color_plane[1].offset; > > > > + int aux_plane = intel_main_to_aux_plane(fb, 0); > > > > + u32 alignment, offset, aux_offset = plane_state->color_plane[aux_plane].offset; > > > > > > > > if (INTEL_GEN(dev_priv) >= 11) > > > > max_width = icl_max_plane_width(fb, 0, rotation); > > > > @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > > > offset, offset - alignment); > > > > } > > > > > > > > - if (x != plane_state->color_plane[1].x || y != plane_state->color_plane[1].y) { > > > > + if (x != plane_state->color_plane[aux_plane].x || > > > > + y != plane_state->color_plane[aux_plane].y) { > > > > DRM_DEBUG_KMS("Unable to find suitable display surface offset due to CCS\n"); > > > > return -EINVAL; > > > > } > > > > -- > > > > 2.22.0 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > VTT-OSGC Platform Enablement > > > Intel Corporation > > > (916) 356-2795 > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 4b8b44c39724..6bda397ae677 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1933,6 +1933,40 @@ static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) return IS_GEN(dev_priv, 2) ? 2048 : 4096; } +static bool is_ccs_plane(const struct drm_framebuffer *fb, int plane) +{ + if (!is_ccs_modifier(fb->modifier)) + return false; + + return plane >= fb->format->num_planes / 2; +} + +static bool is_aux_plane(const struct drm_framebuffer *fb, int plane) +{ + if (is_ccs_modifier(fb->modifier)) + return is_ccs_plane(fb, plane); + + return plane == 1; +} + +static int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane) +{ + WARN_ON(!is_ccs_modifier(fb->modifier) || + (main_plane && main_plane >= fb->format->num_planes / 2)); + + return fb->format->num_planes / 2 + main_plane; +} + +/* Return either the main plane's CCS or - if not a CCS FB - UV plane */ +static int +intel_main_to_aux_plane(const struct drm_framebuffer *fb, int main_plane) +{ + if (is_ccs_modifier(fb->modifier)) + return main_to_ccs_plane(fb, main_plane); + + return 1; +} + static unsigned int intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) { @@ -1948,7 +1982,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) else return 512; case I915_FORMAT_MOD_Y_TILED_CCS: - if (color_plane == 1) + if (is_ccs_plane(fb, color_plane)) return 128; /* fall through */ case I915_FORMAT_MOD_Y_TILED: @@ -1957,7 +1991,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) else return 512; case I915_FORMAT_MOD_Yf_TILED_CCS: - if (color_plane == 1) + if (is_ccs_plane(fb, color_plane)) return 128; /* fall through */ case I915_FORMAT_MOD_Yf_TILED: @@ -2074,7 +2108,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, struct drm_i915_private *dev_priv = to_i915(fb->dev); /* AUX_DIST needs only 4K alignment */ - if (color_plane == 1) + if (is_aux_plane(fb, color_plane)) return 4096; switch (fb->modifier) { @@ -3457,10 +3491,11 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state const struct drm_framebuffer *fb = plane_state->hw.fb; int hsub = fb->format->hsub; int vsub = fb->format->vsub; - int aux_x = plane_state->color_plane[1].x; - int aux_y = plane_state->color_plane[1].y; - u32 aux_offset = plane_state->color_plane[1].offset; - u32 alignment = intel_surf_alignment(fb, 1); + int ccs_plane = main_to_ccs_plane(fb, 0); + int aux_x = plane_state->color_plane[ccs_plane].x; + int aux_y = plane_state->color_plane[ccs_plane].y; + u32 aux_offset = plane_state->color_plane[ccs_plane].offset; + u32 alignment = intel_surf_alignment(fb, ccs_plane); while (aux_offset >= main_offset && aux_y <= main_y) { int x, y; @@ -3473,7 +3508,7 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state x = aux_x / hsub; y = aux_y / vsub; - aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, 1, + aux_offset = intel_plane_adjust_aligned_offset(&x, &y, plane_state, ccs_plane, aux_offset, aux_offset - alignment); aux_x = x * hsub + aux_x % hsub; aux_y = y * vsub + aux_y % vsub; @@ -3482,9 +3517,9 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state if (aux_x != main_x || aux_y != main_y) return false; - plane_state->color_plane[1].offset = aux_offset; - plane_state->color_plane[1].x = aux_x; - plane_state->color_plane[1].y = aux_y; + plane_state->color_plane[ccs_plane].offset = aux_offset; + plane_state->color_plane[ccs_plane].x = aux_x; + plane_state->color_plane[ccs_plane].y = aux_y; return true; } @@ -3500,7 +3535,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) int h = drm_rect_height(&plane_state->uapi.src) >> 16; int max_width; int max_height; - u32 alignment, offset, aux_offset = plane_state->color_plane[1].offset; + int aux_plane = intel_main_to_aux_plane(fb, 0); + u32 alignment, offset, aux_offset = plane_state->color_plane[aux_plane].offset; if (INTEL_GEN(dev_priv) >= 11) max_width = icl_max_plane_width(fb, 0, rotation); @@ -3566,7 +3602,8 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) offset, offset - alignment); } - if (x != plane_state->color_plane[1].x || y != plane_state->color_plane[1].y) { + if (x != plane_state->color_plane[aux_plane].x || + y != plane_state->color_plane[aux_plane].y) { DRM_DEBUG_KMS("Unable to find suitable display surface offset due to CCS\n"); return -EINVAL; }
Using helpers instead of open coding this to select a CCS plane for a main plane makes the code cleaner and less error-prone when the location of CCS plane can be different based on the format (packed vs. YUV semiplanar). The same applies to selecting an AUX plane which can be a UV plane (for an uncompressed YUV semiplanar format), or a CCS plane. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 63 ++++++++++++++++---- 1 file changed, 50 insertions(+), 13 deletions(-)