Re: [PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu:
> On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote:
> > Function intel_framebuffer_init() checks for the possibilities
> > during
> > framebuffer creation (addfb ioctl time). It is missing the fact
> > that
> > the indexed format is not supported with Yf tiling.
> > 
> > It is worth noticing that skl_plane_format_mod_supported()
> > correctly
> > handles for the C8/Yf combination, but this function runs during
> > modeset time, so we only reject the combination later.
> > 
> > Ville recently proposed a new IGT test that only uses addfb to
> > assert
> > supported formats, so that IGT was failing. Add the check so we get
> > green squares right from the start after Ville merges his test.
> 
> I have two of three (possibly) nicer ways to solve this:
> https://patchwork.freedesktop.org/series/39700/

I thought about implementing this one when looking at the code. I agree
the duplicated checks are horrible. I thought maybe this model wouldn't
be acceptable due to the inefficiency of always looping over everything
vs the current linear solution.

I see no review comments on this series besides the vc4 patch. Did you
get anything that's not appearing on patchwork?

> https://patchwork.freedesktop.org/series/39383/

You have blocked your own patch with your own review here.

> https://patchwork.freedesktop.org/series/39813/

Looks like there's some potential controversy to be untangled here if
we wish to follow this route.

> solution 4

I guess it would be to simply not have the checks at all. But this
would be an interface change instead of just refactoring code
duplication.


> 
> Would be nice if someone could figure out a solution (one of those or
> perhaps some other solution I didn't think of) that enough people are
> willing to accept.

I could see solution 1 moving forward more easily, and even could
volunteer myself to review a rebased version.

In the meantime, we could actually review/commit this immediate fix and
have a correct-but-not-yet-reworked codebase instead of waiting for a
patch that has been abandoned since March. I don't think one series
should block the other.


> 
> > 
> > Also drive-by fix the missing /* fall through */ in the chunk we
> > modified by just turning it into a "break;" since IMHO breaks are
> > easier to read than fall-throughs.
> > 
> > BSpec: 18565
> > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index eb25037d7b38..fdff1779f778 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >  			goto err;
> >  		}
> >  		/* fall through */
> > -	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Yf_TILED:
> > +		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
> > +			DRM_DEBUG_KMS("Indexed format does not
> > support Yf tiling\n");
> > +			goto err;
> > +		}
> > +		/* fall through */
> > +	case I915_FORMAT_MOD_Y_TILED:
> >  		if (INTEL_GEN(dev_priv) < 9) {
> >  			DRM_DEBUG_KMS("Unsupported tiling
> > 0x%llx!\n",
> >  				      mode_cmd->modifier[0]);
> >  			goto err;
> >  		}
> > +		break;
> >  	case DRM_FORMAT_MOD_LINEAR:
> >  	case I915_FORMAT_MOD_X_TILED:
> >  		break;
> > -- 
> > 2.14.4
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx