[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.free...
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....