| Message ID | 1440032556-9920-12-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 19, 2015 at 06:02:32PM -0700, Chandra Konduru wrote: > This patch is adding NV12 support to skylake primary plane > programming. It is covering linear/X/Y/Yf tiling formats > for 0 and 180 rotations. > > For 90/270 rotation, Y and UV subplanes should be treated > as separate surfaces and GTT remapping for rotation should > be done separately for each subplane. Once GEM adds support > for seperate remappings for two subplanes, 90/270 support > to be added to plane programming. > > v2: > -Use regular int instead of 16.16 in aux_offset calculations (me) > > v3: > -Allow 90/270 for NV12 as its remapping is now supported (me) > > v4: > -Rebased to current kernel version 4.2.0.rc4 (me) > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Testcase: igt/kms_nv12 > --- > drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4df4d77..329651e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3026,6 +3026,8 @@ u32 skl_plane_ctl_format(uint32_t pixel_format) > return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY; > case DRM_FORMAT_VYUY: > return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY; > + case DRM_FORMAT_NV12: > + return PLANE_CTL_FORMAT_NV12; > default: > MISSING_CASE(pixel_format); > } > @@ -3094,6 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > int src_x = 0, src_y = 0, src_w = 0, src_h = 0; > int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0; > int scaler_id = -1; > + u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0; > + u32 tile_row_adjustment = 0; > > plane_state = to_intel_plane_state(plane->state); > > @@ -3150,11 +3154,34 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > x_offset = stride * tile_height - y - src_h; > y_offset = x; > plane_size = (src_w - 1) << 16 | (src_h - 1); > + /* > + * TBD: For NV12 90/270 rotation, Y and UV subplanes should > + * be treated as separate surfaces and GTT remapping for > + * rotation should be done separately for each subplane. > + * Enable support once seperate remappings are available. > + */ > } else { > stride = fb->pitches[0] / stride_div; > x_offset = x; > y_offset = y; > plane_size = (src_h - 1) << 16 | (src_w - 1); > + tile_height = PAGE_SIZE / stride_div; > + > + if (fb->pixel_format == DRM_FORMAT_NV12) { > + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); > + /* > + * If UV starts from middle of a page, then UV start should > + * be programmed to beginning of that page. And offset into that > + * page to be programmed into y-offset > + */ > + tile_row_adjustment = height_in_mem % tile_height; > + aux_dist = fb->pitches[0] * (height_in_mem - tile_row_adjustment); > + aux_x_offset = DIV_ROUND_UP(x, 2); > + aux_y_offset = DIV_ROUND_UP(y, 2) + tile_row_adjustment; > + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */ > + aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ? > + stride / 2 : stride; The 2x part was rather well hidden in the spec. How do we deal with cases when the Y stride is an odd number of tiles? > + } > } > plane_offset = y_offset << 16 | x_offset; > > @@ -3162,11 +3189,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); > I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); > I915_WRITE(PLANE_STRIDE(pipe, 0), stride); > + I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride); > + I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset); > > if (scaler_id >= 0) { > uint32_t ps_ctrl = 0; > > WARN_ON(!dst_w || !dst_h); > + > ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) | > crtc_state->scaler_state.scalers[scaler_id].mode; > I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl); > @@ -3175,6 +3205,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h); > I915_WRITE(PLANE_POS(pipe, 0), 0); > } else { > + WARN_ON(fb->pixel_format == DRM_FORMAT_NV12); > I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x); > } > > @@ -11626,6 +11657,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > bool turn_off, turn_on, visible, was_visible; > struct drm_framebuffer *fb = plane_state->fb; > > + /* Adjust (macro)pixel boundary */ > + if (fb && intel_format_is_yuv(fb->pixel_format)) { > + to_intel_plane_state(plane_state)->src.x1 &= ~0x10000; > + to_intel_plane_state(plane_state)->src.x2 &= ~0x10000; > + } That can increase the size of the source rectangle, so it needs to be done in a way that avois that. And we already do this for sprites. Really, someone should just finally fix the mess we have made with handling different types of planes in totally different ways and unify things as much as possible. > + > if (crtc_state && INTEL_INFO(dev)->gen >= 9 && > plane->type != DRM_PLANE_TYPE_CURSOR) { > ret = skl_update_scaler_plane( > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 04, 2015 at 02:09:41PM +0300, Ville Syrjälä wrote: > Really, someone should just finally fix the mess we have made with handling > different types of planes in totally different ways and unify things as > much as possible. Yeah I want universal planes for skl finally, and I'd like to have someon on the hook and commited to do it before I pull in this series. We pushed this task off for almost a year by now, and it's been endlessly pain. -Daniel
> > + > > + if (fb->pixel_format == DRM_FORMAT_NV12) { > > + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); > > + /* > > + * If UV starts from middle of a page, then UV start > should > > + * be programmed to beginning of that page. And offset > into that > > + * page to be programmed into y-offset > > + */ > > + tile_row_adjustment = height_in_mem % tile_height; > > + aux_dist = fb->pitches[0] * (height_in_mem - > tile_row_adjustment); > > + aux_x_offset = DIV_ROUND_UP(x, 2); > > + aux_y_offset = DIV_ROUND_UP(y, 2) + > tile_row_adjustment; > > + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane > */ > > + aux_stride = fb->modifier[0] == > I915_FORMAT_MOD_Yf_TILED ? > > + stride / 2 : stride; > > The 2x part was rather well hidden in the spec. How do we deal with > cases when the Y stride is an odd number of tiles? It should be a round up division to take care of that scenario. Fixing this and resolving your other comments. Will be sending updated patches shortly.
On Sat, Sep 05, 2015 at 01:10:11AM +0000, Konduru, Chandra wrote: > > > + > > > + if (fb->pixel_format == DRM_FORMAT_NV12) { > > > + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); > > > + /* > > > + * If UV starts from middle of a page, then UV start > > should > > > + * be programmed to beginning of that page. And offset > > into that > > > + * page to be programmed into y-offset > > > + */ > > > + tile_row_adjustment = height_in_mem % tile_height; > > > + aux_dist = fb->pitches[0] * (height_in_mem - > > tile_row_adjustment); > > > + aux_x_offset = DIV_ROUND_UP(x, 2); > > > + aux_y_offset = DIV_ROUND_UP(y, 2) + > > tile_row_adjustment; > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane > > */ > > > + aux_stride = fb->modifier[0] == > > I915_FORMAT_MOD_Yf_TILED ? > > > + stride / 2 : stride; > > > > The 2x part was rather well hidden in the spec. How do we deal with > > cases when the Y stride is an odd number of tiles? > > It should be a round up division to take care of that scenario. That would stil lresult in a corrupted picture I think. So I was thinking that we should just refuse to create NCV12 framebuffers with a poorly aligned stride. > Fixing this and resolving your other comments. > Will be sending updated patches shortly.
> > > > + if (fb->pixel_format == DRM_FORMAT_NV12) { > > > > + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); > > > > + /* > > > > + * If UV starts from middle of a page, then UV start > > > should > > > > + * be programmed to beginning of that page. And offset > > > into that > > > > + * page to be programmed into y-offset > > > > + */ > > > > + tile_row_adjustment = height_in_mem % tile_height; > > > > + aux_dist = fb->pitches[0] * (height_in_mem - > > > tile_row_adjustment); > > > > + aux_x_offset = DIV_ROUND_UP(x, 2); > > > > + aux_y_offset = DIV_ROUND_UP(y, 2) + > > > tile_row_adjustment; > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane > > > */ > > > > + aux_stride = fb->modifier[0] == > > > I915_FORMAT_MOD_Yf_TILED ? > > > > + stride / 2 : stride; > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > cases when the Y stride is an odd number of tiles? > > > > It should be a round up division to take care of that scenario. > > That would stil lresult in a corrupted picture I think. So I was > thinking that we should just refuse to create NCV12 framebuffers with a > poorly aligned stride. > I added a check in intel_framebuffer_init() which should catch them: if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { DRM_DEBUG("y and uv subplanes have different pitches\n"); return -EINVAL; }
On Wed, Sep 09, 2015 at 02:30:58AM +0300, Konduru, Chandra wrote: > > > > > + if (fb->pixel_format == DRM_FORMAT_NV12) { > > > > > + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); > > > > > + /* > > > > > + * If UV starts from middle of a page, then UV start > > > > should > > > > > + * be programmed to beginning of that page. And offset > > > > into that > > > > > + * page to be programmed into y-offset > > > > > + */ > > > > > + tile_row_adjustment = height_in_mem % tile_height; > > > > > + aux_dist = fb->pitches[0] * (height_in_mem - > > > > tile_row_adjustment); > > > > > + aux_x_offset = DIV_ROUND_UP(x, 2); > > > > > + aux_y_offset = DIV_ROUND_UP(y, 2) + > > > > tile_row_adjustment; > > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane > > > > */ > > > > > + aux_stride = fb->modifier[0] == > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > + stride / 2 : stride; > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > > cases when the Y stride is an odd number of tiles? > > > > > > It should be a round up division to take care of that scenario. > > > > That would stil lresult in a corrupted picture I think. So I was > > thinking that we should just refuse to create NCV12 framebuffers with a > > poorly aligned stride. > > > I added a check in intel_framebuffer_init() which should catch them: > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > DRM_DEBUG("y and uv subplanes have different pitches\n"); > return -EINVAL; > } That won't catch the case I'm worried about. We would also need to make sure pitches[1] is aligned to the UV tile width.
> > > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y- > subplane > > > > > */ > > > > > > + aux_stride = fb->modifier[0] == > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > + stride / 2 : stride; > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > thinking that we should just refuse to create NCV12 framebuffers with a > > > poorly aligned stride. > > > > > I added a check in intel_framebuffer_init() which should catch them: > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > return -EINVAL; > > } > > That won't catch the case I'm worried about. We would also need to make > sure pitches[1] is aligned to the UV tile width. If caller is following tile/row/pitch alignments properly for sub-planes of NV12 Yf buffer, above check will catch. But are you referring a case where userland isn't following tile/row size alignments properly? In that case, above may not catch. But isn't that is the case even with other FB formats if user land not following tile/row/pitch alignments? > > -- > Ville Syrjälä > Intel OTC
On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote: > > > > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y- > > subplane > > > > > > */ > > > > > > > + aux_stride = fb->modifier[0] == > > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > > + stride / 2 : stride; > > > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > > thinking that we should just refuse to create NCV12 framebuffers with a > > > > poorly aligned stride. > > > > > > > I added a check in intel_framebuffer_init() which should catch them: > > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > > return -EINVAL; > > > } > > > > That won't catch the case I'm worried about. We would also need to make > > sure pitches[1] is aligned to the UV tile width. > > If caller is following tile/row/pitch alignments properly for sub-planes of > NV12 Yf buffer, above check will catch. > But are you referring a case where userland isn't following tile/row size > alignments properly? In that case, above may not catch. But > isn't that is the case even with other FB formats if user land not > following tile/row/pitch alignments? We reject any attempt to create a framebuffer with a poorly aligned stride. Hmm. Actually I suppose we should just handle it in intel_fb_stride_alignment(). Eg.: case Yf: if (cpp != 1 || pixel_format == DRM_FORMAT_NV12) return 128; else return 64;
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, September 09, 2015 11:06 AM > To: Konduru, Chandra > Cc: Syrjala, Ville; intel-gfx@lists.freedesktop.org; Vetter, Daniel > Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane > programming. > > On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote: > > > > > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y- > > > subplane > > > > > > > */ > > > > > > > > + aux_stride = fb->modifier[0] == > > > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > > > + stride / 2 : stride; > > > > > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > > > thinking that we should just refuse to create NCV12 framebuffers with a > > > > > poorly aligned stride. > > > > > > > > > I added a check in intel_framebuffer_init() which should catch them: > > > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > > > return -EINVAL; > > > > } > > > > > > That won't catch the case I'm worried about. We would also need to make > > > sure pitches[1] is aligned to the UV tile width. > > > > If caller is following tile/row/pitch alignments properly for sub-planes of > > NV12 Yf buffer, above check will catch. > > But are you referring a case where userland isn't following tile/row size > > alignments properly? In that case, above may not catch. But > > isn't that is the case even with other FB formats if user land not > > following tile/row/pitch alignments? > > We reject any attempt to create a framebuffer with a poorly aligned > stride. > > Hmm. Actually I suppose we should just handle it in > intel_fb_stride_alignment(). Eg.: > > case Yf: > if (cpp != 1 || pixel_format == DRM_FORMAT_NV12) > return 128; > else > return 64; This check is already there in intel_fb_stride_alignment() which is called from intel_framebuffer_init(). But it always does for 1st sub-plane which means does for Y. It needs an update to pass a sub-plane (for UV) parameter, and made another call for UV-plane alignment check. Will this be ok? > > -- > Ville Syrjälä > Intel OTC
On Wed, Sep 09, 2015 at 08:10:30PM +0000, Konduru, Chandra wrote: > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Wednesday, September 09, 2015 11:06 AM > > To: Konduru, Chandra > > Cc: Syrjala, Ville; intel-gfx@lists.freedesktop.org; Vetter, Daniel > > Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane > > programming. > > > > On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote: > > > > > > > > > + /* For tile-Yf, uv-subplane tile width is 2x of Y- > > > > subplane > > > > > > > > */ > > > > > > > > > + aux_stride = fb->modifier[0] == > > > > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > > > > + stride / 2 : stride; > > > > > > > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with > > > > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > > > > thinking that we should just refuse to create NCV12 framebuffers with a > > > > > > poorly aligned stride. > > > > > > > > > > > I added a check in intel_framebuffer_init() which should catch them: > > > > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > > > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > > > > return -EINVAL; > > > > > } > > > > > > > > That won't catch the case I'm worried about. We would also need to make > > > > sure pitches[1] is aligned to the UV tile width. > > > > > > If caller is following tile/row/pitch alignments properly for sub-planes of > > > NV12 Yf buffer, above check will catch. > > > But are you referring a case where userland isn't following tile/row size > > > alignments properly? In that case, above may not catch. But > > > isn't that is the case even with other FB formats if user land not > > > following tile/row/pitch alignments? > > > > We reject any attempt to create a framebuffer with a poorly aligned > > stride. > > > > Hmm. Actually I suppose we should just handle it in > > intel_fb_stride_alignment(). Eg.: > > > > case Yf: > > if (cpp != 1 || pixel_format == DRM_FORMAT_NV12) > > return 128; > > else > > return 64; > > This check is already there in intel_fb_stride_alignment() > which is called from intel_framebuffer_init(). But it always > does for 1st sub-plane which means does for Y. > It needs an update to pass a sub-plane (for UV) parameter, > and made another call for UV-plane alignment check. > Will this be ok? Yeah. You could in fact just have it loop over all the planes and call it for each. Something like this perhaps: for (i = 0; i < drm_format_num_planes(formt); i++) { intel_fb_stride_alignment(modifier[i], format, i); ... } Or you could pass the cpp/bits_per_pixel instead of the plane index, since that's the only thing for which you need the plane index within the function. I also started to wonder whether we should repeat most of the other checks in intel_framebuffer_init() for each plane. But it's probably just easier to check that handle, pitch and modifier matches for both planes in the NV12 case. I think you actually missed the modifier check. You just checked that modifier[1] is Yf, but that leaves modifier[0] unchecked. I failed to notice it as well during my review of the relevant patch.
> > > > > > > > > > + /* For tile-Yf, uv-subplane tile width is > 2x of Y- > > > > > subplane > > > > > > > > > */ > > > > > > > > > > + aux_stride = fb->modifier[0] == > > > > > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > > > > > + stride / 2 : stride; > > > > > > > > > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal > with > > > > > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > > > > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > > > > > thinking that we should just refuse to create NCV12 framebuffers > with a > > > > > > > poorly aligned stride. > > > > > > > > > > > > > I added a check in intel_framebuffer_init() which should catch them: > > > > > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > > > > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > That won't catch the case I'm worried about. We would also need to > make > > > > > sure pitches[1] is aligned to the UV tile width. > > > > > > > > If caller is following tile/row/pitch alignments properly for sub-planes of > > > > NV12 Yf buffer, above check will catch. > > > > But are you referring a case where userland isn't following tile/row size > > > > alignments properly? In that case, above may not catch. But > > > > isn't that is the case even with other FB formats if user land not > > > > following tile/row/pitch alignments? > > > > > > We reject any attempt to create a framebuffer with a poorly aligned > > > stride. > > > > > > Hmm. Actually I suppose we should just handle it in > > > intel_fb_stride_alignment(). Eg.: > > > > > > case Yf: > > > if (cpp != 1 || pixel_format == DRM_FORMAT_NV12) > > > return 128; > > > else > > > return 64; > > > > This check is already there in intel_fb_stride_alignment() > > which is called from intel_framebuffer_init(). But it always > > does for 1st sub-plane which means does for Y. > > It needs an update to pass a sub-plane (for UV) parameter, > > and made another call for UV-plane alignment check. > > Will this be ok? > > Yeah. You could in fact just have it loop over all the planes > and call it for each. Something like this perhaps: > for (i = 0; i < drm_format_num_planes(formt); i++) { > intel_fb_stride_alignment(modifier[i], format, i); > ... > } I planned the same, looping for all subplanes. > > Or you could pass the cpp/bits_per_pixel instead of the plane index, > since that's the only thing for which you need the plane index within > the function. > > I also started to wonder whether we should repeat most of the other > checks in intel_framebuffer_init() for each plane. But it's probably > just easier to check that handle, pitch and modifier matches for > both planes in the NV12 case. I think you actually missed the > modifier check. You just checked that modifier[1] is Yf, but that > leaves modifier[0] unchecked. I failed to notice it as well during > my review of the relevant patch. Added modifier check. Made these changes to " drm/i915: Add NV12 support to intel_framebuffer_init" patch. Also sending out updated WA patch to move to init clockgating. Rest of your feedback is already addressed. Before sending out these 2 patches, any other comments? > > -- > Ville Syrjälä > Intel OTC
On Wed, Sep 09, 2015 at 09:09:27PM +0000, Konduru, Chandra wrote: > > > > > > > > > > > > + /* For tile-Yf, uv-subplane tile width is > > 2x of Y- > > > > > > subplane > > > > > > > > > > */ > > > > > > > > > > > + aux_stride = fb->modifier[0] == > > > > > > > > > > I915_FORMAT_MOD_Yf_TILED ? > > > > > > > > > > > + stride / 2 : stride; > > > > > > > > > > > > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal > > with > > > > > > > > > > cases when the Y stride is an odd number of tiles? > > > > > > > > > > > > > > > > > > It should be a round up division to take care of that scenario. > > > > > > > > > > > > > > > > That would stil lresult in a corrupted picture I think. So I was > > > > > > > > thinking that we should just refuse to create NCV12 framebuffers > > with a > > > > > > > > poorly aligned stride. > > > > > > > > > > > > > > > I added a check in intel_framebuffer_init() which should catch them: > > > > > > > if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > > > > > > DRM_DEBUG("y and uv subplanes have different pitches\n"); > > > > > > > return -EINVAL; > > > > > > > } > > > > > > > > > > > > That won't catch the case I'm worried about. We would also need to > > make > > > > > > sure pitches[1] is aligned to the UV tile width. > > > > > > > > > > If caller is following tile/row/pitch alignments properly for sub-planes of > > > > > NV12 Yf buffer, above check will catch. > > > > > But are you referring a case where userland isn't following tile/row size > > > > > alignments properly? In that case, above may not catch. But > > > > > isn't that is the case even with other FB formats if user land not > > > > > following tile/row/pitch alignments? > > > > > > > > We reject any attempt to create a framebuffer with a poorly aligned > > > > stride. > > > > > > > > Hmm. Actually I suppose we should just handle it in > > > > intel_fb_stride_alignment(). Eg.: > > > > > > > > case Yf: > > > > if (cpp != 1 || pixel_format == DRM_FORMAT_NV12) > > > > return 128; > > > > else > > > > return 64; > > > > > > This check is already there in intel_fb_stride_alignment() > > > which is called from intel_framebuffer_init(). But it always > > > does for 1st sub-plane which means does for Y. > > > It needs an update to pass a sub-plane (for UV) parameter, > > > and made another call for UV-plane alignment check. > > > Will this be ok? > > > > Yeah. You could in fact just have it loop over all the planes > > and call it for each. Something like this perhaps: > > for (i = 0; i < drm_format_num_planes(formt); i++) { > > intel_fb_stride_alignment(modifier[i], format, i); > > ... > > } > I planned the same, looping for all subplanes. > > > > > Or you could pass the cpp/bits_per_pixel instead of the plane index, > > since that's the only thing for which you need the plane index within > > the function. > > > > I also started to wonder whether we should repeat most of the other > > checks in intel_framebuffer_init() for each plane. But it's probably > > just easier to check that handle, pitch and modifier matches for > > both planes in the NV12 case. I think you actually missed the > > modifier check. You just checked that modifier[1] is Yf, but that > > leaves modifier[0] unchecked. I failed to notice it as well during > > my review of the relevant patch. > > Added modifier check. > Made these changes to " drm/i915: Add NV12 support to intel_framebuffer_init" > patch. Also sending out updated WA patch to move to init clockgating. > Rest of your feedback is already addressed. > Before sending out these 2 patches, any other comments? Looking at what was said, I think we covered most open items reasonably well, so fire away. I'll start going through the updated patches tomorrow.
> Looking at what was said, I think we covered most open items > reasonably well, so fire away. I'll start going through the > updated patches tomorrow. > Updates patches: http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4df4d77..329651e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3026,6 +3026,8 @@ u32 skl_plane_ctl_format(uint32_t pixel_format) return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY; case DRM_FORMAT_VYUY: return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY; + case DRM_FORMAT_NV12: + return PLANE_CTL_FORMAT_NV12; default: MISSING_CASE(pixel_format); } @@ -3094,6 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, int src_x = 0, src_y = 0, src_w = 0, src_h = 0; int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0; int scaler_id = -1; + u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0; + u32 tile_row_adjustment = 0; plane_state = to_intel_plane_state(plane->state); @@ -3150,11 +3154,34 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, x_offset = stride * tile_height - y - src_h; y_offset = x; plane_size = (src_w - 1) << 16 | (src_h - 1); + /* + * TBD: For NV12 90/270 rotation, Y and UV subplanes should + * be treated as separate surfaces and GTT remapping for + * rotation should be done separately for each subplane. + * Enable support once seperate remappings are available. + */ } else { stride = fb->pitches[0] / stride_div; x_offset = x; y_offset = y; plane_size = (src_h - 1) << 16 | (src_w - 1); + tile_height = PAGE_SIZE / stride_div; + + if (fb->pixel_format == DRM_FORMAT_NV12) { + int height_in_mem = (fb->offsets[1]/fb->pitches[0]); + /* + * If UV starts from middle of a page, then UV start should + * be programmed to beginning of that page. And offset into that + * page to be programmed into y-offset + */ + tile_row_adjustment = height_in_mem % tile_height; + aux_dist = fb->pitches[0] * (height_in_mem - tile_row_adjustment); + aux_x_offset = DIV_ROUND_UP(x, 2); + aux_y_offset = DIV_ROUND_UP(y, 2) + tile_row_adjustment; + /* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */ + aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ? + stride / 2 : stride; + } } plane_offset = y_offset << 16 | x_offset; @@ -3162,11 +3189,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); I915_WRITE(PLANE_STRIDE(pipe, 0), stride); + I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride); + I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset); if (scaler_id >= 0) { uint32_t ps_ctrl = 0; WARN_ON(!dst_w || !dst_h); + ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) | crtc_state->scaler_state.scalers[scaler_id].mode; I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl); @@ -3175,6 +3205,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h); I915_WRITE(PLANE_POS(pipe, 0), 0); } else { + WARN_ON(fb->pixel_format == DRM_FORMAT_NV12); I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x); } @@ -11626,6 +11657,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, bool turn_off, turn_on, visible, was_visible; struct drm_framebuffer *fb = plane_state->fb; + /* Adjust (macro)pixel boundary */ + if (fb && intel_format_is_yuv(fb->pixel_format)) { + to_intel_plane_state(plane_state)->src.x1 &= ~0x10000; + to_intel_plane_state(plane_state)->src.x2 &= ~0x10000; + } + if (crtc_state && INTEL_INFO(dev)->gen >= 9 && plane->type != DRM_PLANE_TYPE_CURSOR) { ret = skl_update_scaler_plane(
This patch is adding NV12 support to skylake primary plane programming. It is covering linear/X/Y/Yf tiling formats for 0 and 180 rotations. For 90/270 rotation, Y and UV subplanes should be treated as separate surfaces and GTT remapping for rotation should be done separately for each subplane. Once GEM adds support for seperate remappings for two subplanes, 90/270 support to be added to plane programming. v2: -Use regular int instead of 16.16 in aux_offset calculations (me) v3: -Allow 90/270 for NV12 as its remapping is now supported (me) v4: -Rebased to current kernel version 4.2.0.rc4 (me) Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> Testcase: igt/kms_nv12 --- drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)