Re: RFC: Drm-connector properties managed by another driver / privacy screen support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/15/20 11:10 PM, Jani Nikula wrote:
> On Wed, 15 Apr 2020, Rajat Jain <rajatja@google.com> wrote:
>> Hello,
>>
>> On Wed, Apr 15, 2020 at 8:40 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 4/15/20 5:28 PM, Jani Nikula wrote:
>>>> On Wed, 15 Apr 2020, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> ii. Currently the "privacy-screen" property added by Rajat's
>>>>> patch-set is an enum with 2 possible values:
>>>>> "Enabled"
>>>>> "Disabled"
>>>>>
>>>>> We could add a third value "Not Available", which would be the
>>>>> default and then for internal panels always add the property
>>>>> so that we avoid the problem that detecting if the laptop has
>>>>> an internal privacy screen needs to be done before the connector
>>>>> is registered. Then we can add some hooks which allow an
>>>>> lcdshadow-driver to register itself against a connector later
>>>>> (which is non trivial wrt probe order, but lets ignore that for now).
>>>>
>>>> I regret dropping the ball on Rajat's series (sorry!).
>>>>
>>>> I do think having the connector property for this is the way to go.
>>>
>>> I 100% agree.
>>>
>>>> Even
>>>> if we couldn't necessarily figure out all the details on the kernel
>>>> internal connections, can we settle on the property though, so we could
>>>> move forward with Rajat's series?
>>
>> Thanks, it would be great!.
>>
>>>
>>> Yes please, this will also allow us to move forward with userspace
>>> support even if for testing that we do some hacks for the kernel's
>>> internal connections for now.
>>>
>>>> Moreover, do we actually need two properties, one which could indicate
>>>> userspace's desire for the property, and another that tells the hardware
>>>> state?
>>>
>>> No I do not think so. I would expect there to just be one property,
>>> I guess that if the state is (partly) firmware controlled then there
>>> might be a race, but we will need a notification mechanism (*) for
>>> firmware triggered state changes anyways, so shortly after loosing
>>> the race userspace will process the notification and it will know
>>> about it.
>>
>> I agree with Hans here that I think it would be better if we could do
>> it with one property.
>>
>>   * I can imagine demand for laptops that have a "hardware kill switch"
>> for privacy screen (just like there are for camera etc today). So I
>> think in future we may have to deal with this case anyway. In such
>> devices it's the hardware (as opposite to firmware) that will change
>> the state. The HW will likely provide an interrupt to the software to
>> notify of the change. This is all imaginative at this point though.
>>
>> * I think having 2 properties might be a confusing UAPI. Also, we have
>> existing properties like link-status that can be changed by both the
>> user and the hardware.
> 
> I think the consensus is that all properties that get changed by both
> userspace and the kernel are mistakes, and the way to handle it is to
> have two properties.

But the actual privacy screen has only 1 state, having two properties
for this will only be confusing. As I mentioned before we have a similar
case with e.g. keyboard backlighting and there the userspace API
also has a single sysfs attribute for the brightness, with change
notifications to userspace if the firmware changes the brightness
on its own and this works well.

What would the semantics of these 2 different properties be? And
what sort of extra functionality would these semantics offer which
the single property versions does not offer?

Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel