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

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

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.

> +		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