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@xxxxxxxxxxxxxxx> 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@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 > > > > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
