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

[Ville Syrjälä]

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>

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

-- 
Ville Syrjälä
Intel