Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

["Pillai, Aurabindo" <Aurabindo.Pillai@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5166 bytes --]

[AMD Official Use Only - Internal Distribution Only]

Hi Daniel,

Could you please be more specific about the _unsafe API options you mentioned ?

--

Thanks & Regards,
Aurabindo Pillai
________________________________
From: Daniel Vetter <daniel@ffwll.ch>
Sent: Tuesday, January 19, 2021 8:11 AM
To: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Thai, Thong <Thong.Thai@amd.com>; Sharma, Shashank <Shashank.Sharma@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

On Tue, Jan 19, 2021 at 9:35 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 18 Jan 2021 09:36:47 -0500
> Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>
> > On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
> > >
> > > Hi,
> > >
> > > please document somewhere that ends up in git history (commit
> > > message,
> > > code comments, description of the parameter would be the best but
> > > maybe
> > > there isn't enough space?) what Christian König explained in
> > >
> > >
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-December%2F291254.html&amp;data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=GM0ZEM9JeFM5os13E1zlVy8Bn3D8Kxmo%2FajSG02WsGI%3D&amp;reserved=0
> > >
> > > that this is a stop-gap feature intended to be removed as soon as
> > > possible (when a better solution comes up, which could be years).
> > >
> > > So far I have not seen a single mention of this intention in your
> > > patch
> > > submissions, and I think it is very important to make known.
> >
> > Hi,
> >
> > Thanks for the headsup, I shall add the relevant info in the next
> > verison.
> >
> > >
> > > I also did not see an explanation of why this instead of
> > > manufacturing
> > > these video modes in userspace (an idea mentioned by Christian in the
> > > referenced email). I think that too should be part of a commit
> > > message.
> >
> > This is an opt-in feature, which shall be superseded by a better
> > solution. We also add a set of common modes for scaling similarly.
> > Userspace can still add whatever mode they want. So I dont see a reason
> > why this cant be in the kernel.
>
> Hi,
>
> sorry, I think that kind of thinking is backwards. There needs to be a
> reason to put something in the kernel, and if there is no reason, then
> it remains in userspace. So what's the reason to put this in the kernel?
>
> One example reason why this should not be in the kernel is that the set
> of video modes to manufacture is a kind of policy, which modes to add
> and which not. Userspace knows what modes it needs, and establishing
> the modes in the kernel instead is second-guessing what the userspace
> would want. So if userspace needs to manufacture modes in userspace
> anyway as some modes might be missed by the kernel, then why bother in
> the kernel to begin with? Why should the kernel play catch-up with what
> modes userspace wants when we already have everything userspace needs
> to make its own modes, even to add them to the kernel mode list?
>
> Does manufacturing these extra video modes to achieve fast timing
> changes require AMD hardware-specific knowledge, as opposed to the
> general VRR approach of simply adjusting the front porch?
>
> Something like this should also be documented in a commit message. Or
> if you insist that "no reason to not put this in the kernel" is reason
> enough, then write that down, because it does not seem obvious to me or
> others that this feature needs to be in the kernel.

One reason might be debugging, if a feature is known to cause issues.
But imo in that case the knob should be using the _unsafe variants so
it taints the kernel, since otherwise we get stuck in this very cozy
place where kernel maintainers don't have to care much for bugs
"because it's off by default", but also not really care about
polishing the feature "since users can just enable it if they want
it". Just a slightly different flavour of what you're explaining above
already.
-Daniel

> Thanks,
> pq



--
Daniel Vetter
Software Engineer, Intel Corporation
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2isCpwa3V92TnO4njhe9cQjdWVdsV1GQMo7WP7buVZI%3D&amp;reserved=0

[-- Attachment #1.2: Type: text/html, Size: 8747 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel