On Tue, Jan 19, 2021 at 5:08 PM Pillai, Aurabindo <Aurabindo.Pillai@xxxxxxx> 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@xxxxxxxx> > Sent: Tuesday, January 19, 2021 8:11 AM > To: Pekka Paalanen <ppaalanen@xxxxxxxxx> > Cc: Pillai, Aurabindo <Aurabindo.Pillai@xxxxxxx>; amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; dri-devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; Thai, Thong <Thong.Thai@xxxxxxx>; Sharma, Shashank <Shashank.Sharma@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx> > 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@xxxxxxxxx> wrote: > > > > On Mon, 18 Jan 2021 09:36:47 -0500 > > Aurabindo Pillai <aurabindo.pillai@xxxxxxx> 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 _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
