[09/15] drm/i915: Add NV12 support to intel_framebuffer_init
diff mbox

Message ID 1441839543-20005-1-git-send-email-chandra.konduru@intel.com
State New
Headers show

Commit Message

Chandra Konduru Sept. 9, 2015, 10:59 p.m. UTC
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(-)

Comments

Ville Syrjälä Sept. 10, 2015, 6:34 p.m. UTC | #1
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
Chandra Konduru Sept. 10, 2015, 7:14 p.m. UTC | #2
> > +		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?

If condition isn't fitting in a single line, so continued in next line like any other if condition.
Not sure what the exact indentation issue you are observing.

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

Then will merge this assignment into beginning of the function
which does for modifier[0]. Will make a loop there. 
Is that ok?

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

May be not.
But setting NV12 UV offset from userland can be a miss. So added
this check to help userland knowing the issue.
Ville Syrjälä Sept. 10, 2015, 7:43 p.m. UTC | #3
On Thu, Sep 10, 2015 at 07:14:58PM +0000, Konduru, Chandra wrote:
> > > +		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?
> 
> If condition isn't fitting in a single line, so continued in next line like any other if condition.
> Not sure what the exact indentation issue you are observing.

Stuff should line up nicely after the opening (

if (foo &&
    bar &&
    zonk)

and not

if (foo &&
	bar &&
	zonk)


> 
> > 
> > > +			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.
> 
> Then will merge this assignment into beginning of the function
> which does for modifier[0]. Will make a loop there. 
> Is that ok?

Sure.

> 
> > 
> > > +		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?
> 
> May be not.
> But setting NV12 UV offset from userland can be a miss. So added
> this check to help userland knowing the issue.

It only catches the 0 case though. I think if userspace would forget to
set offsets[1] it would most likely also forget handles[1], so I think
we should be fairly well covered against the most obvious userspace
fumbles.
Ville Syrjälä Sept. 10, 2015, 7:46 p.m. UTC | #4
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
Chandra Konduru Sept. 10, 2015, 8:45 p.m. UTC | #5
> > > > +		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?
> >
> > If condition isn't fitting in a single line, so continued in next line like any other if
> condition.
> > Not sure what the exact indentation issue you are observing.
> 
> Stuff should line up nicely after the opening (
> 
> if (foo &&
>     bar &&
>     zonk)
> 
> and not
> 
> if (foo &&
> 	bar &&
> 	zonk)

I am continuing tab to indent the line below "if" to move past "if".
In the code, I see "space" or "tab" to move past "if" in line below.
One or the other, I used tab. But if that is causing issue, it is
Fine to use "spaces".

Anyway, this code is going away, as it is being merged into
beginning of the function.

> 
> 
> >
> > >
> > > > +			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.
> >
> > Then will merge this assignment into beginning of the function
> > which does for modifier[0]. Will make a loop there.
> > Is that ok?
> 
> Sure.
> 
> >
> > >
> > > > +		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?
> >
> > May be not.
> > But setting NV12 UV offset from userland can be a miss. So added
> > this check to help userland knowing the issue.
> 
> It only catches the 0 case though. I think if userspace would forget to
> set offsets[1] it would most likely also forget handles[1], so I think
> we should be fairly well covered against the most obvious userspace
> fumbles.
Yes, both handles and offsets are covered. So keeping this if block.

> 
> --
> Ville Syrjälä
> Intel OTC
Chandra Konduru Sept. 10, 2015, 8:59 p.m. UTC | #6
> > > +		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.

Yes, that is the bspec documentation for this check.
It is requiring the uv-offset to be 4-line aligned i.e., checking
uv-offset that is given to driver. If you take away 12-lsbs
then it is already at the page start. Why you want to take
away 12-LSBs?

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

UV-start is with respect to buffer and not relate to any incoming flip
clipping the uv where macro-pixel adjustments are done.
Sorry, I didn't get why you think X/Y offset maco-pixel adjustments 
have anything to do with uv-offset checking here in this if block?
 
> >
> > 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.
Daniel Vetter Sept. 14, 2015, 8:45 a.m. UTC | #7
On Thu, Sep 10, 2015 at 08:45:48PM +0000, Konduru, Chandra wrote:
> > > > > +		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?
> > >
> > > If condition isn't fitting in a single line, so continued in next line like any other if
> > condition.
> > > Not sure what the exact indentation issue you are observing.
> > 
> > Stuff should line up nicely after the opening (
> > 
> > if (foo &&
> >     bar &&
> >     zonk)
> > 
> > and not
> > 
> > if (foo &&
> > 	bar &&
> > 	zonk)
> 
> I am continuing tab to indent the line below "if" to move past "if".
> In the code, I see "space" or "tab" to move past "if" in line below.
> One or the other, I used tab. But if that is causing issue, it is
> Fine to use "spaces".
> 
> Anyway, this code is going away, as it is being merged into
> beginning of the function.

Yeah, continuation lines should be aligned with the opening ( whether
that's an if condition, function parameter list or something else.
checkpatch --strict will spot these for you.

I don't mind if it's just the occasional one, but if you get it
consistently wrong it does make the code a bit harder to follow (simply
because it's unexpected).
-Daniel
Chandra Konduru Sept. 16, 2015, 1:35 a.m. UTC | #8
> > > > > > +		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?
> > > >
> > > > If condition isn't fitting in a single line, so continued in next line like any
> other if
> > > condition.
> > > > Not sure what the exact indentation issue you are observing.
> > >
> > > Stuff should line up nicely after the opening (
> > >
> > > if (foo &&
> > >     bar &&
> > >     zonk)
> > >
> > > and not
> > >
> > > if (foo &&
> > > 	bar &&
> > > 	zonk)
> >
> > I am continuing tab to indent the line below "if" to move past "if".
> > In the code, I see "space" or "tab" to move past "if" in line below.
> > One or the other, I used tab. But if that is causing issue, it is
> > Fine to use "spaces".
> >
> > Anyway, this code is going away, as it is being merged into
> > beginning of the function.
> 
> Yeah, continuation lines should be aligned with the opening ( whether
> that's an if condition, function parameter list or something else.
> checkpatch --strict will spot these for you.
> 
> I don't mind if it's just the occasional one, but if you get it
> consistently wrong it does make the code a bit harder to follow (simply
> because it's unexpected).

Point noted.
Chandra Konduru Sept. 21, 2015, 4:14 p.m. UTC | #9
> > > > +		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.
> > >
> Ignore my previous response for this. Yes, above check should
> check for 12-lsbs. Will update and respun shortly.
Submitted updated patch. 
With this change, resolved all your feedback.
Is there any more feedback?
If not, can you issue R-B tag for the patches?

Patch
diff mbox

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)) {
+			mode_cmd->modifier[1] = I915_FORMAT_MOD_X_TILED;
+		}
+		if (!mode_cmd->offsets[1]) {
+			DRM_DEBUG("uv start offset not set\n");
+			return -EINVAL;
+		}
+		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;
+		}
+		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;