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

[Paulo Zanoni]

Em Qui, 2018-09-27 às 17:16 +0300, Ville Syrjälä escreveu:
> On Tue, Sep 25, 2018 at 03:02:21PM -0700, Paulo Zanoni wrote:
> > 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.
> 
> Sure. Was just trying to trick someone into finishing what I started
> ;)
> 
> Patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Thanks a lot! Feel free to CC me on the series that kills the whole
thing, I agree it's a good thing to do.

> 
> > 
> > 
> > > 
> > > > 
> > > > 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 at linux.intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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
> > > 
> > > 
> 
>