| Message ID | 20180719182214.4323-4-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2018-07-19 at 21:21 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Each plane may have different stride limitations. Let's add a new > plane function to retutn the maximum stride for each plane. There's > going to be some use for this outside the .atomic_check() stuff hence > the separate hook. I just not checked the the spec for the VLV and CHV other than that LGTM but just take a look to the nitpick bellow: Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 46 > ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++ > drivers/gpu/drm/i915/intel_sprite.c | 34 ++++++++++++++++++++++++-- > 3 files changed, 88 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 5f8304a11482..a09e11e0596f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3210,6 +3210,31 @@ int skl_check_plane_surface(const struct > intel_crtc_state *crtc_state, > return 0; > } > > +unsigned int > +i9xx_plane_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + if (INTEL_GEN(dev_priv) >= 4) { > + if (modifier == I915_FORMAT_MOD_X_TILED) > + return 16*1024; > + else > + return 32*1024; > + } else if (INTEL_GEN(dev_priv) >= 3) { > + if (modifier == I915_FORMAT_MOD_X_TILED) > + return 8*1024; > + else > + return 16*1024; > + } else { > + if (plane->i9xx_plane == PLANE_C) > + return 4*1024; > + else > + return 8*1024; > + } > +} > + > static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state) > { > @@ -9672,6 +9697,14 @@ static int intel_check_cursor(struct > intel_crtc_state *crtc_state, > return 0; > } > > +static unsigned int > +i845_cursor_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation) > +{ > + return 2048; > +} > + > static u32 i845_cursor_ctl(const struct intel_crtc_state > *crtc_state, > const struct intel_plane_state *plane_state) > { > @@ -9805,6 +9838,14 @@ static bool i845_cursor_get_hw_state(struct > intel_plane *plane, > return ret; > } > > +static unsigned int > +i9xx_cursor_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation) > +{ > + return plane->base.dev->mode_config.cursor_width * 4; > +} > + > static u32 i9xx_cursor_ctl(const struct intel_crtc_state > *crtc_state, > const struct intel_plane_state *plane_state) > { > @@ -13699,6 +13740,7 @@ intel_primary_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > else > modifiers = skl_format_modifiers_noccs; > > + primary->max_stride = skl_plane_max_stride; > primary->update_plane = skl_update_plane; > primary->disable_plane = skl_disable_plane; > primary->get_hw_state = skl_plane_get_hw_state; > @@ -13709,6 +13751,7 @@ intel_primary_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > num_formats = ARRAY_SIZE(i965_primary_formats); > modifiers = i9xx_format_modifiers; > > + primary->max_stride = i9xx_plane_max_stride; > primary->update_plane = i9xx_update_plane; > primary->disable_plane = i9xx_disable_plane; > primary->get_hw_state = i9xx_plane_get_hw_state; > @@ -13719,6 +13762,7 @@ intel_primary_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > num_formats = ARRAY_SIZE(i8xx_primary_formats); > modifiers = i9xx_format_modifiers; > > + primary->max_stride = i9xx_plane_max_stride; > primary->update_plane = i9xx_update_plane; > primary->disable_plane = i9xx_disable_plane; > primary->get_hw_state = i9xx_plane_get_hw_state; > @@ -13826,11 +13870,13 @@ intel_cursor_plane_create(struct > drm_i915_private *dev_priv, > cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id); > > if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) { > + cursor->max_stride = i845_cursor_max_stride; > cursor->update_plane = i845_update_cursor; > cursor->disable_plane = i845_disable_cursor; > cursor->get_hw_state = i845_cursor_get_hw_state; > cursor->check_plane = i845_check_cursor; > } else { > + cursor->max_stride = i9xx_cursor_max_stride; > cursor->update_plane = i9xx_update_cursor; > cursor->disable_plane = i9xx_disable_cursor; > cursor->get_hw_state = i9xx_cursor_get_hw_state; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index b9f6de3a8f53..ad2bd62ee553 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -963,6 +963,9 @@ struct intel_plane { > * the intel_plane_state structure and accessed via > plane_state. > */ > > + unsigned int (*max_stride)(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation); > void (*update_plane)(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state, > const struct intel_plane_state > *plane_state); > @@ -1652,6 +1655,9 @@ int skl_check_plane_surface(const struct > intel_crtc_state *crtc_state, > struct intel_plane_state *plane_state); > int i9xx_check_plane_surface(struct intel_plane_state *plane_state); > int skl_format_to_fourcc(int format, bool rgb_order, bool alpha); > +unsigned int i9xx_plane_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation); > > /* intel_csr.c */ > void intel_csr_ucode_init(struct drm_i915_private *); > @@ -2102,6 +2108,10 @@ bool skl_plane_has_ccs(struct drm_i915_private > *dev_priv, > enum pipe pipe, enum plane_id plane_id); > bool skl_plane_has_planar(struct drm_i915_private *dev_priv, > enum pipe pipe, enum plane_id plane_id); > +unsigned int skl_plane_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation); > + > > /* intel_tv.c */ > void intel_tv_init(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index f7026e887fa9..e35760814f25 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -228,6 +228,23 @@ void intel_pipe_update_end(struct > intel_crtc_state *new_crtc_state) > #endif > } > > +unsigned int > +skl_plane_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation) > +{ > + int cpp = drm_format_plane_cpp(pixel_format, 0); > + > + /* > + * "The stride in bytes must not exceed the > + * of the size of 8K pixels and 32K bytes." > + */ > + if (drm_rotation_90_or_270(rotation)) > + return min(8192, 32768 / cpp); > + else > + return min(8192 * cpp, 32768); > +} > + > void > skl_update_plane(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state, > @@ -798,6 +815,14 @@ ivb_plane_get_hw_state(struct intel_plane > *plane, > return ret; > } > > +static unsigned int > +g4x_sprite_max_stride(struct intel_plane *plane, > + u32 pixel_format, u64 modifier, > + unsigned int rotation) > +{ > + return 16384; > +} > + > static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state) > { > @@ -964,7 +989,6 @@ intel_check_sprite_plane(struct intel_plane > *plane, > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > struct drm_framebuffer *fb = state->base.fb; > - int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; Why not keep max_stride and call plane->max_stride() here or in a line bellow, this would make less changes a easier 'if' to read and fix the line over 80 characters warning. > int max_scale, min_scale; > bool can_scale; > int ret; > @@ -982,7 +1006,9 @@ intel_check_sprite_plane(struct intel_plane > *plane, > } > > /* FIXME check all gen limits */ > - if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > > max_stride) { > + if (fb->width < 3 || fb->height < 3 || > + fb->pitches[0] > plane->max_stride(plane, fb->format- > >format, > + fb->modifier, > DRM_MODE_ROTATE_0)) { > DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n"); > return -EINVAL; > } > @@ -1528,6 +1554,7 @@ intel_sprite_plane_create(struct > drm_i915_private *dev_priv, > intel_plane->has_ccs = skl_plane_has_ccs(dev_priv, > pipe, > PLANE_SPRITE0 > + plane); > > + intel_plane->max_stride = skl_plane_max_stride; > intel_plane->update_plane = skl_update_plane; > intel_plane->disable_plane = skl_disable_plane; > intel_plane->get_hw_state = skl_plane_get_hw_state; > @@ -1551,6 +1578,7 @@ intel_sprite_plane_create(struct > drm_i915_private *dev_priv, > intel_plane->can_scale = false; > intel_plane->max_downscale = 1; > > + intel_plane->max_stride = i9xx_plane_max_stride; > intel_plane->update_plane = vlv_update_plane; > intel_plane->disable_plane = vlv_disable_plane; > intel_plane->get_hw_state = vlv_plane_get_hw_state; > @@ -1569,6 +1597,7 @@ intel_sprite_plane_create(struct > drm_i915_private *dev_priv, > intel_plane->max_downscale = 1; > } > > + intel_plane->max_stride = g4x_sprite_max_stride; > intel_plane->update_plane = ivb_update_plane; > intel_plane->disable_plane = ivb_disable_plane; > intel_plane->get_hw_state = ivb_plane_get_hw_state; > @@ -1582,6 +1611,7 @@ intel_sprite_plane_create(struct > drm_i915_private *dev_priv, > intel_plane->can_scale = true; > intel_plane->max_downscale = 16; > > + intel_plane->max_stride = g4x_sprite_max_stride; > intel_plane->update_plane = g4x_update_plane; > intel_plane->disable_plane = g4x_disable_plane; > intel_plane->get_hw_state = g4x_plane_get_hw_state;
On Wed, 2018-08-22 at 22:03 +0000, Souza, Jose wrote: > On Thu, 2018-07-19 at 21:21 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Each plane may have different stride limitations. Let's add a new > > plane function to retutn the maximum stride for each plane. There's > > going to be some use for this outside the .atomic_check() stuff > > hence > > the separate hook. > > I just not checked the the spec for the VLV and CHV other than that > LGTM but just take a look to the nitpick bellow: > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 46 > > ++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++ > > drivers/gpu/drm/i915/intel_sprite.c | 34 > > ++++++++++++++++++++++++-- > > 3 files changed, 88 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 5f8304a11482..a09e11e0596f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3210,6 +3210,31 @@ int skl_check_plane_surface(const struct > > intel_crtc_state *crtc_state, > > return 0; > > } > > > > +unsigned int > > +i9xx_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + Reviwing 'drm/i915: Use pipe A primary plane .max_stride() as the global stride limit' I notice that this case was not moved here: } else if (gen >= 5 && !HAS_GMCH_DISPLAY(dev_priv)) { return 32*1024; } Not relevant anymore? > > + if (INTEL_GEN(dev_priv) >= 4) { > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > + return 16*1024; > > + else > > + return 32*1024; > > + } else if (INTEL_GEN(dev_priv) >= 3) { > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > + return 8*1024; > > + else > > + return 16*1024; > > + } else { > > + if (plane->i9xx_plane == PLANE_C) > > + return 4*1024; > > + else > > + return 8*1024; > > + } > > +} > > + > > static u32 i9xx_plane_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -9672,6 +9697,14 @@ static int intel_check_cursor(struct > > intel_crtc_state *crtc_state, > > return 0; > > } > > > > +static unsigned int > > +i845_cursor_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return 2048; > > +} > > + > > static u32 i845_cursor_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -9805,6 +9838,14 @@ static bool i845_cursor_get_hw_state(struct > > intel_plane *plane, > > return ret; > > } > > > > +static unsigned int > > +i9xx_cursor_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return plane->base.dev->mode_config.cursor_width * 4; > > +} > > + > > static u32 i9xx_cursor_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -13699,6 +13740,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > else > > modifiers = skl_format_modifiers_noccs; > > > > + primary->max_stride = skl_plane_max_stride; > > primary->update_plane = skl_update_plane; > > primary->disable_plane = skl_disable_plane; > > primary->get_hw_state = skl_plane_get_hw_state; > > @@ -13709,6 +13751,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > num_formats = ARRAY_SIZE(i965_primary_formats); > > modifiers = i9xx_format_modifiers; > > > > + primary->max_stride = i9xx_plane_max_stride; > > primary->update_plane = i9xx_update_plane; > > primary->disable_plane = i9xx_disable_plane; > > primary->get_hw_state = i9xx_plane_get_hw_state; > > @@ -13719,6 +13762,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > num_formats = ARRAY_SIZE(i8xx_primary_formats); > > modifiers = i9xx_format_modifiers; > > > > + primary->max_stride = i9xx_plane_max_stride; > > primary->update_plane = i9xx_update_plane; > > primary->disable_plane = i9xx_disable_plane; > > primary->get_hw_state = i9xx_plane_get_hw_state; > > @@ -13826,11 +13870,13 @@ intel_cursor_plane_create(struct > > drm_i915_private *dev_priv, > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id); > > > > if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) { > > + cursor->max_stride = i845_cursor_max_stride; > > cursor->update_plane = i845_update_cursor; > > cursor->disable_plane = i845_disable_cursor; > > cursor->get_hw_state = i845_cursor_get_hw_state; > > cursor->check_plane = i845_check_cursor; > > } else { > > + cursor->max_stride = i9xx_cursor_max_stride; > > cursor->update_plane = i9xx_update_cursor; > > cursor->disable_plane = i9xx_disable_cursor; > > cursor->get_hw_state = i9xx_cursor_get_hw_state; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index b9f6de3a8f53..ad2bd62ee553 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -963,6 +963,9 @@ struct intel_plane { > > * the intel_plane_state structure and accessed via > > plane_state. > > */ > > > > + unsigned int (*max_stride)(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > void (*update_plane)(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state, > > const struct intel_plane_state > > *plane_state); > > @@ -1652,6 +1655,9 @@ int skl_check_plane_surface(const struct > > intel_crtc_state *crtc_state, > > struct intel_plane_state *plane_state); > > int i9xx_check_plane_surface(struct intel_plane_state > > *plane_state); > > int skl_format_to_fourcc(int format, bool rgb_order, bool alpha); > > +unsigned int i9xx_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > > > /* intel_csr.c */ > > void intel_csr_ucode_init(struct drm_i915_private *); > > @@ -2102,6 +2108,10 @@ bool skl_plane_has_ccs(struct > > drm_i915_private > > *dev_priv, > > enum pipe pipe, enum plane_id plane_id); > > bool skl_plane_has_planar(struct drm_i915_private *dev_priv, > > enum pipe pipe, enum plane_id plane_id); > > +unsigned int skl_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > + > > > > /* intel_tv.c */ > > void intel_tv_init(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index f7026e887fa9..e35760814f25 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -228,6 +228,23 @@ void intel_pipe_update_end(struct > > intel_crtc_state *new_crtc_state) > > #endif > > } > > > > +unsigned int > > +skl_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + int cpp = drm_format_plane_cpp(pixel_format, 0); > > + > > + /* > > + * "The stride in bytes must not exceed the > > + * of the size of 8K pixels and 32K bytes." > > + */ > > + if (drm_rotation_90_or_270(rotation)) > > + return min(8192, 32768 / cpp); > > + else > > + return min(8192 * cpp, 32768); > > +} > > + > > void > > skl_update_plane(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state, > > @@ -798,6 +815,14 @@ ivb_plane_get_hw_state(struct intel_plane > > *plane, > > return ret; > > } > > > > +static unsigned int > > +g4x_sprite_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return 16384; > > +} > > + > > static u32 g4x_sprite_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -964,7 +989,6 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > struct drm_framebuffer *fb = state->base.fb; > > - int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; > > Why not keep max_stride and call plane->max_stride() here or in a > line > bellow, this would make less changes a easier 'if' to read and fix > the > line over 80 characters warning. > > > int max_scale, min_scale; > > bool can_scale; > > int ret; > > @@ -982,7 +1006,9 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > } > > > > /* FIXME check all gen limits */ > > - if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > > > max_stride) { > > + if (fb->width < 3 || fb->height < 3 || > > + fb->pitches[0] > plane->max_stride(plane, fb->format- > > > format, > > > > + fb->modifier, > > DRM_MODE_ROTATE_0)) { > > DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n"); > > return -EINVAL; > > } > > @@ -1528,6 +1554,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->has_ccs = skl_plane_has_ccs(dev_priv, > > pipe, > > PLANE_SPRITE0 > > + plane); > > > > + intel_plane->max_stride = skl_plane_max_stride; > > intel_plane->update_plane = skl_update_plane; > > intel_plane->disable_plane = skl_disable_plane; > > intel_plane->get_hw_state = skl_plane_get_hw_state; > > @@ -1551,6 +1578,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->can_scale = false; > > intel_plane->max_downscale = 1; > > > > + intel_plane->max_stride = i9xx_plane_max_stride; > > intel_plane->update_plane = vlv_update_plane; > > intel_plane->disable_plane = vlv_disable_plane; > > intel_plane->get_hw_state = vlv_plane_get_hw_state; > > @@ -1569,6 +1597,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->max_downscale = 1; > > } > > > > + intel_plane->max_stride = g4x_sprite_max_stride; > > intel_plane->update_plane = ivb_update_plane; > > intel_plane->disable_plane = ivb_disable_plane; > > intel_plane->get_hw_state = ivb_plane_get_hw_state; > > @@ -1582,6 +1611,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->can_scale = true; > > intel_plane->max_downscale = 16; > > > > + intel_plane->max_stride = g4x_sprite_max_stride; > > intel_plane->update_plane = g4x_update_plane; > > intel_plane->disable_plane = g4x_disable_plane; > > intel_plane->get_hw_state = g4x_plane_get_hw_state; > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 22, 2018 at 10:19:55PM +0000, Souza, Jose wrote: > On Wed, 2018-08-22 at 22:03 +0000, Souza, Jose wrote: > > On Thu, 2018-07-19 at 21:21 +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Each plane may have different stride limitations. Let's add a new > > > plane function to retutn the maximum stride for each plane. There's > > > going to be some use for this outside the .atomic_check() stuff > > > hence > > > the separate hook. > > > > I just not checked the the spec for the VLV and CHV other than that > > LGTM but just take a look to the nitpick bellow: > > > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 46 > > > ++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++ > > > drivers/gpu/drm/i915/intel_sprite.c | 34 > > > ++++++++++++++++++++++++-- > > > 3 files changed, 88 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 5f8304a11482..a09e11e0596f 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3210,6 +3210,31 @@ int skl_check_plane_surface(const struct > > > intel_crtc_state *crtc_state, > > > return 0; > > > } > > > > > > +unsigned int > > > +i9xx_plane_max_stride(struct intel_plane *plane, > > > + u32 pixel_format, u64 modifier, > > > + unsigned int rotation) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > Reviwing 'drm/i915: Use pipe A primary plane .max_stride() as the > global stride limit' I notice that this case was not moved here: > > } else if (gen >= 5 && !HAS_GMCH_DISPLAY(dev_priv)) { > return 32*1024; > } > > Not relevant anymore? No, that should not have been dropped. Good catch. I'll respin. > > > > > + if (INTEL_GEN(dev_priv) >= 4) { > > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > > + return 16*1024; > > > + else > > > + return 32*1024; > > > + } else if (INTEL_GEN(dev_priv) >= 3) { > > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > > + return 8*1024; > > > + else > > > + return 16*1024; > > > + } else { > > > + if (plane->i9xx_plane == PLANE_C) > > > + return 4*1024; > > > + else > > > + return 8*1024; > > > + } > > > +} > > > +
On Wed, Aug 22, 2018 at 10:03:17PM +0000, Souza, Jose wrote: > On Thu, 2018-07-19 at 21:21 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Each plane may have different stride limitations. Let's add a new > > plane function to retutn the maximum stride for each plane. There's > > going to be some use for this outside the .atomic_check() stuff hence > > the separate hook. > > I just not checked the the spec for the VLV and CHV other than that > LGTM but just take a look to the nitpick bellow: > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 46 > > ++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++ > > drivers/gpu/drm/i915/intel_sprite.c | 34 ++++++++++++++++++++++++-- > > 3 files changed, 88 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 5f8304a11482..a09e11e0596f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3210,6 +3210,31 @@ int skl_check_plane_surface(const struct > > intel_crtc_state *crtc_state, > > return 0; > > } > > > > +unsigned int > > +i9xx_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + if (INTEL_GEN(dev_priv) >= 4) { > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > + return 16*1024; > > + else > > + return 32*1024; > > + } else if (INTEL_GEN(dev_priv) >= 3) { > > + if (modifier == I915_FORMAT_MOD_X_TILED) > > + return 8*1024; > > + else > > + return 16*1024; > > + } else { > > + if (plane->i9xx_plane == PLANE_C) > > + return 4*1024; > > + else > > + return 8*1024; > > + } > > +} > > + > > static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -9672,6 +9697,14 @@ static int intel_check_cursor(struct > > intel_crtc_state *crtc_state, > > return 0; > > } > > > > +static unsigned int > > +i845_cursor_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return 2048; > > +} > > + > > static u32 i845_cursor_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -9805,6 +9838,14 @@ static bool i845_cursor_get_hw_state(struct > > intel_plane *plane, > > return ret; > > } > > > > +static unsigned int > > +i9xx_cursor_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return plane->base.dev->mode_config.cursor_width * 4; > > +} > > + > > static u32 i9xx_cursor_ctl(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -13699,6 +13740,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > else > > modifiers = skl_format_modifiers_noccs; > > > > + primary->max_stride = skl_plane_max_stride; > > primary->update_plane = skl_update_plane; > > primary->disable_plane = skl_disable_plane; > > primary->get_hw_state = skl_plane_get_hw_state; > > @@ -13709,6 +13751,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > num_formats = ARRAY_SIZE(i965_primary_formats); > > modifiers = i9xx_format_modifiers; > > > > + primary->max_stride = i9xx_plane_max_stride; > > primary->update_plane = i9xx_update_plane; > > primary->disable_plane = i9xx_disable_plane; > > primary->get_hw_state = i9xx_plane_get_hw_state; > > @@ -13719,6 +13762,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > num_formats = ARRAY_SIZE(i8xx_primary_formats); > > modifiers = i9xx_format_modifiers; > > > > + primary->max_stride = i9xx_plane_max_stride; > > primary->update_plane = i9xx_update_plane; > > primary->disable_plane = i9xx_disable_plane; > > primary->get_hw_state = i9xx_plane_get_hw_state; > > @@ -13826,11 +13870,13 @@ intel_cursor_plane_create(struct > > drm_i915_private *dev_priv, > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id); > > > > if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) { > > + cursor->max_stride = i845_cursor_max_stride; > > cursor->update_plane = i845_update_cursor; > > cursor->disable_plane = i845_disable_cursor; > > cursor->get_hw_state = i845_cursor_get_hw_state; > > cursor->check_plane = i845_check_cursor; > > } else { > > + cursor->max_stride = i9xx_cursor_max_stride; > > cursor->update_plane = i9xx_update_cursor; > > cursor->disable_plane = i9xx_disable_cursor; > > cursor->get_hw_state = i9xx_cursor_get_hw_state; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index b9f6de3a8f53..ad2bd62ee553 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -963,6 +963,9 @@ struct intel_plane { > > * the intel_plane_state structure and accessed via > > plane_state. > > */ > > > > + unsigned int (*max_stride)(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > void (*update_plane)(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state, > > const struct intel_plane_state > > *plane_state); > > @@ -1652,6 +1655,9 @@ int skl_check_plane_surface(const struct > > intel_crtc_state *crtc_state, > > struct intel_plane_state *plane_state); > > int i9xx_check_plane_surface(struct intel_plane_state *plane_state); > > int skl_format_to_fourcc(int format, bool rgb_order, bool alpha); > > +unsigned int i9xx_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > > > /* intel_csr.c */ > > void intel_csr_ucode_init(struct drm_i915_private *); > > @@ -2102,6 +2108,10 @@ bool skl_plane_has_ccs(struct drm_i915_private > > *dev_priv, > > enum pipe pipe, enum plane_id plane_id); > > bool skl_plane_has_planar(struct drm_i915_private *dev_priv, > > enum pipe pipe, enum plane_id plane_id); > > +unsigned int skl_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation); > > + > > > > /* intel_tv.c */ > > void intel_tv_init(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index f7026e887fa9..e35760814f25 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -228,6 +228,23 @@ void intel_pipe_update_end(struct > > intel_crtc_state *new_crtc_state) > > #endif > > } > > > > +unsigned int > > +skl_plane_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + int cpp = drm_format_plane_cpp(pixel_format, 0); > > + > > + /* > > + * "The stride in bytes must not exceed the > > + * of the size of 8K pixels and 32K bytes." > > + */ > > + if (drm_rotation_90_or_270(rotation)) > > + return min(8192, 32768 / cpp); > > + else > > + return min(8192 * cpp, 32768); > > +} > > + > > void > > skl_update_plane(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state, > > @@ -798,6 +815,14 @@ ivb_plane_get_hw_state(struct intel_plane > > *plane, > > return ret; > > } > > > > +static unsigned int > > +g4x_sprite_max_stride(struct intel_plane *plane, > > + u32 pixel_format, u64 modifier, > > + unsigned int rotation) > > +{ > > + return 16384; > > +} > > + > > static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state, > > const struct intel_plane_state *plane_state) > > { > > @@ -964,7 +989,6 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > struct drm_framebuffer *fb = state->base.fb; > > - int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; > > Why not keep max_stride and call plane->max_stride() here or in a line > bellow, this would make less changes a easier 'if' to read and fix the > line over 80 characters warning. I guess we could do that. Although this code is going away later anyway so doesn't matter too much. > > > int max_scale, min_scale; > > bool can_scale; > > int ret; > > @@ -982,7 +1006,9 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > } > > > > /* FIXME check all gen limits */ > > - if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > > > max_stride) { > > + if (fb->width < 3 || fb->height < 3 || > > + fb->pitches[0] > plane->max_stride(plane, fb->format- > > >format, > > + fb->modifier, > > DRM_MODE_ROTATE_0)) { > > DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n"); > > return -EINVAL; > > } > > @@ -1528,6 +1554,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->has_ccs = skl_plane_has_ccs(dev_priv, > > pipe, > > PLANE_SPRITE0 > > + plane); > > > > + intel_plane->max_stride = skl_plane_max_stride; > > intel_plane->update_plane = skl_update_plane; > > intel_plane->disable_plane = skl_disable_plane; > > intel_plane->get_hw_state = skl_plane_get_hw_state; > > @@ -1551,6 +1578,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->can_scale = false; > > intel_plane->max_downscale = 1; > > > > + intel_plane->max_stride = i9xx_plane_max_stride; > > intel_plane->update_plane = vlv_update_plane; > > intel_plane->disable_plane = vlv_disable_plane; > > intel_plane->get_hw_state = vlv_plane_get_hw_state; > > @@ -1569,6 +1597,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->max_downscale = 1; > > } > > > > + intel_plane->max_stride = g4x_sprite_max_stride; > > intel_plane->update_plane = ivb_update_plane; > > intel_plane->disable_plane = ivb_disable_plane; > > intel_plane->get_hw_state = ivb_plane_get_hw_state; > > @@ -1582,6 +1611,7 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > intel_plane->can_scale = true; > > intel_plane->max_downscale = 16; > > > > + intel_plane->max_stride = g4x_sprite_max_stride; > > intel_plane->update_plane = g4x_update_plane; > > intel_plane->disable_plane = g4x_disable_plane; > > intel_plane->get_hw_state = g4x_plane_get_hw_state;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5f8304a11482..a09e11e0596f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3210,6 +3210,31 @@ int skl_check_plane_surface(const struct intel_crtc_state *crtc_state, return 0; } +unsigned int +i9xx_plane_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + if (INTEL_GEN(dev_priv) >= 4) { + if (modifier == I915_FORMAT_MOD_X_TILED) + return 16*1024; + else + return 32*1024; + } else if (INTEL_GEN(dev_priv) >= 3) { + if (modifier == I915_FORMAT_MOD_X_TILED) + return 8*1024; + else + return 16*1024; + } else { + if (plane->i9xx_plane == PLANE_C) + return 4*1024; + else + return 8*1024; + } +} + static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { @@ -9672,6 +9697,14 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state, return 0; } +static unsigned int +i845_cursor_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation) +{ + return 2048; +} + static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { @@ -9805,6 +9838,14 @@ static bool i845_cursor_get_hw_state(struct intel_plane *plane, return ret; } +static unsigned int +i9xx_cursor_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation) +{ + return plane->base.dev->mode_config.cursor_width * 4; +} + static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { @@ -13699,6 +13740,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) else modifiers = skl_format_modifiers_noccs; + primary->max_stride = skl_plane_max_stride; primary->update_plane = skl_update_plane; primary->disable_plane = skl_disable_plane; primary->get_hw_state = skl_plane_get_hw_state; @@ -13709,6 +13751,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) num_formats = ARRAY_SIZE(i965_primary_formats); modifiers = i9xx_format_modifiers; + primary->max_stride = i9xx_plane_max_stride; primary->update_plane = i9xx_update_plane; primary->disable_plane = i9xx_disable_plane; primary->get_hw_state = i9xx_plane_get_hw_state; @@ -13719,6 +13762,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) num_formats = ARRAY_SIZE(i8xx_primary_formats); modifiers = i9xx_format_modifiers; + primary->max_stride = i9xx_plane_max_stride; primary->update_plane = i9xx_update_plane; primary->disable_plane = i9xx_disable_plane; primary->get_hw_state = i9xx_plane_get_hw_state; @@ -13826,11 +13870,13 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id); if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) { + cursor->max_stride = i845_cursor_max_stride; cursor->update_plane = i845_update_cursor; cursor->disable_plane = i845_disable_cursor; cursor->get_hw_state = i845_cursor_get_hw_state; cursor->check_plane = i845_check_cursor; } else { + cursor->max_stride = i9xx_cursor_max_stride; cursor->update_plane = i9xx_update_cursor; cursor->disable_plane = i9xx_disable_cursor; cursor->get_hw_state = i9xx_cursor_get_hw_state; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b9f6de3a8f53..ad2bd62ee553 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -963,6 +963,9 @@ struct intel_plane { * the intel_plane_state structure and accessed via plane_state. */ + unsigned int (*max_stride)(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation); void (*update_plane)(struct intel_plane *plane, const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state); @@ -1652,6 +1655,9 @@ int skl_check_plane_surface(const struct intel_crtc_state *crtc_state, struct intel_plane_state *plane_state); int i9xx_check_plane_surface(struct intel_plane_state *plane_state); int skl_format_to_fourcc(int format, bool rgb_order, bool alpha); +unsigned int i9xx_plane_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation); /* intel_csr.c */ void intel_csr_ucode_init(struct drm_i915_private *); @@ -2102,6 +2108,10 @@ bool skl_plane_has_ccs(struct drm_i915_private *dev_priv, enum pipe pipe, enum plane_id plane_id); bool skl_plane_has_planar(struct drm_i915_private *dev_priv, enum pipe pipe, enum plane_id plane_id); +unsigned int skl_plane_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation); + /* intel_tv.c */ void intel_tv_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index f7026e887fa9..e35760814f25 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -228,6 +228,23 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) #endif } +unsigned int +skl_plane_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation) +{ + int cpp = drm_format_plane_cpp(pixel_format, 0); + + /* + * "The stride in bytes must not exceed the + * of the size of 8K pixels and 32K bytes." + */ + if (drm_rotation_90_or_270(rotation)) + return min(8192, 32768 / cpp); + else + return min(8192 * cpp, 32768); +} + void skl_update_plane(struct intel_plane *plane, const struct intel_crtc_state *crtc_state, @@ -798,6 +815,14 @@ ivb_plane_get_hw_state(struct intel_plane *plane, return ret; } +static unsigned int +g4x_sprite_max_stride(struct intel_plane *plane, + u32 pixel_format, u64 modifier, + unsigned int rotation) +{ + return 16384; +} + static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { @@ -964,7 +989,6 @@ intel_check_sprite_plane(struct intel_plane *plane, struct drm_i915_private *dev_priv = to_i915(plane->base.dev); struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_framebuffer *fb = state->base.fb; - int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384; int max_scale, min_scale; bool can_scale; int ret; @@ -982,7 +1006,9 @@ intel_check_sprite_plane(struct intel_plane *plane, } /* FIXME check all gen limits */ - if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > max_stride) { + if (fb->width < 3 || fb->height < 3 || + fb->pitches[0] > plane->max_stride(plane, fb->format->format, + fb->modifier, DRM_MODE_ROTATE_0)) { DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n"); return -EINVAL; } @@ -1528,6 +1554,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, intel_plane->has_ccs = skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0 + plane); + intel_plane->max_stride = skl_plane_max_stride; intel_plane->update_plane = skl_update_plane; intel_plane->disable_plane = skl_disable_plane; intel_plane->get_hw_state = skl_plane_get_hw_state; @@ -1551,6 +1578,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, intel_plane->can_scale = false; intel_plane->max_downscale = 1; + intel_plane->max_stride = i9xx_plane_max_stride; intel_plane->update_plane = vlv_update_plane; intel_plane->disable_plane = vlv_disable_plane; intel_plane->get_hw_state = vlv_plane_get_hw_state; @@ -1569,6 +1597,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, intel_plane->max_downscale = 1; } + intel_plane->max_stride = g4x_sprite_max_stride; intel_plane->update_plane = ivb_update_plane; intel_plane->disable_plane = ivb_disable_plane; intel_plane->get_hw_state = ivb_plane_get_hw_state; @@ -1582,6 +1611,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, intel_plane->can_scale = true; intel_plane->max_downscale = 16; + intel_plane->max_stride = g4x_sprite_max_stride; intel_plane->update_plane = g4x_update_plane; intel_plane->disable_plane = g4x_disable_plane; intel_plane->get_hw_state = g4x_plane_get_hw_state;