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

From: "Srinivas, Vidya" <vidya.srinivas@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 8/8] drm/i915: Add NV12 support to intel_framebuffer_init
Date: Thu, 15 Jun 2017 11:30:37 +0000	[thread overview]
Message-ID: <F653A0A18852B74D88578FA2EB7094EAB2BF06E1@BGSMSX108.gar.corp.intel.com> (raw)
In-Reply-To: <20170612142531.GG12629@intel.com>

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Monday, June 12, 2017 7:56 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Wed, Jun 07, 2017 at 04:11:48PM +0530, Vidya Srinivas wrote:
> > From: Chandra Konduru <chandra.konduru@intel.com>
> >
> > This patch adds NV12 as supported format to intel_framebuffer_init and
> > performs various checks.
> >
> > v2:
> > -Fix an issue in checks added (Chandra Konduru)
> >
> > v3: rebased (me)
> >
> > Link: https://patchwork.kernel.org/patch/6426221/
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 300f589..6e89421 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14767,6 +14767,21 @@ static int intel_framebuffer_init(struct
> intel_framebuffer *intel_fb,
> >  			goto err;
> >  		}
> >  		break;
> > +	case DRM_FORMAT_NV12:
> 
> Needs platform checks here.
> 
> > +		if (!mode_cmd->offsets[1])
> > +			DRM_ERROR("uv start offset not set\n");
> 
> Remove all the offset checks here. They should have handled by the generic
> code. The check you have here are inconsistent anyway as one of them
> assumes offsets[] is a lienar offset and the other assumes it's not.

Thank you. I will make the change and re-submit.

Regards
Vidya
> 
> > +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
> 
> I thought we could have different strides for the planes?
Yes true, I will make the necessary changes and re-submit.
> 
> > +			mode_cmd->handles[0] != mode_cmd->handles[1])
> 
> Should already be handled by the generic code.
> 
> > +			DRM_ERROR("y & uv subplanes have different
> params\n");
> > +		if (mode_cmd->modifier[1] ==
> I915_FORMAT_MOD_Yf_TILED &&
> > +			(mode_cmd->offsets[1] & 0xFFF))
> > +			DRM_ERROR("tile-Yf uv offset 0x%x isn't starting on
> new tile-row\n",
> > +				mode_cmd->offsets[1]);
> > +		if (mode_cmd->modifier[1] ==
> I915_FORMAT_MOD_Y_TILED &&
> > +			((mode_cmd->offsets[1] / mode_cmd->pitches[1])
> % 4))
> > +			DRM_ERROR("tile-Y uv offset 0x%x isn't 4-line
> aligned\n",
> > +				mode_cmd->offsets[1]);
> > +		break;
> >  	default:
> >  		DRM_DEBUG_KMS("unsupported pixel format: %s\n",
> >  			      drm_get_format_name(mode_cmd-
> >pixel_format, &format_name));
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx