<div dir="ltr"><div dir="ltr">On Thu, Jan 7, 2021 at 7:04 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 Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>> wrote:<br>
><br>
> On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote:<br>
>><br>
>> 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>
><br>
> Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?<br>
<br>
Full disclosure, Ville wanted to do something similar since forever<br>
I'm not a huge fan of removing limitations of legacy ioctls. Worst<br>
case we break something, best case no gain in features since why don't<br>
you just use atomic. Since this (amdgpu modifiers) broke something we<br>
have to fix it, hence I'd go with the more minimal version from Bas<br>
here.<br>
<br></blockquote><div><br></div><div>Fair point. Means though that somebody will have to convert many user-space clients, e.g., all OSS Vulkan drivers. And XOrg could not do that, as the kernel uabi even blocks use of atomic drmSetClientCap(...ATOMIC...) for any process whose taskname starts with 'X', as a workaround for a modesetting-ddx with broken atomic implementation. So at least for (pun ahead) "X" applications, atomic modesetting is not an option.<br></div><div><br></div><div>For my use cases, X11/XOrg native is still the only display server capable enough to fulfill the needs, although I'm mixing in a bit of Vulkan/WSI/DirectDisplay for direct DRM/KMS access to work around some limitations, e.g., to get HDR or fp16 support.<br></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">
But in general your patch should be correct too.<br>
-Daniel<br>
<br></blockquote><div><div><br></div><div>Thanks for the feedback. I rest my case.</div><div>-mario</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> thanks,<br>
> -mario<br>
><br>
>> ><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>
<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>