Re: [PATCH 09/15] drm/i915: Add NV12 support to intel_framebuffer_init

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 10, 2015 at 09:34:59PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 09, 2015 at 03:59:03PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 as supported format to
> > intel_framebuffer_init and performs various checks.
> > 
> > v2:
> > -Fix an issue in checks added (me)
> > 
> > v3:
> > -cosmetic update, split checks into two (Ville)
> > 
> > v4:
> > -Add stride alignment and modifier checks for UV subplane (Ville)
> > 
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Testcase: igt/kms_nv12
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   67 ++++++++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
> >  3 files changed, 57 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 84dad95..6124339 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2906,9 +2906,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  }
> >  
> >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > -			      uint32_t pixel_format)
> > +			      uint32_t pixel_format, int plane)
> >  {
> > -	u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> > +	u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8;
> >  
> >  	/*
> >  	 * The stride is either expressed as a multiple of 64 bytes
> > @@ -3117,7 +3117,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  
> >  	obj = intel_fb_obj(fb);
> >  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > -					       fb->pixel_format);
> > +					       fb->pixel_format, 0);
> >  	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
> >  
> >  	/*
> > @@ -9101,7 +9101,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> >  
> >  	val = I915_READ(PLANE_STRIDE(pipe, 0));
> >  	stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> > -						fb->pixel_format);
> > +						fb->pixel_format, 0);
> >  	fb->pitches[0] = (val & 0x3ff) * stride_mult;
> >  
> >  	aligned_height = intel_fb_align_height(dev, fb->height,
> > @@ -11172,7 +11172,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >  	 */
> >  	stride = fb->pitches[0] /
> >  		 intel_fb_stride_alignment(dev, fb->modifier[0],
> > -					   fb->pixel_format);
> > +					   fb->pixel_format, 0);
> >  
> >  	/*
> >  	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> > @@ -14238,6 +14238,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >  {
> >  	unsigned int aligned_height;
> >  	int ret;
> > +	int i;
> >  	u32 pitch_limit, stride_alignment;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > @@ -14277,12 +14278,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > -	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> > -						     mode_cmd->pixel_format);
> > -	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> > -		DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> > -			  mode_cmd->pitches[0], stride_alignment);
> > -		return -EINVAL;
> > +	/* check stride alignment for sub-planes */
> > +	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> > +		stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[i],
> > +						     mode_cmd->pixel_format, i);
> > +		if (mode_cmd->pitches[i] & (stride_alignment - 1)) {
> > +			DRM_DEBUG("subplane %d pitch (%d) must be at least %u bytes "
> > +				"aligned\n", i, mode_cmd->pitches[i], stride_alignment);
> > +			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0],
> > @@ -14349,9 +14353,48 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >  			return -EINVAL;
> >  		}
> >  		break;
> > +	case DRM_FORMAT_NV12:
> > +		if (INTEL_INFO(dev)->gen < 9) {
> > +			DRM_DEBUG("unsupported pixel format: %s\n",
> > +				drm_get_format_name(mode_cmd->pixel_format));
> > +			return -EINVAL;
> > +		}
> > +		if (obj->tiling_mode == I915_TILING_X &&
> > +			!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
> 
> Your editor still seems to mess up the indentation in these cases. Can
> you try to fix that?
> 
> > +			mode_cmd->modifier[1] = I915_FORMAT_MOD_X_TILED;
> > +		}
> 
> Hmm. This obj tiling -> modifier conversion should be a fairly generic
> thing to do, so I suggest doing it for all planes in one place. Maybe
> add a new function intel_fb_obj_tiling_to_modifier() or something like
> that and loop over all the planes there.
> 
> > +		if (!mode_cmd->offsets[1]) {
> > +			DRM_DEBUG("uv start offset not set\n");
> > +			return -EINVAL;
> > +		}
> 
> Still not really happy with this check since it's either too limiting, or
> not restrictive enough. Depending on how you look at it. Do you know
> if the hardware gets upset if you tell it use overlapping Y and UV
> surfaces?
> 
> > +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > +			DRM_DEBUG("y and uv subplanes have different pitches\n");
> > +			return -EINVAL;
> > +		}
> > +		if (mode_cmd->handles[0] != mode_cmd->handles[1]) {
> > +			DRM_DEBUG("y and uv subplanes have different handles\n");
> > +			return -EINVAL;
> > +		}
> > +		if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) {
> > +			DRM_DEBUG("y and uv subplanes have different modifiers\n");
> > +			return -EINVAL;
> > +		}
> > +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED &&
> > +			(mode_cmd->offsets[1] & 0xFFF)) {
> > +			DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n",
> > +				mode_cmd->offsets[1]);
> > +			return -EINVAL;
> > +		}
> > +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED &&
> > +			((mode_cmd->offsets[1] / mode_cmd->pitches[1]) % 4)) {
> > +			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
> > +				mode_cmd->offsets[1]);
> > +			return -EINVAL;
> > +		}
> 
> I was going to say I can't find anything in the spec to support this,
> but after some more reading I got it "The display hardware requires
> that the UV surface start satisfies four line alignment from the
> begining of the page." So the check should be something like
> ((offsets[1] & 0xfff) / pitches[1] % 4.
> 
> However we should anyway be able to adjust the X/Y offsets to account
> for misalignment of offsets[1]. I think the only thing we should need
> to check is that offsets[1] is macropixel aligned. The patch lacks such
> a check in fact.
> 
> Feels like it would be time to expand intel_gen4_compute_page_offset()
> to handle the new tiling formats and start using it for SKL+. Though
> rotation may need some additional thought. Also maybe it's time to
> dig up my old offsets[0] handling patch and refresh it a bit and try
> to get it merged again.

Just to clarify a bit. I think we can initially go with the checks in
place, and we can work on refactoring the page offset stuff afterwards
Actually I already started to sketch something together here :) so I
might have a few patches to post sooner rather than later.

> 
> > +		break;
> >  	default:
> >  		DRM_DEBUG("unsupported pixel format: %s\n",
> > -			  drm_get_format_name(mode_cmd->pixel_format));
> > +			drm_get_format_name(mode_cmd->pixel_format));
> >  		return -EINVAL;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index d50b8cb..62d2a11 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev,
> >  void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> >  			enum fb_op_origin origin);
> >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > -			      uint32_t pixel_format);
> > +			      uint32_t pixel_format, int plane);
> >  
> >  /* intel_audio.c */
> >  void intel_init_audio(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 797594e..49feae0 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  				       src_w != crtc_w || src_h != crtc_h);
> >  
> >  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > -					       fb->pixel_format);
> > +					       fb->pixel_format, 0);
> >  
> >  	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> >  
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx