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

Tue Jan 19 18:58:48 UTC 2021

On Tue, Jan 19, 2021 at 5:08 PM Pillai, Aurabindo
<Aurabindo.Pillai at amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> Hi Daniel,
>
> Could you please be more specific about the _unsafe API options you mentioned ?

module_param_named_unsafe()

Cheers, Daniel

>
> --
>
> Thanks & Regards,
> Aurabindo Pillai
> ________________________________
> From: Daniel Vetter <daniel at ffwll.ch>
> Sent: Tuesday, January 19, 2021 8:11 AM
> To: Pekka Paalanen <ppaalanen at gmail.com>
> Cc: Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; amd-gfx list <amd-gfx at lists.freedesktop.org>; dri-devel <dri-devel at lists.freedesktop.org>; Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang at amd.com>; Thai, Thong <Thong.Thai at amd.com>; Sharma, Shashank <Shashank.Sharma at amd.com>; Lin, Wayne <Wayne.Lin at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian <Christian.Koenig at 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 at gmail.com> wrote:
> >
> > On Mon, 18 Jan 2021 09:36:47 -0500
> > Aurabindo Pillai <aurabindo.pillai at 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&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GM0ZEM9JeFM5os13E1zlVy8Bn3D8Kxmo%2FajSG02WsGI%3D&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&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2isCpwa3V92TnO4njhe9cQjdWVdsV1GQMo7WP7buVZI%3D&reserved=0


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch