[Intel-gfx] [PATCH v3 1/2] drm/i915/skl+: Check for supported plane configuration in Interlace mode

[Ville Syrjälä]

On Wed, Jul 05, 2017 at 12:09:19PM +0530, Mahesh Kumar wrote:
> Hi,
> 
> 
> On Tuesday 04 July 2017 08:11 PM, Ville Syrjälä wrote:
> > On Mon, Jul 03, 2017 at 09:28:00PM +0530, Mahesh Kumar wrote:
> >> Hi,
> >>
> >>
> >> On Monday 03 July 2017 07:32 PM, Ville Syrjälä wrote:
> >>> On Sat, Jul 01, 2017 at 09:35:12AM +0530, Mahesh Kumar wrote:
> >>>> Hi,
> >>>>
> >>>>
> >>>> On Friday 30 June 2017 05:56 PM, Ville Syrjälä wrote:
> >>>>> On Fri, Jun 30, 2017 at 05:40:59PM +0530, Mahesh Kumar wrote:
> >>>>>> In Gen9 platform Interlaced fetch mode doesn't support following plane
> >>>>>> configuration:
> >>>>>>     - Y/Yf tiling
> >>>>>>     - 90/270 rotation
> >>>>> The rotation check seems to be missing from the code?
> >>>> 90/270 rotation require Y/Yf tiling, so that will be automagically
> >>>> handled as Y-tile case.
> >>> Right. OK, series lgtm
> >>> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>
> >>> Was there a bugzilla link we should include in these patches?
> >> following is bugzilla link for the issue.
> >> https://bugs.freedesktop.org/show_bug.cgi?id=90238
> > Added that, and pushed the series to dinq. Thanks for the patches.
> >
> > I had to fix a small checkpatch complaint while applying. Plase always
> > review the checkpatch warnings when submitting patches.
> ok sure, will start reviewing checkpatch warnings.
> thanks for review,
> Do we need to push these patches to "stable" as well?

I was initially thinking of adding cc:stable, but forgot. In the end
I think this combination might be rare enough that we might not have
to bother unless we get an actual bug report.

> for enabling PF-ID Interlace mode what should be the approach?
> Should we always enable PF-ID in GEN9+, or should we create property to 
> select the fetching mode?

I'm a bit leery on always enabling it becasue that means you can no
longer present interlaced material properly. It sounds a bit unlikely
that anyone would explicitly choose to use an interlaced mode for
progressive material when there's progressive mode available. Not
sure if such crappy TVs exist anymore that can't do eg. 1080p but
can do 1080i. I would hope not.

> for property based implementation who will be the user-space consumer 
> for property?

If it were a connector property then I think X should pick it up
automatically. But would anyone actually use it? I don't know.

> Please let me know your thoughts on that.
> thanks,
> 
> -Mahesh
> >
> >> -Mahesh
> >>>> -Mahesh
> >>>>>>     - YUV420 hybrid planar source pixel formats.
> >>>>>>
> >>>>>> This patch adds check to fail the flip if any of the above configuration
> >>>>>> is requested.
> >>>>>>
> >>>>>> Changes since V1:
> >>>>>>     - handle checks in intel_plane_atomic_check_with_state (ville)
> >>>>>>     - takeout plane scaler checks combile with pipe scaler in next patch
> >>>>>> Changes since V2:
> >>>>>>     - No need to check for NV12 as it need scaling, so it will be rejected
> >>>>>>       by scaling check (ville)
> >>>>>>
> >>>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
> >>>>>>     1 file changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>>> index 4325cb0a04f5..ee76fab7bb6f 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>>> @@ -114,6 +114,8 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> >>>>>>     	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> >>>>>>     	struct drm_plane_state *state = &intel_state->base;
> >>>>>>     	struct intel_plane *intel_plane = to_intel_plane(plane);
> >>>>>> +	const struct drm_display_mode *adjusted_mode =
> >>>>>> +		&crtc_state->base.adjusted_mode;
> >>>>>>     	int ret;
> >>>>>>     
> >>>>>>     	/*
> >>>>>> @@ -173,6 +175,19 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> >>>>>>     	if (ret)
> >>>>>>     		return ret;
> >>>>>>     
> >>>>>> +	/*
> >>>>>> +	 * Y-tiling is not supported in IF-ID Interlace mode in
> >>>>>> +	 * GEN9 and above.
> >>>>>> +	 */
> >>>>>> +	if (state->fb && INTEL_GEN(dev_priv) >= 9 && crtc_state->base.enable &&
> >>>>>> +	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> >>>>>> +		if (state->fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> >>>>>> +		    state->fb->modifier == I915_FORMAT_MOD_Yf_TILED) {
> >>>>>> +			DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
> >>>>>> +			return -EINVAL;
> >>>>>> +		}
> >>>>>> +	}
> >>>>>> +
> >>>>>>     	/* FIXME pre-g4x don't work like this */
> >>>>>>     	if (intel_state->base.visible)
> >>>>>>     		crtc_state->active_planes |= BIT(intel_plane->id);
> >>>>>> -- 
> >>>>>> 2.13.0

-- 
Ville Syrjälä
Intel OTC