<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <p style="font-family:Arial;font-size:11pt;color:#0078D7;margin:5pt;" align="Left"> [AMD Official Use Only - Internal Distribution Only]<br> </p> <br> <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> Hi Daniel,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);"> Could you please be more specific about the _unsafe API options you mentioned ?<br> </div> <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div id="Signature"> <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> --</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks & Regards,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Aurabindo Pillai<br> </div> </div> </div> </div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Daniel Vetter <daniel@ffwll.ch><br> <b>Sent:</b> Tuesday, January 19, 2021 8:11 AM<br> <b>To:</b> Pekka Paalanen <ppaalanen@gmail.com><br> <b>Cc:</b> 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><br> <b>Subject:</b> Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode</font> <div> </div> </div> <div class="BodyFragment"><font size="2"><span style="font-size:11pt;"> <div class="PlainText">On Tue, Jan 19, 2021 at 9:35 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:<br> ><br> > On Mon, 18 Jan 2021 09:36:47 -0500<br> > Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:<br> ><br> > > On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:<br> > > ><br> > > > Hi,<br> > > ><br> > > > please document somewhere that ends up in git history (commit<br> > > > message,<br> > > > code comments, description of the parameter would be the best but<br> > > > maybe<br> > > > there isn't enough space?) what Christian König explained in<br> > > ><br> > > ><br> > > > <a href="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"> 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</a><br> > > ><br> > > > that this is a stop-gap feature intended to be removed as soon as<br> > > > possible (when a better solution comes up, which could be years).<br> > > ><br> > > > So far I have not seen a single mention of this intention in your<br> > > > patch<br> > > > submissions, and I think it is very important to make known.<br> > ><br> > > Hi,<br> > ><br> > > Thanks for the headsup, I shall add the relevant info in the next<br> > > verison.<br> > ><br> > > ><br> > > > I also did not see an explanation of why this instead of<br> > > > manufacturing<br> > > > these video modes in userspace (an idea mentioned by Christian in the<br> > > > referenced email). I think that too should be part of a commit<br> > > > message.<br> > ><br> > > This is an opt-in feature, which shall be superseded by a better<br> > > solution. We also add a set of common modes for scaling similarly.<br> > > Userspace can still add whatever mode they want. So I dont see a reason<br> > > why this cant be in the kernel.<br> ><br> > Hi,<br> ><br> > sorry, I think that kind of thinking is backwards. There needs to be a<br> > reason to put something in the kernel, and if there is no reason, then<br> > it remains in userspace. So what's the reason to put this in the kernel?<br> ><br> > One example reason why this should not be in the kernel is that the set<br> > of video modes to manufacture is a kind of policy, which modes to add<br> > and which not. Userspace knows what modes it needs, and establishing<br> > the modes in the kernel instead is second-guessing what the userspace<br> > would want. So if userspace needs to manufacture modes in userspace<br> > anyway as some modes might be missed by the kernel, then why bother in<br> > the kernel to begin with? Why should the kernel play catch-up with what<br> > modes userspace wants when we already have everything userspace needs<br> > to make its own modes, even to add them to the kernel mode list?<br> ><br> > Does manufacturing these extra video modes to achieve fast timing<br> > changes require AMD hardware-specific knowledge, as opposed to the<br> > general VRR approach of simply adjusting the front porch?<br> ><br> > Something like this should also be documented in a commit message. Or<br> > if you insist that "no reason to not put this in the kernel" is reason<br> > enough, then write that down, because it does not seem obvious to me or<br> > others that this feature needs to be in the kernel.<br> <br> One reason might be debugging, if a feature is known to cause issues.<br> But imo in that case the knob should be using the _unsafe variants so<br> it taints the kernel, since otherwise we get stuck in this very cozy<br> place where kernel maintainers don't have to care much for bugs<br> "because it's off by default", but also not really care about<br> polishing the feature "since users can just enable it if they want<br> it". Just a slightly different flavour of what you're explaining above<br> already.<br> -Daniel<br> <br> > Thanks,<br> > pq<br> <br> <br> <br> -- <br> Daniel Vetter<br> Software Engineer, Intel Corporation<br> <a href="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">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</a><br> </div> </span></font></div> </div> </body> </html>