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

[Pekka Paalanen]

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://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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210119/7f027c42/attachment-0001.sig>