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@xxxxxxxxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxxxxxxxx> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > > --- > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
