| Message ID | 20210104210800.789944-2-aurabindo.pillai@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Experimental freesync video mode optimization | expand |
On Mon, 4 Jan 2021 16:07:58 -0500 Aurabindo Pillai <aurabindo.pillai@amd.com> wrote: > [Why&How] > Adds a module parameter to enable experimental freesync video mode modeset > optimization. Enabling this mode allows the driver to skip a full modeset when > freesync compatible modes are requested by the userspace. This paramters also > adds some standard modes based on the connected monitor's VRR range. > > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com> > Acked-by: Christian König <christian.koenig at amd.com> > Reviewed-by: Shashank Sharma <shashank.sharma@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 100a431f0792..12b13a90eddf 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery; > extern int amdgpu_emu_mode; > extern uint amdgpu_smu_memory_pool_size; > extern uint amdgpu_dc_feature_mask; > +extern uint amdgpu_exp_freesync_vid_mode; > extern uint amdgpu_dc_debug_mask; > extern uint amdgpu_dm_abm_level; > extern struct amdgpu_mgpu_info mgpu_info; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index b48d7a3c2a11..2badbc8b2294 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -158,6 +158,7 @@ int amdgpu_mes; > int amdgpu_noretry = -1; > int amdgpu_force_asic_type = -1; > int amdgpu_tmz; > +uint amdgpu_exp_freesync_vid_mode; > int amdgpu_reset_method = -1; /* auto */ > int amdgpu_num_kcq = -1; > > @@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); > MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)"); > module_param_named(tmz, amdgpu_tmz, int, 0444); > > +/** > + * DOC: experimental_freesync_video (uint) > + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience > + * when setting a freesync supported mode for which full modeset is not needed. > + * The default value: 0 (off). > + */ > +MODULE_PARM_DESC( > + freesync_video, > + "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)"); > +module_param_named(freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444); > + > /** > * DOC: reset_method (int) > * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco) 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://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html 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. 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. Thanks, pq
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://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html > > 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. -- Regards, Aurabindo Pillai > > > Thanks, > pq
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://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html > > > > 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. Thanks, pq
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://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html > > > > > > 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
[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
On Tue, Jan 19, 2021 at 5:08 PM Pillai, Aurabindo <Aurabindo.Pillai@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@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&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
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 100a431f0792..12b13a90eddf 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery; extern int amdgpu_emu_mode; extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; +extern uint amdgpu_exp_freesync_vid_mode; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; extern struct amdgpu_mgpu_info mgpu_info; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index b48d7a3c2a11..2badbc8b2294 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -158,6 +158,7 @@ int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; int amdgpu_tmz; +uint amdgpu_exp_freesync_vid_mode; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1; @@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)"); module_param_named(tmz, amdgpu_tmz, int, 0444); +/** + * DOC: experimental_freesync_video (uint) + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience + * when setting a freesync supported mode for which full modeset is not needed. + * The default value: 0 (off). + */ +MODULE_PARM_DESC( + freesync_video, + "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)"); +module_param_named(freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444); + /** * DOC: reset_method (int) * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)