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