<div dir="ltr"><div dir="ltr">On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:<br>
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen<br>
> <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote:<br>
> ><br>
> > With modifiers one can actually have different format_info structs<br>
> > for the same format, which now matters for AMDGPU since we convert<br>
> > implicit modifiers to explicit modifiers with multiple planes.<br>
> ><br>
> > I checked other drivers and it doesn't look like they end up triggering<br>
> > this case so I think this is safe to relax.<br>
> ><br>
> > Signed-off-by: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>><br>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")<br>
> > ---<br>
> >  drivers/gpu/drm/drm_plane.c | 2 +-<br>
> >  1 file changed, 1 insertion(+), 1 deletion(-)<br>
> ><br>
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c<br>
> > index e6231947f987..f5085990cfac 100644<br>
> > --- a/drivers/gpu/drm/drm_plane.c<br>
> > +++ b/drivers/gpu/drm/drm_plane.c<br>
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,<br>
> >         if (ret)<br>
> >                 goto out;<br>
> ><br>
> > -       if (old_fb->format != fb->format) {<br>
> > +       if (old_fb->format->format != fb->format->format) {<br>
> <br>
> This was btw. the original way before Ville made it more strict about<br>
> 4 years ago, to catch issues related to tiling, and more complex<br>
> layouts, like the dcc tiling/retiling introduced by your modifier<br>
> patches. That's why I hope my alternative patch is a good solution for<br>
> atomic drivers while keeping the strictness for potential legacy<br>
> drivers.<br>
<br>
Yeah this doesn't work in full generality, because hw might need to do a<br>
full modeset to do a full modeset to reallocate resources (like scanout<br>
fifo space) if the format changes.<br>
<br>
But for atomic drivers that should be caught in ->atomic_check, which<br>
should result in -EINVAL, so should do the right thing. So it should be<br>
all good, but imo needs a comment to explain what's going on:<br>
<br>
        /*<br>
         * Only check the FOURCC format code, excluding modifiers. This is<br>
         * enough for all legacy drivers. Atomic drivers have their own<br>
         * checks in their ->atomic_check implementation, which will<br>
         * return -EINVAL if any hw or driver constraint is violated due<br>
         * to modifier changes.<br>
         */<br>
<br>
Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?<br>
<br>
With that:<br>
<br>
Reviewed-by: Daniel Vetter <<a href="mailto:daniel.vetter@ffwll.ch" target="_blank">daniel.vetter@ffwll.ch</a>><br>
<br></blockquote><div><br></div><div>Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?</div><div><br></div><div>thanks,<br></div><div>-mario</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> -mario<br>
> <br>
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");<br>
> >                 ret = -EINVAL;<br>
> >                 goto out;<br>
> > --<br>
> > 2.29.2<br>
> ><br>
<br>
-- <br>
Daniel Vetter<br>
Software Engineer, Intel Corporation<br>
<a href="http://blog.ffwll.ch" rel="noreferrer" target="_blank">http://blog.ffwll.ch</a><br>
</blockquote></div></div>