[Intel-xe] [PATCH 08/10] fixup! drm/xe/display: Implement display support

[Lucas De Marchi]

On Wed, Oct 04, 2023 at 08:11:18PM +0300, Jani Nikula wrote:
>On Wed, 04 Oct 2023, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> On Tue, Oct 03, 2023 at 05:34:55PM +0300, Jani Nikula wrote:
>>>Turn the enable_display module parameter to the same thing as it is for
>>>i915: assuming you have display hardware, take over it, put it to sleep,
>>>and keep connectors disconnected.
>>
>> that was not what it was created for and it was on purpose. That is not
>> what we need when enabling a platform. The argument that *users* would
>> then be left in a situation that the display hardware is still consuming
>> power is IMO wrong as those *users* are not supposed to be using module
>> parameters and if they do, it's their only responsibility.
>>
>> The module parameter here is for kernel developers and CI/validation,
>> particularly when enabling a new platform, to decide if the display side
>> should be enabled or not.
>
>If you have a module param without the _unsafe annotation, and the only
>documentation is "enable display", it *is* for users.

There's only a handfull of users of the unsafe variant and I'm sure
there are many more module params not intended to be used in normal
scenarios.

>
>If you want to have a module param that's for development or debugging
>only, make it _unsafe, and add documentation to that effect.
>
>In any case, what's currently in the tree is conflated and broken, and
>this is/was an attempt to fix it. It's impossible to integrate xe with
>i915 display if the xe has different notions on the basic idea of what
>having or enabling display means.

i915 side is called *disable*_display since that's what it does. Here
it's called *enable* and we set it to 0 so it doesn't try to enable it.
I'm totally ok with renaming the module param on the xe side to something
more informative and making it _unsafe. Suggestions about a different
name?

Lucas De Marchi

>
>BR,
>Jani.
>
>-- 
>Jani Nikula, Intel