Re: [PATCH v7 09/11] drm: uevent for connector status change
On Thu, 4 Jul 2019 16:12:10 +0530
Ramalingam C wrote:
> On 2019-07-04 at 14:12:27 +0300, Pekka Paalanen wrote:
> > On Tue, 7 May 2019 21:57:43 +0530
> > Ramalingam C wrote:
> >
> > > DRM API for generating uevent for a status changes of connector's
> > > property.
> > >
> > > This uevent will have following details related to the status change:
> > >
> > > HOTPLUG=1, CONNECTOR= and PROPERTY=
> > >
> > > Need ACK from this uevent from userspace consumer.
> > >
> > > v2:
> > > Minor fixes at KDoc comments [Daniel]
> > > v3:
> > > Check the property is really attached with connector [Daniel]
> > >
> > > Signed-off-by: Ramalingam C
> > > Reviewed-by: Daniel Vetter
> > > ---
> > > drivers/gpu/drm/drm_sysfs.c | 35 +++
> > > include/drm/drm_sysfs.h | 5 -
> > > 2 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > > index 18b1ac442997..63fa951a20db 100644
> > > --- a/drivers/gpu/drm/drm_sysfs.c
> > > +++ b/drivers/gpu/drm/drm_sysfs.c
> > > @@ -21,6 +21,7 @@
> > > #include
> > > #include
> > > #include "drm_internal.h"
> > > +#include "drm_crtc_internal.h"
> > >
> > > #define to_drm_minor(d) dev_get_drvdata(d)
> > > #define to_drm_connector(d) dev_get_drvdata(d)
> > > @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> > > * Send a uevent for the DRM device specified by @dev. Currently we only
> > > * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> > > * deal with other types of events.
> > > + *
> > > + * Any new uapi should be using the drm_sysfs_connector_status_event()
> > > + * for uevents on connector status change.
> > > */
> > > void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > {
> > > @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > }
> > > EXPORT_SYMBOL(drm_sysfs_hotplug_event);
> > >
> > > +/**
> > > + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> > > + * property status change
> > > + * @connector: connector on which property status changed
> > > + * @property: connector property whoes status changed.
> > > + *
> > > + * Send a uevent for the DRM device specified by @dev. Currently we
> > > + * set HOTPLUG=1 and connector id along with the attached property id
> > > + * related to the status change.
> > > + */
> This is the kernel doc added for drm_sysfs_connector_status_event()
> similar to drm_sysfs_hotplug_event()
Hi,
yes, it is the kernel internal doc. An UAPI doc would spell out the
attributes "CONNECTOR" and "PROPERTY" and describe their values
explaining what they are, instead of decsribing a kernel-internal
function arguments.
However, as discussed, we cannot have UAPI docs at this time, so I
guess this is the best we can have.
Thanks,
pq
> > > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > > + struct drm_property *property)
> > > +{
> > > + struct drm_device *dev = connector->dev;
> > > + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> > > + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> > > +
> > > + WARN_ON(!drm_mode_obj_find_prop_id(>base,
> > > +property->base.id));
> > > +
> > > + snprintf(conn_id, ARRAY_SIZE(conn_id),
> > > + "CONNECTOR=%u", connector->base.id);
> > > + snprintf(prop_id, ARRAY_SIZE(prop_id),
> > > + "PROPERTY=%u", property->base.id);
> > > +
> > > + DRM_DEBUG("generating connector status event\n");
> > > +
> > > + kobject_uevent_env(>primary->kdev->kobj, KOBJ_CHANGE, envp);
> > > +}
> > > +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> > > +
> > > static void drm_sysfs_release(struct device *dev)
> > > {
> > > kfree(dev);
> > > diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> > > index 4f311e836cdc..d454ef617b2c 100644
> > > --- a/include/drm/drm_sysfs.h
> > > +++ b/include/drm/drm_sysfs.h
> > > @@ -4,10 +4,13 @@
> > >
> > > struct drm_device;
> > > struct device;
> > > +struct drm_connector;
> > > +struct drm_property;
> > >
> > > int drm_class_device_register(struct device *dev);
> > > void drm_class_device_unregister(struct device *dev);
> > >
> > > void drm_sysfs_hotplug_event(struct drm_device *dev);
> > > -
> > > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > > + struct drm_property *property);
> > > #endif
> >
> > Hi,
> >
> > this patch is completely missing the UAPI documentation.
> >
> > Weston in
> > https://gitlab.freedesktop.org/wayland/weston/merge_requests/48
> > does have good looking code to parse this event.
> >
> >
> > Thanks,
> > pq
>
>
pgpz7kkIrbFDI.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
Re: [PATCH v7 09/11] drm: uevent for connector status change
On 2019-07-04 at 14:12:27 +0300, Pekka Paalanen wrote:
> On Tue, 7 May 2019 21:57:43 +0530
> Ramalingam C wrote:
>
> > DRM API for generating uevent for a status changes of connector's
> > property.
> >
> > This uevent will have following details related to the status change:
> >
> > HOTPLUG=1, CONNECTOR= and PROPERTY=
> >
> > Need ACK from this uevent from userspace consumer.
> >
> > v2:
> > Minor fixes at KDoc comments [Daniel]
> > v3:
> > Check the property is really attached with connector [Daniel]
> >
> > Signed-off-by: Ramalingam C
> > Reviewed-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/drm_sysfs.c | 35 +++
> > include/drm/drm_sysfs.h | 5 -
> > 2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 18b1ac442997..63fa951a20db 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> > #include "drm_internal.h"
> > +#include "drm_crtc_internal.h"
> >
> > #define to_drm_minor(d) dev_get_drvdata(d)
> > #define to_drm_connector(d) dev_get_drvdata(d)
> > @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> > * Send a uevent for the DRM device specified by @dev. Currently we only
> > * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> > * deal with other types of events.
> > + *
> > + * Any new uapi should be using the drm_sysfs_connector_status_event()
> > + * for uevents on connector status change.
> > */
> > void drm_sysfs_hotplug_event(struct drm_device *dev)
> > {
> > @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> > }
> > EXPORT_SYMBOL(drm_sysfs_hotplug_event);
> >
> > +/**
> > + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> > + * property status change
> > + * @connector: connector on which property status changed
> > + * @property: connector property whoes status changed.
> > + *
> > + * Send a uevent for the DRM device specified by @dev. Currently we
> > + * set HOTPLUG=1 and connector id along with the attached property id
> > + * related to the status change.
> > + */
This is the kernel doc added for drm_sysfs_connector_status_event()
similar to drm_sysfs_hotplug_event()
-Ram
> > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > + struct drm_property *property)
> > +{
> > + struct drm_device *dev = connector->dev;
> > + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> > + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> > +
> > + WARN_ON(!drm_mode_obj_find_prop_id(>base,
> > + property->base.id));
> > +
> > + snprintf(conn_id, ARRAY_SIZE(conn_id),
> > +"CONNECTOR=%u", connector->base.id);
> > + snprintf(prop_id, ARRAY_SIZE(prop_id),
> > +"PROPERTY=%u", property->base.id);
> > +
> > + DRM_DEBUG("generating connector status event\n");
> > +
> > + kobject_uevent_env(>primary->kdev->kobj, KOBJ_CHANGE, envp);
> > +}
> > +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> > +
> > static void drm_sysfs_release(struct device *dev)
> > {
> > kfree(dev);
> > diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> > index 4f311e836cdc..d454ef617b2c 100644
> > --- a/include/drm/drm_sysfs.h
> > +++ b/include/drm/drm_sysfs.h
> > @@ -4,10 +4,13 @@
> >
> > struct drm_device;
> > struct device;
> > +struct drm_connector;
> > +struct drm_property;
> >
> > int drm_class_device_register(struct device *dev);
> > void drm_class_device_unregister(struct device *dev);
> >
> > void drm_sysfs_hotplug_event(struct drm_device *dev);
> > -
> > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > + struct drm_property *property);
> > #endif
>
> Hi,
>
> this patch is completely missing the UAPI documentation.
>
> Weston in
> https://gitlab.freedesktop.org/wayland/weston/merge_requests/48
> does have good looking code to parse this event.
>
>
> Thanks,
> pq
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, 7 May 2019 21:57:43 +0530
Ramalingam C wrote:
> DRM API for generating uevent for a status changes of connector's
> property.
>
> This uevent will have following details related to the status change:
>
> HOTPLUG=1, CONNECTOR= and PROPERTY=
>
> Need ACK from this uevent from userspace consumer.
>
> v2:
> Minor fixes at KDoc comments [Daniel]
> v3:
> Check the property is really attached with connector [Daniel]
>
> Signed-off-by: Ramalingam C
> Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_sysfs.c | 35 +++
> include/drm/drm_sysfs.h | 5 -
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 18b1ac442997..63fa951a20db 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include "drm_internal.h"
> +#include "drm_crtc_internal.h"
>
> #define to_drm_minor(d) dev_get_drvdata(d)
> #define to_drm_connector(d) dev_get_drvdata(d)
> @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> * Send a uevent for the DRM device specified by @dev. Currently we only
> * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> * deal with other types of events.
> + *
> + * Any new uapi should be using the drm_sysfs_connector_status_event()
> + * for uevents on connector status change.
> */
> void drm_sysfs_hotplug_event(struct drm_device *dev)
> {
> @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_sysfs_hotplug_event);
>
> +/**
> + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> + * property status change
> + * @connector: connector on which property status changed
> + * @property: connector property whoes status changed.
> + *
> + * Send a uevent for the DRM device specified by @dev. Currently we
> + * set HOTPLUG=1 and connector id along with the attached property id
> + * related to the status change.
> + */
> +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> + struct drm_property *property)
> +{
> + struct drm_device *dev = connector->dev;
> + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> +
> + WARN_ON(!drm_mode_obj_find_prop_id(>base,
> +property->base.id));
> +
> + snprintf(conn_id, ARRAY_SIZE(conn_id),
> + "CONNECTOR=%u", connector->base.id);
> + snprintf(prop_id, ARRAY_SIZE(prop_id),
> + "PROPERTY=%u", property->base.id);
> +
> + DRM_DEBUG("generating connector status event\n");
> +
> + kobject_uevent_env(>primary->kdev->kobj, KOBJ_CHANGE, envp);
> +}
> +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> +
> static void drm_sysfs_release(struct device *dev)
> {
> kfree(dev);
> diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> index 4f311e836cdc..d454ef617b2c 100644
> --- a/include/drm/drm_sysfs.h
> +++ b/include/drm/drm_sysfs.h
> @@ -4,10 +4,13 @@
>
> struct drm_device;
> struct device;
> +struct drm_connector;
> +struct drm_property;
>
> int drm_class_device_register(struct device *dev);
> void drm_class_device_unregister(struct device *dev);
>
> void drm_sysfs_hotplug_event(struct drm_device *dev);
> -
> +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> + struct drm_property *property);
> #endif
Hi,
this patch is completely missing the UAPI documentation.
Weston in
https://gitlab.freedesktop.org/wayland/weston/merge_requests/48
does have good looking code to parse this event.
Thanks,
pq
pgp8GmYVa2iCG.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 3 Jun 2019 15:19:13 + "Ser, Simon" wrote: > On Mon, 2019-06-03 at 17:08 +0200, Daniel Vetter wrote: > > It's definitely a very suboptimal situation. Not sure there's a good way > > out. The trouble here is that i915 ended up configuring crtc/connectors > > differently than -modesetting (to allow fastboot, which I think is still > > i915 exclusive). This then highlighted that modesetting can't do atomic > > modesets if you try to reassign connectors. > > > > One idea I have is that vgms would help compositors to play out a bunch of > > Just so people aren't confused: I think Daniel meant "vkms" here :P > > > standard scenarios, even automated. But that's not there yet, and every > > compositor project needs to care beyond "boots on my laptop, ship it". No > > idea that's even possible. > > Having documentation for userspace is also important IMHO. > > Regarding automated compositor testing, it's probably not possible to > have a single place where all compositors are tested: vkms should > probably be included as part of their CI. Thoughts? > > Anyway, we could start a discussion to see if compositor people are > interested. Or have you already talked to some compositor maintainers? FWIW, I would absolutely *love* to be able to exercise Weston's DRM-backend in Gitlab CI with anything, even just vkms, at least in Weston's test suite to test Weston against KMS in general. Once that is up, kernel people could replicate from that to their own CI for testing drivers against known userspace. I think it would be an awesome long term plan, whether uAPI specs appear or not. Long term, because I have no idea who could work on it when. In theory, all I would be waiting for is for vkms to be just featureful enough and to figure out a way how to get that running inside Gitlab CI. Thanks, pq pgpHn4YRqTbiB.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 2019-06-03 at 17:08 +0200, Daniel Vetter wrote: > On Mon, Jun 03, 2019 at 11:50:53AM +0200, Michel Dänzer wrote: > > On 2019-05-21 9:52 a.m., Daniel Vetter wrote: > > > On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen > > > wrote: > > > > On Mon, 20 May 2019 18:11:07 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > There's also a fairly easy fix for that -modesetting issue: We don't > > > > > expose atomic if the compositor has a process name of "Xserver". > > > > > Brutal, > > > > > but gets the job done. Once X is fixed, we can give a new "I'm not > > > > > totally > > > > > broken anymore" interface to get back at atomic. > > > > > > > > You mean "Xorg". Or maybe "X". Or maybe the setuid helper? Wait, do you > > > > check against the process issuing ioctl by ioctl, or the process that > > > > opened the device? Which would be logind? What about DRM leasing? ... > > > > > > In the Get/SetCaps ioctl we can do the check, which is called from X, > > > not logind. We just need some way to tell -modesetting apart from > > > everything else, and luckily there's not any other atomic X drivers. > > > > Not yet... > > > > As for a "I'm not totally broken anymore" interface, we did something > > like that (though kind of in the other direction) with > > RADEON_INFO_ACCEL_WORKING, but later RADEON_INFO_ACCEL_WORKING2 had to > > be added, because the former claimed acceleration was "working" in cases > > where it really wasn't... That kind of thing could become ugly in the > > long run if other Xorg driver start using atomic, and they'll inevitably > > be broken in different ways. > > It's definitely a very suboptimal situation. Not sure there's a good way > out. The trouble here is that i915 ended up configuring crtc/connectors > differently than -modesetting (to allow fastboot, which I think is still > i915 exclusive). This then highlighted that modesetting can't do atomic > modesets if you try to reassign connectors. > > One idea I have is that vgms would help compositors to play out a bunch of Just so people aren't confused: I think Daniel meant "vkms" here :P > standard scenarios, even automated. But that's not there yet, and every > compositor project needs to care beyond "boots on my laptop, ship it". No > idea that's even possible. Having documentation for userspace is also important IMHO. Regarding automated compositor testing, it's probably not possible to have a single place where all compositors are tested: vkms should probably be included as part of their CI. Thoughts? Anyway, we could start a discussion to see if compositor people are interested. Or have you already talked to some compositor maintainers? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, Jun 03, 2019 at 11:50:53AM +0200, Michel Dänzer wrote: > On 2019-05-21 9:52 a.m., Daniel Vetter wrote: > > On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen wrote: > >> On Mon, 20 May 2019 18:11:07 +0200 > >> Daniel Vetter wrote: > >> > >>> There's also a fairly easy fix for that -modesetting issue: We don't > >>> expose atomic if the compositor has a process name of "Xserver". Brutal, > >>> but gets the job done. Once X is fixed, we can give a new "I'm not totally > >>> broken anymore" interface to get back at atomic. > >> > >> You mean "Xorg". Or maybe "X". Or maybe the setuid helper? Wait, do you > >> check against the process issuing ioctl by ioctl, or the process that > >> opened the device? Which would be logind? What about DRM leasing? ... > > > > In the Get/SetCaps ioctl we can do the check, which is called from X, > > not logind. We just need some way to tell -modesetting apart from > > everything else, and luckily there's not any other atomic X drivers. > > Not yet... > > As for a "I'm not totally broken anymore" interface, we did something > like that (though kind of in the other direction) with > RADEON_INFO_ACCEL_WORKING, but later RADEON_INFO_ACCEL_WORKING2 had to > be added, because the former claimed acceleration was "working" in cases > where it really wasn't... That kind of thing could become ugly in the > long run if other Xorg driver start using atomic, and they'll inevitably > be broken in different ways. It's definitely a very suboptimal situation. Not sure there's a good way out. The trouble here is that i915 ended up configuring crtc/connectors differently than -modesetting (to allow fastboot, which I think is still i915 exclusive). This then highlighted that modesetting can't do atomic modesets if you try to reassign connectors. One idea I have is that vgms would help compositors to play out a bunch of standard scenarios, even automated. But that's not there yet, and every compositor project needs to care beyond "boots on my laptop, ship it". No idea that's even possible. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On 2019-05-21 9:52 a.m., Daniel Vetter wrote: > On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen wrote: >> On Mon, 20 May 2019 18:11:07 +0200 >> Daniel Vetter wrote: >> >>> There's also a fairly easy fix for that -modesetting issue: We don't >>> expose atomic if the compositor has a process name of "Xserver". Brutal, >>> but gets the job done. Once X is fixed, we can give a new "I'm not totally >>> broken anymore" interface to get back at atomic. >> >> You mean "Xorg". Or maybe "X". Or maybe the setuid helper? Wait, do you >> check against the process issuing ioctl by ioctl, or the process that >> opened the device? Which would be logind? What about DRM leasing? ... > > In the Get/SetCaps ioctl we can do the check, which is called from X, > not logind. We just need some way to tell -modesetting apart from > everything else, and luckily there's not any other atomic X drivers. Not yet... As for a "I'm not totally broken anymore" interface, we did something like that (though kind of in the other direction) with RADEON_INFO_ACCEL_WORKING, but later RADEON_INFO_ACCEL_WORKING2 had to be added, because the former claimed acceleration was "working" in cases where it really wasn't... That kind of thing could become ugly in the long run if other Xorg driver start using atomic, and they'll inevitably be broken in different ways. -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, May 21, 2019 at 12:01:29PM +0300, Pekka Paalanen wrote: > Hi Daniel, > > what says the assumption of the only monitor being driven by CRTC 0 > was a bad one? :-p > > It's probably not obvious that userspace needs to explicitly try to > avoid invalid configuration combinations by inspecting the current full > configuration and not just the one CRTC/connector it wants to use. Well the entire point of atomic is that you do set the entire config. The -modesetting atomic conversation tried to just use the atomic ioctl by 1:1 replacing legacy ioctl calls, and they screwed things up. If it would have applied the entire configuration in one go, it would work. > > All current atomic in -modesetting can do is pageflip and dpms off/on > > on the first screen on the first crtc. Anything more fancy goes boom, > > like where you change the connector/crtc links. > > > > It's _really_ broken :-) > > But it worked exactly that much, until a kernel change broke it, right? > Yes, I totally see the sillyness, but if it worked and we have these > no-regression rules... Which is why the offending patch has been reverted. Any the way forward is to just disable atomic from the kernel for X.org, because the legacy path actually works. We also have patches to disable atomic in -modesetting, but they're stuck. > > > I don't personally really like these rules, but if these are the rules, > > > then so be it. In my opinion it would be a huge step forward to get and > > > require uAPI specifications, that people could verify both kernel and > > > userspace against. Verifying against kernel code with no spec is what > > > leads to the -modesetting issue by the sounds of it. > > > > > > Documenting kernel internal interfaces is not it. People reading > > > DRM internal interface docs would need to know how DRM works internally > > > before they could map that information into uAPI, which makes it less > > > useful if not even useless for userspace developers. > > > > vgem is the idea here for validation, but if people ship atomic code > > that was never tested except for "boots on my laptop", then nothing is > > going to help. > > A testing pattern library with vkms would be awesome indeed. > > > And yes we have a huge gap with uapi documentation. btw for properties > > those section are meant to be useful for userspace people too: > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties > > > > and all subsequent chapters. I guess it's a bit burried, but this part > > is meant to be the uapi spec for properties. Is that also failing your > > expectations? > > Yes: it is hard to find (it is in Driver Developer's Guide, buried > several chapters in), it is interleaved with lots of DRM internal > details, makes references to DRM internal functions, and probably > relies on DRM internals behaviour through the references by not > repeating what they do. > > It is useful once you find it, but I don't think it's enough for making > good use in userspace for someone who hasn't been a DRM kernel > developer. Summarizing our long irc chat on this: I agree, everyone else agrees, but it's a question of making it happen and having someone with sufficient tech writer experience to make it useful. What we have now is essentially what happens if I type uapi specs (I pushed for these property docs). As you can see, it ain't pretty :-/ I think at least the process v4l has, where missing docs for uapi causes the doc build to fail, is sound. I have no idea how to adapt that to what we do, both from a "this will take a few years to fill the gaps and I don't know how to write good specs" and more implementation pov - properties can be created anywhere, no changes to include/uapi required. So hard to spot automatically. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, 21 May 2019 09:52:50 +0200 Daniel Vetter wrote: > On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen wrote: > > > > On Mon, 20 May 2019 18:11:07 +0200 > > Daniel Vetter wrote: > > > > > On Fri, May 17, 2019 at 01:08:24PM +0300, Pekka Paalanen wrote: > > > > On Thu, 16 May 2019 14:24:55 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: ... > > > > > > No, my concern is not an issue with netlink reliability. It is a > > > > > > potential issue when userspace chooses to not use netlink, and uses > > > > > > something else instead. I'm not sure what that else is but Paul says > > > > > > there is code in libudev and that is completely outside the control > > > > > > of > > > > > > KMS apps like display servers. > > > > > > > > > > afaik this other path only exists because it's the older one, for uapi > > > > > backwards compatibility with older userspace. Shouldn't be used for > > > > > anything. > > > > > > > > "Shouldn't be used" and someone screaming "kernel regression"... are you > > > > sure that path won't matter? > > > > > > > > Like some home-brewn distribution that happens to configure their > > > > libudev and kernel to use the old method, uses already new userspace, > > > > and then upgrades the kernel that starts sending fine-grained hotplug > > > > events, resulting the display server randomly handling hotplug wrong. > > > > > > > > Reading Airlie's recent rant about kernel regression handling make this > > > > a scary scenario where you would have no other choice than to rip all > > > > the fine-grained uevents out again. > > > > > > > > Is there any difference in the kernel code between the old method and > > > > the netlink method? Would it be possible to send fine-grained hotplug > > > > events only through netlink, and fall back to the old 'HOTPLUG=1' for > > > > the old method? > > > > > > There's a lot of grey in kernel regressions, and for fringe setups used by > > > few people I wouldn't worry about this. If they expect their shit to keep > > > working when using new stuff and crappy old interfaces, they get to keep > > > all the pieces. > > > > It didn't sound gray at all, reading Dave Airlie's email about it. If > > someone updates the kernel, and something works worse after that, then > > it is by definition a kernel regression. Period. And the earliest > > regression wins, i.e. if a revert breaks other things, the revert will > > be done regardless. > > > > > Dave's recent rant was a bit special, since userspace is clearly smoking > > > some strong stuff (-modesetting's atomic is seriously not using atomic > > > correctly), but it was also affecting too many people, and changing the > > > boot setup meant you'd get a black screen on boot-up already. Instead of > > > just on the first modeset with more than 1 screen. > > > > Then I think I missed the context of Dave's email. Reading it again, I > > still do not see that context. > > > > Btw. how do you determine "not using atomic correctly"? Has some uAPI > > specification for atomic appeared? I wasn't aware there was any uAPI > > specs, so there is no "incorrect use" if it happened to work once. > > -modesetting atomic blows up with more than one screen (even if you > just move that screen between crtc). The breakage was that with the > fastboot changes we've put the one screen onto crtc 1, but by default > modesetting wants it on crtc 0, and it couldn't do that switch > anymore. Hi Daniel, what says the assumption of the only monitor being driven by CRTC 0 was a bad one? :-p It's probably not obvious that userspace needs to explicitly try to avoid invalid configuration combinations by inspecting the current full configuration and not just the one CRTC/connector it wants to use. > All current atomic in -modesetting can do is pageflip and dpms off/on > on the first screen on the first crtc. Anything more fancy goes boom, > like where you change the connector/crtc links. > > It's _really_ broken :-) But it worked exactly that much, until a kernel change broke it, right? Yes, I totally see the sillyness, but if it worked and we have these no-regression rules... > > I don't personally really like these rules, but if these are the rules, > > then so be it. In my opinion it would be a huge step forward to get and > > require uAPI specifications, that people could verify both kernel and > > userspace against. Verifying against kernel code with no spec is what > > leads to the -modesetting issue by the sounds of it. > > > > Documenting kernel internal interfaces is not it. People reading > > DRM internal interface docs would need to know how DRM works internally > > before they could map that information into uAPI, which makes it less > > useful if not even useless for userspace developers. > > vgem is the idea here for validation, but if people ship atomic code > that was never tested except for "boots on my laptop", then nothing
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen wrote: > > On Mon, 20 May 2019 18:11:07 +0200 > Daniel Vetter wrote: > > > On Fri, May 17, 2019 at 01:08:24PM +0300, Pekka Paalanen wrote: > > > On Thu, 16 May 2019 14:24:55 +0200 > > > Daniel Vetter wrote: > > > > > > > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > > > > > On Wed, 15 May 2019 10:24:49 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > > > > > On Tue, 14 May 2019 16:34:01 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very > > > > > > > > > > > > particular > > > > > > > > > > > > property: > > > > > > > > > > > > > > > > > > > > > > > > With HDCP, there is a property that may change > > > > > > > > > > > > dynamically at runtime > > > > > > > > > > > > (the undesired/desired/enabled tristate). Userspace > > > > > > > > > > > > must be notified > > > > > > > > > > > > when it changes, I do not want userspace have to poll > > > > > > > > > > > > that property > > > > > > > > > > > > with a timer. > > > > > > > > > > > > > > > > > > > > > > > > When that property alone changes, and userspace is > > > > > > > > > > > > prepared to handle > > > > > > > > > > > > that property changing alone, it must not trigger a > > > > > > > > > > > > reprobe of the > > > > > > > > > > > > connector. There is no reason to reprobe at that point > > > > > > > > > > > > AFAIU. > > > > > > > > > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a > > > > > > > > > > > > reprobe with the > > > > > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > > > > > > > > > We need an event to userspace that indicates that > > > > > > > > > > > > re-reading the > > > > > > > > > > > > properties is enough and reprobe of the connector is > > > > > > > > > > > > not necessary. > > > > > > > > > > > > This is complementary to indicating to userspace that > > > > > > > > > > > > only some > > > > > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP > > > > > > > > > > > one, skip the > > > > > > > > > > > reprobing. Would that work? > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. > > > > > > > > > The replies > > > > > > > > > to Paul seemed to be going south so fast that I thought we > > > > > > > > > wouldn't get > > > > > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > > > > > > > > > Yes that's the idea, depending upon which property you get > > > > > > > > > > you know > > > > > > > > > > it's a sink change (needs full reprobe) or something else > > > > > > > > > > like hdcp > > > > > > > > > > state machinery update. > > > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we > > > > > > > > > > should > > > > > > > > > > indeed decouple that from the per-connector event for sink > > > > > > > > > > changes. > > > > > > > > > > That along is a good win already, since you know for which > > > > > > > > > > connector > > > > > > > > > > you need to call drmGetConnector (which forces the > > > > > > > > > > reprobe). It would > > > > > > > > > > be nice to only call drmGetConnectorCurrent (avoids the > > > > > > > > > > reprobe), but > > > > > > > > > > historically speaking every time we tried to rely on this > > > > > > > > > > we ended up > > > > > > > > > > regretting things. > > > > > > > > > > > > > > > > > > What changed? This sounds very much what Paul suggested. > > > > > > > > > Looking at it > > > > > > > > > from userspace side: > > > > > > > > > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > > > > > > > > > - If yy is "Content Protection", no need to > > > > > > > > > drmModeGetConnector(), just > > > > > > > > > re-get the connector properties. > > > > > > > > > > > > > > > > > > - Kernel probably shouldn't bother sending this for > > > > > > > > > properties where > > > > > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 20 May 2019 18:11:07 +0200 Daniel Vetter wrote: > On Fri, May 17, 2019 at 01:08:24PM +0300, Pekka Paalanen wrote: > > On Thu, 16 May 2019 14:24:55 +0200 > > Daniel Vetter wrote: > > > > > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > > > > On Wed, 15 May 2019 10:24:49 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > > > > On Tue, 14 May 2019 16:34:01 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very > > > > > > > > > > > particular > > > > > > > > > > > property: > > > > > > > > > > > > > > > > > > > > > > With HDCP, there is a property that may change > > > > > > > > > > > dynamically at runtime > > > > > > > > > > > (the undesired/desired/enabled tristate). Userspace must > > > > > > > > > > > be notified > > > > > > > > > > > when it changes, I do not want userspace have to poll > > > > > > > > > > > that property > > > > > > > > > > > with a timer. > > > > > > > > > > > > > > > > > > > > > > When that property alone changes, and userspace is > > > > > > > > > > > prepared to handle > > > > > > > > > > > that property changing alone, it must not trigger a > > > > > > > > > > > reprobe of the > > > > > > > > > > > connector. There is no reason to reprobe at that point > > > > > > > > > > > AFAIU. > > > > > > > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a > > > > > > > > > > > reprobe with the > > > > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > > > > > > > We need an event to userspace that indicates that > > > > > > > > > > > re-reading the > > > > > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > > > > > necessary. > > > > > > > > > > > This is complementary to indicating to userspace that > > > > > > > > > > > only some > > > > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP > > > > > > > > > > one, skip the > > > > > > > > > > reprobing. Would that work? > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The > > > > > > > > replies > > > > > > > > to Paul seemed to be going south so fast that I thought we > > > > > > > > wouldn't get > > > > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > > > > > > > Yes that's the idea, depending upon which property you get > > > > > > > > > you know > > > > > > > > > it's a sink change (needs full reprobe) or something else > > > > > > > > > like hdcp > > > > > > > > > state machinery update. > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we > > > > > > > > > should > > > > > > > > > indeed decouple that from the per-connector event for sink > > > > > > > > > changes. > > > > > > > > > That along is a good win already, since you know for which > > > > > > > > > connector > > > > > > > > > you need to call drmGetConnector (which forces the reprobe). > > > > > > > > > It would > > > > > > > > > be nice to only call drmGetConnectorCurrent (avoids the > > > > > > > > > reprobe), but > > > > > > > > > historically speaking every time we tried to rely on this we > > > > > > > > > ended up > > > > > > > > > regretting things. > > > > > > > > > > > > > > > > What changed? This sounds very much what Paul suggested. > > > > > > > > Looking at it > > > > > > > > from userspace side: > > > > > > > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > > > > > > > - If yy is "Content Protection", no need to > > > > > > > > drmModeGetConnector(), just > > > > > > > > re-get the connector properties. > > > > > > > > > > > > > > > > - Kernel probably shouldn't bother sending this for properties > > > > > > > > where > > > > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we should assert that the kernel can get the new
Re: [PATCH v7 09/11] drm: uevent for connector status change
Le lundi 20 mai 2019 à 18:11 +0200, Daniel Vetter a écrit : > On Fri, May 17, 2019 at 01:08:24PM +0300, Pekka Paalanen wrote: > > On Thu, 16 May 2019 14:24:55 +0200 > > Daniel Vetter wrote: > > > > > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > > > > On Wed, 15 May 2019 10:24:49 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > > > > On Tue, 14 May 2019 16:34:01 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > > > > wrote: > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > > > > wrote: > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very > > > > > > > > > > > particular > > > > > > > > > > > property: > > > > > > > > > > > > > > > > > > > > > > With HDCP, there is a property that may change > > > > > > > > > > > dynamically at runtime > > > > > > > > > > > (the undesired/desired/enabled tristate). Userspace must > > > > > > > > > > > be notified > > > > > > > > > > > when it changes, I do not want userspace have to poll > > > > > > > > > > > that property > > > > > > > > > > > with a timer. > > > > > > > > > > > > > > > > > > > > > > When that property alone changes, and userspace is > > > > > > > > > > > prepared to handle > > > > > > > > > > > that property changing alone, it must not trigger a > > > > > > > > > > > reprobe of the > > > > > > > > > > > connector. There is no reason to reprobe at that point > > > > > > > > > > > AFAIU. > > > > > > > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a > > > > > > > > > > > reprobe with the > > > > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > > > > > > > We need an event to userspace that indicates that > > > > > > > > > > > re-reading the > > > > > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > > > > > necessary. > > > > > > > > > > > This is complementary to indicating to userspace that > > > > > > > > > > > only some > > > > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP > > > > > > > > > > one, skip the > > > > > > > > > > reprobing. Would that work? > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The > > > > > > > > replies > > > > > > > > to Paul seemed to be going south so fast that I thought we > > > > > > > > wouldn't get > > > > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > > > > > > > Yes that's the idea, depending upon which property you get > > > > > > > > > you know > > > > > > > > > it's a sink change (needs full reprobe) or something else > > > > > > > > > like hdcp > > > > > > > > > state machinery update. > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we > > > > > > > > > should > > > > > > > > > indeed decouple that from the per-connector event for sink > > > > > > > > > changes. > > > > > > > > > That along is a good win already, since you know for which > > > > > > > > > connector > > > > > > > > > you need to call drmGetConnector (which forces the reprobe). > > > > > > > > > It would > > > > > > > > > be nice to only call drmGetConnectorCurrent (avoids the > > > > > > > > > reprobe), but > > > > > > > > > historically speaking every time we tried to rely on this we > > > > > > > > > ended up > > > > > > > > > regretting things. > > > > > > > > > > > > > > > > What changed? This sounds very much what Paul suggested. > > > > > > > > Looking at it > > > > > > > > from userspace side: > > > > > > > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > > > > > > > - If yy is "Content Protection", no need to > > > > > > > > drmModeGetConnector(), just > > > > > > > > re-get the connector properties. > > > > > > > > > > > > > > > > - Kernel probably shouldn't bother sending this for properties > > > > > > > > where > > > > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > > > > > > I think we should assert that the kernel can get the new property > > > > > > > values using drmModeGetConnectorCurrent for this case, i.e. the > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Fri, May 17, 2019 at 01:08:24PM +0300, Pekka Paalanen wrote: > On Thu, 16 May 2019 14:24:55 +0200 > Daniel Vetter wrote: > > > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > > > On Wed, 15 May 2019 10:24:49 +0200 > > > Daniel Vetter wrote: > > > > > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > > > On Tue, 14 May 2019 16:34:01 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > > > wrote: > > > > > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > > > > > ... > > > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very > > > > > > > > > > particular > > > > > > > > > > property: > > > > > > > > > > > > > > > > > > > > With HDCP, there is a property that may change dynamically > > > > > > > > > > at runtime > > > > > > > > > > (the undesired/desired/enabled tristate). Userspace must be > > > > > > > > > > notified > > > > > > > > > > when it changes, I do not want userspace have to poll that > > > > > > > > > > property > > > > > > > > > > with a timer. > > > > > > > > > > > > > > > > > > > > When that property alone changes, and userspace is prepared > > > > > > > > > > to handle > > > > > > > > > > that property changing alone, it must not trigger a reprobe > > > > > > > > > > of the > > > > > > > > > > connector. There is no reason to reprobe at that point > > > > > > > > > > AFAIU. > > > > > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a > > > > > > > > > > reprobe with the > > > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > > > > > We need an event to userspace that indicates that > > > > > > > > > > re-reading the > > > > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > > > > necessary. > > > > > > > > > > This is complementary to indicating to userspace that only > > > > > > > > > > some > > > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, > > > > > > > > > skip the > > > > > > > > > reprobing. Would that work? > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The > > > > > > > replies > > > > > > > to Paul seemed to be going south so fast that I thought we > > > > > > > wouldn't get > > > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > > > > > Yes that's the idea, depending upon which property you get you > > > > > > > > know > > > > > > > > it's a sink change (needs full reprobe) or something else like > > > > > > > > hdcp > > > > > > > > state machinery update. > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we > > > > > > > > should > > > > > > > > indeed decouple that from the per-connector event for sink > > > > > > > > changes. > > > > > > > > That along is a good win already, since you know for which > > > > > > > > connector > > > > > > > > you need to call drmGetConnector (which forces the reprobe). It > > > > > > > > would > > > > > > > > be nice to only call drmGetConnectorCurrent (avoids the > > > > > > > > reprobe), but > > > > > > > > historically speaking every time we tried to rely on this we > > > > > > > > ended up > > > > > > > > regretting things. > > > > > > > > > > > > > > What changed? This sounds very much what Paul suggested. Looking > > > > > > > at it > > > > > > > from userspace side: > > > > > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > > > > > - If yy is "Content Protection", no need to > > > > > > > drmModeGetConnector(), just > > > > > > > re-get the connector properties. > > > > > > > > > > > > > > - Kernel probably shouldn't bother sending this for properties > > > > > > > where > > > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > > > > > > > > > > I think we should assert that the kernel can get the new property > > > > > > values using drmModeGetConnectorCurrent for this case, i.e. the > > > > > > kernel > > > > > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > > > > > to "must" > > > > > > > > > > Hi Daniel, > > > > > > > > > > ok, that's good. > > > > > > > > > > > Furthermore different property can indicate different kind of > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Thu, 16 May 2019 14:24:55 +0200 Daniel Vetter wrote: > On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > > On Wed, 15 May 2019 10:24:49 +0200 > > Daniel Vetter wrote: > > > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > > On Tue, 14 May 2019 16:34:01 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > > wrote: > > > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > > > ... > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very > > > > > > > > > particular > > > > > > > > > property: > > > > > > > > > > > > > > > > > > With HDCP, there is a property that may change dynamically at > > > > > > > > > runtime > > > > > > > > > (the undesired/desired/enabled tristate). Userspace must be > > > > > > > > > notified > > > > > > > > > when it changes, I do not want userspace have to poll that > > > > > > > > > property > > > > > > > > > with a timer. > > > > > > > > > > > > > > > > > > When that property alone changes, and userspace is prepared > > > > > > > > > to handle > > > > > > > > > that property changing alone, it must not trigger a reprobe > > > > > > > > > of the > > > > > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a > > > > > > > > > reprobe with the > > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > > > We need an event to userspace that indicates that re-reading > > > > > > > > > the > > > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > > > necessary. > > > > > > > > > This is complementary to indicating to userspace that only > > > > > > > > > some > > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, > > > > > > > > skip the > > > > > > > > reprobing. Would that work? > > > > > > > > > > > > Hi, > > > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The > > > > > > replies > > > > > > to Paul seemed to be going south so fast that I thought we wouldn't > > > > > > get > > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > > > Yes that's the idea, depending upon which property you get you > > > > > > > know > > > > > > > it's a sink change (needs full reprobe) or something else like > > > > > > > hdcp > > > > > > > state machinery update. > > > > > > > > > > > > Right. > > > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > > > > > indeed decouple that from the per-connector event for sink > > > > > > > changes. > > > > > > > That along is a good win already, since you know for which > > > > > > > connector > > > > > > > you need to call drmGetConnector (which forces the reprobe). It > > > > > > > would > > > > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), > > > > > > > but > > > > > > > historically speaking every time we tried to rely on this we > > > > > > > ended up > > > > > > > regretting things. > > > > > > > > > > > > What changed? This sounds very much what Paul suggested. Looking at > > > > > > it > > > > > > from userspace side: > > > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), > > > > > > just > > > > > > re-get the connector properties. > > > > > > > > > > > > - Kernel probably shouldn't bother sending this for properties where > > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > > > > > > > I think we should assert that the kernel can get the new property > > > > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > > > > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > > > > to "must" > > > > > > > > Hi Daniel, > > > > > > > > ok, that's good. > > > > > > > > > Furthermore different property can indicate different kind of updates, > > > > > e.g. hdcp vs general sink change vs. whatever else might come in the > > > > > future. > > > > > > > > What do you mean by different kinds of updates? > > > > > > Atm we're discussing two: > > > > > > - "Content Protection" > > > - "sink changed, but you don't need to reprobe" this would be quite a bit > > > a catch all from the output
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote: > On Wed, 15 May 2019 10:24:49 +0200 > Daniel Vetter wrote: > > > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > > On Tue, 14 May 2019 16:34:01 +0200 > > > Daniel Vetter wrote: > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > > wrote: > > > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > > > ... > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > just to clarify the first case, specific to one very particular > > > > > > > > property: > > > > > > > > > > > > > > > > With HDCP, there is a property that may change dynamically at > > > > > > > > runtime > > > > > > > > (the undesired/desired/enabled tristate). Userspace must be > > > > > > > > notified > > > > > > > > when it changes, I do not want userspace have to poll that > > > > > > > > property > > > > > > > > with a timer. > > > > > > > > > > > > > > > > When that property alone changes, and userspace is prepared to > > > > > > > > handle > > > > > > > > that property changing alone, it must not trigger a reprobe of > > > > > > > > the > > > > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe > > > > > > > > with the > > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > > necessary. > > > > > > > > This is complementary to indicating to userspace that only some > > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, > > > > > > > skip the > > > > > > > reprobing. Would that work? > > > > > > > > > > Hi, > > > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The > > > > > replies > > > > > to Paul seemed to be going south so fast that I thought we wouldn't > > > > > get > > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > > > Yes that's the idea, depending upon which property you get you know > > > > > > it's a sink change (needs full reprobe) or something else like hdcp > > > > > > state machinery update. > > > > > > > > > > Right. > > > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > > > > indeed decouple that from the per-connector event for sink changes. > > > > > > That along is a good win already, since you know for which connector > > > > > > you need to call drmGetConnector (which forces the reprobe). It > > > > > > would > > > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), > > > > > > but > > > > > > historically speaking every time we tried to rely on this we ended > > > > > > up > > > > > > regretting things. > > > > > > > > > > What changed? This sounds very much what Paul suggested. Looking at it > > > > > from userspace side: > > > > > > > > This sounds solid, some refinements below: > > > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), > > > > > just > > > > > re-get the connector properties. > > > > > > > > > > - Kernel probably shouldn't bother sending this for properties where > > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > > > > I think we should assert that the kernel can get the new property > > > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > > > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > > > to "must" > > > > > > Hi Daniel, > > > > > > ok, that's good. > > > > > > > Furthermore different property can indicate different kind of updates, > > > > e.g. hdcp vs general sink change vs. whatever else might come in the > > > > future. > > > > > > What do you mean by different kinds of updates? > > > > Atm we're discussing two: > > > > - "Content Protection" > > - "sink changed, but you don't need to reprobe" this would be quite a bit > > a catch all from the output detection. Paul thinks differently, but I'm > > not sold on splitting this up more, at least not right now. This would > > include connector status (and related things returned by drmGetConnector > > which currently aren't a property), EDID, the mst path id, that kind of > > stuff. > > > > Ime once we have 2, there's more bound to come :-) > > Hi Daniel, > > I don't understand what the "sink changed" thing could be, but sure, > there can be
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Wed, 15 May 2019 10:24:49 +0200 Daniel Vetter wrote: > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > > On Tue, 14 May 2019 16:34:01 +0200 > > Daniel Vetter wrote: > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen > > > wrote: > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > > wrote: > > > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > ... > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > just to clarify the first case, specific to one very particular > > > > > > > property: > > > > > > > > > > > > > > With HDCP, there is a property that may change dynamically at > > > > > > > runtime > > > > > > > (the undesired/desired/enabled tristate). Userspace must be > > > > > > > notified > > > > > > > when it changes, I do not want userspace have to poll that > > > > > > > property > > > > > > > with a timer. > > > > > > > > > > > > > > When that property alone changes, and userspace is prepared to > > > > > > > handle > > > > > > > that property changing alone, it must not trigger a reprobe of the > > > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe > > > > > > > with the > > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > > > > properties is enough and reprobe of the connector is not > > > > > > > necessary. > > > > > > > This is complementary to indicating to userspace that only some > > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip > > > > > > the > > > > > > reprobing. Would that work? > > > > > > > > Hi, > > > > > > > > yes, that would work, if it was acceptable to DRM upstream. The replies > > > > to Paul seemed to be going south so fast that I thought we wouldn't get > > > > any new uevent fields in favour of "epoch counters". > > > > > > > > > Yes that's the idea, depending upon which property you get you know > > > > > it's a sink change (needs full reprobe) or something else like hdcp > > > > > state machinery update. > > > > > > > > Right. > > > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > > > indeed decouple that from the per-connector event for sink changes. > > > > > That along is a good win already, since you know for which connector > > > > > you need to call drmGetConnector (which forces the reprobe). It would > > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but > > > > > historically speaking every time we tried to rely on this we ended up > > > > > regretting things. > > > > > > > > What changed? This sounds very much what Paul suggested. Looking at it > > > > from userspace side: > > > > > > This sounds solid, some refinements below: > > > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), just > > > > re-get the connector properties. > > > > > > > > - Kernel probably shouldn't bother sending this for properties where > > > > re-probe could be necessary, and send the below instead. > > > > > > > > > I think we should assert that the kernel can get the new property > > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > > to "must" > > > > Hi Daniel, > > > > ok, that's good. > > > > > Furthermore different property can indicate different kind of updates, > > > e.g. hdcp vs general sink change vs. whatever else might come in the > > > future. > > > > What do you mean by different kinds of updates? > > Atm we're discussing two: > > - "Content Protection" > - "sink changed, but you don't need to reprobe" this would be quite a bit > a catch all from the output detection. Paul thinks differently, but I'm > not sold on splitting this up more, at least not right now. This would > include connector status (and related things returned by drmGetConnector > which currently aren't a property), EDID, the mst path id, that kind of > stuff. > > Ime once we have 2, there's more bound to come :-) Hi Daniel, I don't understand what the "sink changed" thing could be, but sure, there can be more. > > Btw. I started thinking, maybe we should completely leave out the "If > > yy is "Content Protection"" and require the kernel to guarantee, that > > if PROPERTY is set, then drmModeGetConnector() (probing) must not be > > necessary based on this event alone. > > > > Writing it down again: > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > - yy denotes which connector xx
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote: > On Tue, 14 May 2019 16:34:01 +0200 > Daniel Vetter wrote: > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen wrote: > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > Daniel Vetter wrote: > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > wrote: > > > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > ... > > > > > > > Hi Daniel, > > > > > > > > > > > > just to clarify the first case, specific to one very particular > > > > > > property: > > > > > > > > > > > > With HDCP, there is a property that may change dynamically at > > > > > > runtime > > > > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > > > > when it changes, I do not want userspace have to poll that property > > > > > > with a timer. > > > > > > > > > > > > When that property alone changes, and userspace is prepared to > > > > > > handle > > > > > > that property changing alone, it must not trigger a reprobe of the > > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe > > > > > > with the > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > > > properties is enough and reprobe of the connector is not necessary. > > > > > > This is complementary to indicating to userspace that only some > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > > > > reprobing. Would that work? > > > > > > Hi, > > > > > > yes, that would work, if it was acceptable to DRM upstream. The replies > > > to Paul seemed to be going south so fast that I thought we wouldn't get > > > any new uevent fields in favour of "epoch counters". > > > > > > > Yes that's the idea, depending upon which property you get you know > > > > it's a sink change (needs full reprobe) or something else like hdcp > > > > state machinery update. > > > > > > Right. > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > > indeed decouple that from the per-connector event for sink changes. > > > > That along is a good win already, since you know for which connector > > > > you need to call drmGetConnector (which forces the reprobe). It would > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but > > > > historically speaking every time we tried to rely on this we ended up > > > > regretting things. > > > > > > What changed? This sounds very much what Paul suggested. Looking at it > > > from userspace side: > > > > This sounds solid, some refinements below: > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), just > > > re-get the connector properties. > > > > > > - Kernel probably shouldn't bother sending this for properties where > > > re-probe could be necessary, and send the below instead. > > > > > > I think we should assert that the kernel can get the new property > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > to "must" > > Hi Daniel, > > ok, that's good. > > > Furthermore different property can indicate different kind of updates, > > e.g. hdcp vs general sink change vs. whatever else might come in the > > future. > > What do you mean by different kinds of updates? Atm we're discussing two: - "Content Protection" - "sink changed, but you don't need to reprobe" this would be quite a bit a catch all from the output detection. Paul thinks differently, but I'm not sold on splitting this up more, at least not right now. This would include connector status (and related things returned by drmGetConnector which currently aren't a property), EDID, the mst path id, that kind of stuff. Ime once we have 2, there's more bound to come :-) > Btw. I started thinking, maybe we should completely leave out the "If > yy is "Content Protection"" and require the kernel to guarantee, that > if PROPERTY is set, then drmModeGetConnector() (probing) must not be > necessary based on this event alone. > > Writing it down again: > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > - yy denotes which connector xx property changed. Maybe yy denotes which group of properties changed, and part of the uapi is picking the canonical one. E.g. content protection might also gain more properties in the future (there's patches, but the userspace won't be open sourced). And for that case I don't think we should then send an even for every single individual property, but just for the lead property. Maybe we should change it to UPDATE_TYPE=, but it felt better to use the property id we already have for
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi, On Wed, 2019-05-15 at 10:37 +0300, Pekka Paalanen wrote: > On Tue, 14 May 2019 16:34:01 +0200 > Daniel Vetter wrote: > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen wrote: > > > On Tue, 14 May 2019 13:02:09 +0200 > > > Daniel Vetter wrote: > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon > > > > wrote: > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > ... > > > > > > > Hi Daniel, > > > > > > > > > > > > just to clarify the first case, specific to one very particular > > > > > > property: > > > > > > > > > > > > With HDCP, there is a property that may change dynamically at > > > > > > runtime > > > > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > > > > when it changes, I do not want userspace have to poll that property > > > > > > with a timer. > > > > > > > > > > > > When that property alone changes, and userspace is prepared to > > > > > > handle > > > > > > that property changing alone, it must not trigger a reprobe of the > > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe > > > > > > with the > > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > > > properties is enough and reprobe of the connector is not necessary. > > > > > > This is complementary to indicating to userspace that only some > > > > > > connectors need to be reprobed instead of everything. > > > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > > > > reprobing. Would that work? > > > > > > Hi, > > > > > > yes, that would work, if it was acceptable to DRM upstream. The replies > > > to Paul seemed to be going south so fast that I thought we wouldn't get > > > any new uevent fields in favour of "epoch counters". > > > > > > > Yes that's the idea, depending upon which property you get you know > > > > it's a sink change (needs full reprobe) or something else like hdcp > > > > state machinery update. > > > > > > Right. > > > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > > indeed decouple that from the per-connector event for sink changes. > > > > That along is a good win already, since you know for which connector > > > > you need to call drmGetConnector (which forces the reprobe). It would > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but > > > > historically speaking every time we tried to rely on this we ended up > > > > regretting things. > > > > > > What changed? This sounds very much what Paul suggested. Looking at it > > > from userspace side: > > > > This sounds solid, some refinements below: > > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), just > > > re-get the connector properties. > > > > > > - Kernel probably shouldn't bother sending this for properties where > > > re-probe could be necessary, and send the below instead. > > > > I think we should assert that the kernel can get the new property > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > > does not expect a full reprobe. I.e. upgrade your idea from "should" > > to "must" > > Hi Daniel, > > ok, that's good. > > > Furthermore different property can indicate different kind of updates, > > e.g. hdcp vs general sink change vs. whatever else might come in the > > future. > > What do you mean by different kinds of updates? > > Btw. I started thinking, maybe we should completely leave out the "If > yy is "Content Protection"" and require the kernel to guarantee, that > if PROPERTY is set, then drmModeGetConnector() (probing) must not be > necessary based on this event alone. I agree, this is precisely what I had in mind. > Writing it down again: > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > - yy denotes which connector xx property changed. > > - Userspace does not need to do drmModeGetConnector(), it only needs to > drmModeObjectGetProperties() on the connector to receive the new > updated property values. > > - Kernel must not send this event for changes that may require probing > for correct results, exceptional conditions (buggy hardware, etc.) > included. Instead, the kernel must send one of the below events. Agreed, and leave this up to the driver in the end, not the core. > Is there actually anything interesting that > drmModeGetConnectorCurrent() could guaranteed correctly return that is > not a property already? I'd probably leave this consideration out > completely, and just say do one of the needs-probing events if anything > there changed. In the end, I think this should help move to a situation where userspace would not have to do a reprobe at any point and the kernel side just does it. I see no justification for
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Wed, May 15, 2019 at 09:43:24AM +0200, Paul Kocialkowski wrote: > Hi, > > On Tue, 2019-05-14 at 16:28 +0200, Daniel Vetter wrote: > > On Tue, May 14, 2019 at 4:13 PM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > On Tue, 2019-05-14 at 13:09 +0200, Daniel Vetter wrote: > > > > On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski > > > > wrote: > > > > > Hey, > > > > > > > > > > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > > > wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > > > connector's > > > > > > > > > > property. > > > > > > > > > > > > > > > > > > > > This uevent will have following details related to the > > > > > > > > > > status change: > > > > > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and > > > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > > > hotplug > > > > > > > > > issue and came up with similar ideas: > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have > > > > > > > > > a more or > > > > > > > > > less fine grain of uevent reporting depending on what > > > > > > > > > happened. The > > > > > > > > > point is that we need to cover different cases: > > > > > > > > > - one or more properties changed; > > > > > > > > > - the connector status changed; > > > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > > > HOTPLUG=1 > > > > > > > > > CONNECTOR= > > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > > > > > For the second one, something like: > > > > > > > > > HOTPLUG=1 > > > > > > > > > CONNECTOR= > > > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > > > disconnected; > > > > > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > > > HOTPLUG=1 > > > > > > > > > CONNECTOR= > > > > > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > > > > > There's no material difference between this one and the > > > > > > > > previous one. > > > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > > > property, > > > > > > > > i.e. we can reuse the same PROPERTY= > > > > > > > > trick. > > > > > > > > > > > > > > That's the idea, but we need to handle status changes differently > > > > > > > than > > > > > > > properties, since as far as I know, connected/unconnected status > > > > > > > is not > > > > > > > exposed as a prop for the connector. > > > > > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > > > into a property. Or another excuse why we should expose the epoch > > > > > > property :-) > > > > > > > > > > Well I think it would make sense anyway, as long as we can make sure > > > > > it > > > > > stays consistent with the one reported in the connector struct. > > > > > > > > > > > > > Here's why: > > > > > > > > - A side effect of forcing a probe on a connector is that you > > > > > > > > get to > > > > > > > > read all the properties, so supplying them is kinda pointless. > > > > > > > > > > > > > > Agreed, except for the status case where it's useful to know it's > > > > > > > a > > > > > > > disconnect, because we don't need any probe step in that case. > > > > > > > > > > > > > > > - You can read STATUS without forcing a reprobe, if you want to > > > > > > > > avoid > > > > > > > > the reprobe for disconnected. I'd kinda not recommend that > > > > > > > > though, > > > > > > > > feels a bit like overoptimizing. And for reasonable connectors > > > > > > > > (i.e. > > > > > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > > > > > reasonable unfortunately, but oh well. > > > > > > > > > > > > > > How would that be retreived then? From the looks of it, that's a > > > > > > > MODE_GETCONNECTOR ioctl and I was under the impression this is > > > > > > > what > > > > > > > does the full reprobe. > > > > > > > > > > > > drmGetConnector vs drmGetConnectorCurrent. > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi, On Tue, 2019-05-14 at 16:28 +0200, Daniel Vetter wrote: > On Tue, May 14, 2019 at 4:13 PM Paul Kocialkowski > wrote: > > Hi, > > > > On Tue, 2019-05-14 at 13:09 +0200, Daniel Vetter wrote: > > > On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski > > > wrote: > > > > Hey, > > > > > > > > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > > wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > > connector's > > > > > > > > > property. > > > > > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > > > change: > > > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and > > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > > hotplug > > > > > > > > issue and came up with similar ideas: > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have a > > > > > > > > more or > > > > > > > > less fine grain of uevent reporting depending on what happened. > > > > > > > > The > > > > > > > > point is that we need to cover different cases: > > > > > > > > - one or more properties changed; > > > > > > > > - the connector status changed; > > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > > > For the second one, something like: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > > disconnected; > > > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > > > There's no material difference between this one and the previous > > > > > > > one. > > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > > property, > > > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > > > > > That's the idea, but we need to handle status changes differently > > > > > > than > > > > > > properties, since as far as I know, connected/unconnected status is > > > > > > not > > > > > > exposed as a prop for the connector. > > > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > > into a property. Or another excuse why we should expose the epoch > > > > > property :-) > > > > > > > > Well I think it would make sense anyway, as long as we can make sure it > > > > stays consistent with the one reported in the connector struct. > > > > > > > > > > > Here's why: > > > > > > > - A side effect of forcing a probe on a connector is that you get > > > > > > > to > > > > > > > read all the properties, so supplying them is kinda pointless. > > > > > > > > > > > > Agreed, except for the status case where it's useful to know it's a > > > > > > disconnect, because we don't need any probe step in that case. > > > > > > > > > > > > > - You can read STATUS without forcing a reprobe, if you want to > > > > > > > avoid > > > > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > > > > feels a bit like overoptimizing. And for reasonable connectors > > > > > > > (i.e. > > > > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > > > > reasonable unfortunately, but oh well. > > > > > > > > > > > > How would that be retreived then? From the looks of it, that's a > > > > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > > > > does the full reprobe. > > > > > > > > > > drmGetConnector vs drmGetConnectorCurrent. > > > > > > > > Ah right, forgot about that one, thanks. > > > > > > > > > > Not sure what issues could arise in case of disconnect without > > > > > > reprobe > > > > > > -- at least I don't see why userspace should have to do anything in > > > > > > particular except no longer using the connector, even in complex DP > > > > > > MST > > > > > > cases. > > > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, 14 May 2019 16:34:01 +0200 Daniel Vetter wrote: > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen wrote: > > > > On Tue, 14 May 2019 13:02:09 +0200 > > Daniel Vetter wrote: > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon wrote: > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: ... > > > > > Hi Daniel, > > > > > > > > > > just to clarify the first case, specific to one very particular > > > > > property: > > > > > > > > > > With HDCP, there is a property that may change dynamically at runtime > > > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > > > when it changes, I do not want userspace have to poll that property > > > > > with a timer. > > > > > > > > > > When that property alone changes, and userspace is prepared to handle > > > > > that property changing alone, it must not trigger a reprobe of the > > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe with > > > > > the > > > > > epoch approach or with any alternate uevent design? > > > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > > properties is enough and reprobe of the connector is not necessary. > > > > > This is complementary to indicating to userspace that only some > > > > > connectors need to be reprobed instead of everything. > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > > > reprobing. Would that work? > > > > Hi, > > > > yes, that would work, if it was acceptable to DRM upstream. The replies > > to Paul seemed to be going south so fast that I thought we wouldn't get > > any new uevent fields in favour of "epoch counters". > > > > > Yes that's the idea, depending upon which property you get you know > > > it's a sink change (needs full reprobe) or something else like hdcp > > > state machinery update. > > > > Right. > > > > > Wrt avoiding the full reprobe for sink changes: I think we should > > > indeed decouple that from the per-connector event for sink changes. > > > That along is a good win already, since you know for which connector > > > you need to call drmGetConnector (which forces the reprobe). It would > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but > > > historically speaking every time we tried to rely on this we ended up > > > regretting things. > > > > What changed? This sounds very much what Paul suggested. Looking at it > > from userspace side: > > This sounds solid, some refinements below: > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), just > > re-get the connector properties. > > > > - Kernel probably shouldn't bother sending this for properties where > > re-probe could be necessary, and send the below instead. > > > I think we should assert that the kernel can get the new property > values using drmModeGetConnectorCurrent for this case, i.e. the kernel > does not expect a full reprobe. I.e. upgrade your idea from "should" > to "must" Hi Daniel, ok, that's good. > Furthermore different property can indicate different kind of updates, > e.g. hdcp vs general sink change vs. whatever else might come in the > future. What do you mean by different kinds of updates? Btw. I started thinking, maybe we should completely leave out the "If yy is "Content Protection"" and require the kernel to guarantee, that if PROPERTY is set, then drmModeGetConnector() (probing) must not be necessary based on this event alone. Writing it down again: HOTPLUG=1 CONNECTOR=xx PROPERTY=yy - yy denotes which connector xx property changed. - Userspace does not need to do drmModeGetConnector(), it only needs to drmModeObjectGetProperties() on the connector to receive the new updated property values. - Kernel must not send this event for changes that may require probing for correct results, exceptional conditions (buggy hardware, etc.) included. Instead, the kernel must send one of the below events. Is there actually anything interesting that drmModeGetConnectorCurrent() could guaranteed correctly return that is not a property already? I'd probably leave this consideration out completely, and just say do one of the needs-probing events if anything there changed. > > HOTPLUG=1 CONNECTOR=xx > > > > - Needs to drmModeGetConnector() on the one connector, no need to probe > > others. Implies that one needs to re-get the connector properties as > > well. > > Sounds good. > > > HOTPLUG=1 > > > > - Need to do drmModeGetResouces() to discover new/disappeared > > connectors, and need to drmModeGetConnector to re-probe every > > connector. (As always.) > > Maybe we should clarify that this is also what you get when an entire > connector appears/disappears (for dp mst hotplug). Yes, that's what I wrote. :-) Weston implements the discovery of
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen wrote: > > On Tue, 14 May 2019 13:02:09 +0200 > Daniel Vetter wrote: > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon wrote: > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > On Mon, 13 May 2019 11:34:58 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > > wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > > connector's > > > > > > > > > property. > > > > > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > > > change: > > > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and > > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > > hotplug > > > > > > > > issue and came up with similar ideas: > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have a > > > > > > > > more or > > > > > > > > less fine grain of uevent reporting depending on what happened. > > > > > > > > The > > > > > > > > point is that we need to cover different cases: > > > > > > > > - one or more properties changed; > > > > > > > > - the connector status changed; > > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > > > For the second one, something like: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > > disconnected; > > > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > > > There's no material difference between this one and the previous > > > > > > > one. > > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > > property, > > > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > > > > > That's the idea, but we need to handle status changes differently > > > > > > than > > > > > > properties, since as far as I know, connected/unconnected status is > > > > > > not > > > > > > exposed as a prop for the connector. > > > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > > into a property. Or another excuse why we should expose the epoch > > > > > property :-) > > > > > > > > Hi Daniel, > > > > > > > > just to clarify the first case, specific to one very particular > > > > property: > > > > > > > > With HDCP, there is a property that may change dynamically at runtime > > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > > when it changes, I do not want userspace have to poll that property > > > > with a timer. > > > > > > > > When that property alone changes, and userspace is prepared to handle > > > > that property changing alone, it must not trigger a reprobe of the > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe with the > > > > epoch approach or with any alternate uevent design? > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > properties is enough and reprobe of the connector is not necessary. > > > > This is complementary to indicating to userspace that only some > > > > connectors need to be reprobed instead of everything. > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > > reprobing. Would that work? > > Hi, > > yes, that would work, if it was acceptable to DRM upstream. The replies > to Paul seemed to be going south so fast that I thought we wouldn't get > any new uevent fields in favour of "epoch counters". > > > Yes that's the idea, depending upon which property you get you know > > it's a sink change (needs full reprobe) or something else like hdcp > > state machinery update. > > Right. > > > Wrt avoiding the full reprobe for sink changes: I think we should >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, May 14, 2019 at 4:13 PM Paul Kocialkowski wrote: > > Hi, > > On Tue, 2019-05-14 at 13:09 +0200, Daniel Vetter wrote: > > On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski > > wrote: > > > Hey, > > > > > > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > connector's > > > > > > > > property. > > > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > > change: > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > hotplug > > > > > > > issue and came up with similar ideas: > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have a > > > > > > > more or > > > > > > > less fine grain of uevent reporting depending on what happened. > > > > > > > The > > > > > > > point is that we need to cover different cases: > > > > > > > - one or more properties changed; > > > > > > > - the connector status changed; > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > PROPERTY= > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > For the second one, something like: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > disconnected; > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > There's no material difference between this one and the previous > > > > > > one. > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > property, > > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > > > That's the idea, but we need to handle status changes differently than > > > > > properties, since as far as I know, connected/unconnected status is > > > > > not > > > > > exposed as a prop for the connector. > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > into a property. Or another excuse why we should expose the epoch > > > > property :-) > > > > > > Well I think it would make sense anyway, as long as we can make sure it > > > stays consistent with the one reported in the connector struct. > > > > > > > > > Here's why: > > > > > > - A side effect of forcing a probe on a connector is that you get to > > > > > > read all the properties, so supplying them is kinda pointless. > > > > > > > > > > Agreed, except for the status case where it's useful to know it's a > > > > > disconnect, because we don't need any probe step in that case. > > > > > > > > > > > - You can read STATUS without forcing a reprobe, if you want to > > > > > > avoid > > > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > > > reasonable unfortunately, but oh well. > > > > > > > > > > How would that be retreived then? From the looks of it, that's a > > > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > > > does the full reprobe. > > > > > > > > drmGetConnector vs drmGetConnectorCurrent. > > > > > > Ah right, forgot about that one, thanks. > > > > > > > > Not sure what issues could arise in case of disconnect without reprobe > > > > > -- at least I don't see why userspace should have to do anything in > > > > > particular except no longer using the connector, even in complex DP > > > > > MST > > > > > cases. > > > > > > > > connector->status might be a lie without a full reprobe, and wrongly > > > > indicate that the connector is disconnected while there's still > > > > something plugged in. I'm not sure we've fixed those bugs by now > > > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > > > > connected, and we can't break this because every intel platform ever > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi, On Tue, 2019-05-14 at 13:09 +0200, Daniel Vetter wrote: > On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski > wrote: > > Hey, > > > > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > > property. > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > change: > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > > issue and came up with similar ideas: > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > The conclusions of these discussions so far would be to have a more > > > > > > or > > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > > point is that we need to cover different cases: > > > > > > - one or more properties changed; > > > > > > - the connector status changed; > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > For the first case, we can send out: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > PROPERTY= > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > For the second one, something like: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > disconnected; > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > There's no material difference between this one and the previous one. > > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > That's the idea, but we need to handle status changes differently than > > > > properties, since as far as I know, connected/unconnected status is not > > > > exposed as a prop for the connector. > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > into a property. Or another excuse why we should expose the epoch > > > property :-) > > > > Well I think it would make sense anyway, as long as we can make sure it > > stays consistent with the one reported in the connector struct. > > > > > > > Here's why: > > > > > - A side effect of forcing a probe on a connector is that you get to > > > > > read all the properties, so supplying them is kinda pointless. > > > > > > > > Agreed, except for the status case where it's useful to know it's a > > > > disconnect, because we don't need any probe step in that case. > > > > > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > > reasonable unfortunately, but oh well. > > > > > > > > How would that be retreived then? From the looks of it, that's a > > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > > does the full reprobe. > > > > > > drmGetConnector vs drmGetConnectorCurrent. > > > > Ah right, forgot about that one, thanks. > > > > > > Not sure what issues could arise in case of disconnect without reprobe > > > > -- at least I don't see why userspace should have to do anything in > > > > particular except no longer using the connector, even in complex DP MST > > > > cases. > > > > > > connector->status might be a lie without a full reprobe, and wrongly > > > indicate that the connector is disconnected while there's still > > > something plugged in. I'm not sure we've fixed those bugs by now > > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > > > connected, and we can't break this because every intel platform ever > > > shipped has a few devices where this is somehow broken, irrespective > > > of the sink). > > > > Mhh either way, I think it's up to the driver to report that and make > > it consistent. I think we have poll helpers to make up for cases where > > hotplug is not available too. So I'm not sure why a full reprobe would > > be needed: drivers just need to do the
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi, On Tue, 2019-05-14 at 16:36 +0300, Pekka Paalanen wrote: > On Tue, 14 May 2019 13:02:09 +0200 > Daniel Vetter wrote: > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon wrote: > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > > On Mon, 13 May 2019 11:34:58 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > > wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > > connector's > > > > > > > > > property. > > > > > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > > > change: > > > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and > > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > > hotplug > > > > > > > > issue and came up with similar ideas: > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have a > > > > > > > > more or > > > > > > > > less fine grain of uevent reporting depending on what happened. > > > > > > > > The > > > > > > > > point is that we need to cover different cases: > > > > > > > > - one or more properties changed; > > > > > > > > - the connector status changed; > > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > PROPERTY= > > > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > > > For the second one, something like: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > > disconnected; > > > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > > HOTPLUG=1 > > > > > > > > CONNECTOR= > > > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > > > There's no material difference between this one and the previous > > > > > > > one. > > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > > property, > > > > > > > i.e. we can reuse the same PROPERTY= > > > > > > > trick. > > > > > > > > > > > > That's the idea, but we need to handle status changes differently > > > > > > than > > > > > > properties, since as far as I know, connected/unconnected status is > > > > > > not > > > > > > exposed as a prop for the connector. > > > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > > into a property. Or another excuse why we should expose the epoch > > > > > property :-) > > > > > > > > Hi Daniel, > > > > > > > > just to clarify the first case, specific to one very particular > > > > property: > > > > > > > > With HDCP, there is a property that may change dynamically at runtime > > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > > when it changes, I do not want userspace have to poll that property > > > > with a timer. > > > > > > > > When that property alone changes, and userspace is prepared to handle > > > > that property changing alone, it must not trigger a reprobe of the > > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > > > How do you ensure that userspace can avoid triggering a reprobe with the > > > > epoch approach or with any alternate uevent design? > > > > > > > > We need an event to userspace that indicates that re-reading the > > > > properties is enough and reprobe of the connector is not necessary. > > > > This is complementary to indicating to userspace that only some > > > > connectors need to be reprobed instead of everything. > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > > reprobing. Would that work? > > Hi, > > yes, that would work, if it was acceptable to DRM upstream. The replies > to Paul seemed to be going south so fast that I thought we wouldn't get > any new uevent fields in favour of "epoch counters". > > > Yes that's the idea, depending upon which property you get you know > > it's a sink change (needs full reprobe) or something else like hdcp > > state machinery update. > > Right. > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, 14 May 2019 13:02:09 +0200 Daniel Vetter wrote: > On Tue, May 14, 2019 at 10:18 AM Ser, Simon wrote: > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > > On Mon, 13 May 2019 11:34:58 +0200 > > > Daniel Vetter wrote: > > > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > > DRM API for generating uevent for a status changes of > > > > > > > > connector's > > > > > > > > property. > > > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > > change: > > > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > > > So we just had some discussions over on IRC and at about the > > > > > > > hotplug > > > > > > > issue and came up with similar ideas: > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > > > The conclusions of these discussions so far would be to have a > > > > > > > more or > > > > > > > less fine grain of uevent reporting depending on what happened. > > > > > > > The > > > > > > > point is that we need to cover different cases: > > > > > > > - one or more properties changed; > > > > > > > - the connector status changed; > > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > > > For the first case, we can send out: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > PROPERTY= > > > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > > > For the second one, something like: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > > disconnected; > > > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > > HOTPLUG=1 > > > > > > > CONNECTOR= > > > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > > > There's no material difference between this one and the previous > > > > > > one. > > > > > > Plus there's no beenfit in supplying the actual value of the > > > > > > property, > > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > > > That's the idea, but we need to handle status changes differently than > > > > > properties, since as far as I know, connected/unconnected status is > > > > > not > > > > > exposed as a prop for the connector. > > > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > > into a property. Or another excuse why we should expose the epoch > > > > property :-) > > > > > > Hi Daniel, > > > > > > just to clarify the first case, specific to one very particular > > > property: > > > > > > With HDCP, there is a property that may change dynamically at runtime > > > (the undesired/desired/enabled tristate). Userspace must be notified > > > when it changes, I do not want userspace have to poll that property > > > with a timer. > > > > > > When that property alone changes, and userspace is prepared to handle > > > that property changing alone, it must not trigger a reprobe of the > > > connector. There is no reason to reprobe at that point AFAIU. > > > > > > How do you ensure that userspace can avoid triggering a reprobe with the > > > epoch approach or with any alternate uevent design? > > > > > > We need an event to userspace that indicates that re-reading the > > > properties is enough and reprobe of the connector is not necessary. > > > This is complementary to indicating to userspace that only some > > > connectors need to be reprobed instead of everything. > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > > reprobing. Would that work? Hi, yes, that would work, if it was acceptable to DRM upstream. The replies to Paul seemed to be going south so fast that I thought we wouldn't get any new uevent fields in favour of "epoch counters". > Yes that's the idea, depending upon which property you get you know > it's a sink change (needs full reprobe) or something else like hdcp > state machinery update. Right. > Wrt avoiding the full reprobe for sink changes: I think we should > indeed decouple that from the per-connector event for sink changes. > That along is a good win already, since you know for which connector > you need to call drmGetConnector (which forces the reprobe). It would > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, May 13, 2019 at 7:14 PM Paul Kocialkowski wrote: > > Hey, > > Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > property. > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > change: > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > issue and came up with similar ideas: > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > point is that we need to cover different cases: > > > > > - one or more properties changed; > > > > > - the connector status changed; > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > For the first case, we can send out: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > PROPERTY= > > > > > > > > > > and no reprobe is required. > > > > > > > > > > For the second one, something like: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > STATUS=Connected/Disconnected > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > disconnected; > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > There's no material difference between this one and the previous one. > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > That's the idea, but we need to handle status changes differently than > > > properties, since as far as I know, connected/unconnected status is not > > > exposed as a prop for the connector. > > > > Oops, totally missed that. "Everything is a property" is kinda > > new-ish, at least compared to kms. Kinda tempted to just make status > > into a property. Or another excuse why we should expose the epoch > > property :-) > > Well I think it would make sense anyway, as long as we can make sure it > stays consistent with the one reported in the connector struct. > > > > > Here's why: > > > > - A side effect of forcing a probe on a connector is that you get to > > > > read all the properties, so supplying them is kinda pointless. > > > > > > Agreed, except for the status case where it's useful to know it's a > > > disconnect, because we don't need any probe step in that case. > > > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > reasonable unfortunately, but oh well. > > > > > > How would that be retreived then? From the looks of it, that's a > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > does the full reprobe. > > > > drmGetConnector vs drmGetConnectorCurrent. > > Ah right, forgot about that one, thanks. > > > > Not sure what issues could arise in case of disconnect without reprobe > > > -- at least I don't see why userspace should have to do anything in > > > particular except no longer using the connector, even in complex DP MST > > > cases. > > > > connector->status might be a lie without a full reprobe, and wrongly > > indicate that the connector is disconnected while there's still > > something plugged in. I'm not sure we've fixed those bugs by now > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > > connected, and we can't break this because every intel platform ever > > shipped has a few devices where this is somehow broken, irrespective > > of the sink). > > Mhh either way, I think it's up to the driver to report that and make > it consistent. I think we have poll helpers to make up for cases where > hotplug is not available too. So I'm not sure why a full reprobe would > be needed: drivers just need to do the right thing. > > > > > - There's no way to only reprobe status, you can only ever reprobe > > > > everything with the current ioctl and implementations. Having an > > > > option to reprobe only parts of it doesn't seem useful to me (we need > > > > to read the EDID anyway, and that's the expensive part of reprobing in > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, May 13, 2019 at 11:20 PM Lyude Paul wrote:
>
> Hi-just wanted to give some general thoughts here.
>
> First off I'm 100% behind the epoch idea, that was one of the ideas I had been
> thinking of proposing here in the first place but probably forgot at some
> point down the road.
>
> A couple of other things:
> * I think it would also probably be good to have events for when connectors
>are added or removed from the system (mainly for MST)
Current uevent + userspace looks at the connector list from
drmGetResources? That should be enough to figure this out I think ...
> * Have we considered having any sort of SYNC event, like what evdev uses for
>signaling the end of a frame of events for input devices?
If we go with just 1 event, then that's the natural sync marker
stating "everything we updated for this connector is now updated". For
more global events the global uevent could serve that role (I guess
this is useful for hotpluggin/unpluggin entire mst chains - we might
want to coalesce these indeed).
I also don't think we need to make this an uapi guarantee, just best
effort kernel implementation - userspace needs to always be able to
deal with an event right after the one it's just processing.
-Daniel
>
> On Fri, 2019-05-10 at 14:12 +0200, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:
> > > DRM API for generating uevent for a status changes of connector's
> > > property.
> > >
> > > This uevent will have following details related to the status change:
> > >
> > > HOTPLUG=1, CONNECTOR= and PROPERTY=
> > >
> > > Need ACK from this uevent from userspace consumer.
> >
> > So we just had some discussions over on IRC and at about the hotplug
> > issue and came up with similar ideas:
> > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html
> >
> > The conclusions of these discussions so far would be to have a more or
> > less fine grain of uevent reporting depending on what happened. The
> > point is that we need to cover different cases:
> > - one or more properties changed;
> > - the connector status changed;
> > - something else about the connector changed (e.g. EDID/modes)
> >
> > For the first case, we can send out:
> > HOTPLUG=1
> > CONNECTOR=
> > PROPERTY=
> >
> > and no reprobe is required.
> >
> > For the second one, something like:
> > HOTPLUG=1
> > CONNECTOR=
> > STATUS=Connected/Disconnected
> >
> > and a connector probe is needed for connected, but not for
> > disconnected;
> >
> > For the third one, we can only indicate the connector:
> > HOTPLUG=1
> > CONNECTOR=
> >
> > and a reprobe of the connector is always needed
> >
> > Then we still have the legacy case:
> > HOTPLUG=1
> >
> > where userspace is expected to reprobe all the connectors.
> >
> > I think this would deserve to be a separate series on its own. So I am
> > proposing to take this one off your plate and come up with another
> > seres implementing this proposal. What do you think?
> >
> > Cheers,
> >
> > Paul
> >
> > > v2:
> > > Minor fixes at KDoc comments [Daniel]
> > > v3:
> > > Check the property is really attached with connector [Daniel]
> > >
> > > Signed-off-by: Ramalingam C
> > > Reviewed-by: Daniel Vetter
> > > ---
> > > drivers/gpu/drm/drm_sysfs.c | 35 +++
> > > include/drm/drm_sysfs.h | 5 -
> > > 2 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > > index 18b1ac442997..63fa951a20db 100644
> > > --- a/drivers/gpu/drm/drm_sysfs.c
> > > +++ b/drivers/gpu/drm/drm_sysfs.c
> > > @@ -21,6 +21,7 @@
> > > #include
> > > #include
> > > #include "drm_internal.h"
> > > +#include "drm_crtc_internal.h"
> > >
> > > #define to_drm_minor(d) dev_get_drvdata(d)
> > > #define to_drm_connector(d) dev_get_drvdata(d)
> > > @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> > > * Send a uevent for the DRM device specified by @dev. Currently we only
> > > * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> > > * deal with other types of events.
> > > + *
> > > + * Any new uapi should be using the drm_sysfs_connector_status_event()
> > > + * for uevents on connector status change.
> > > */
> > > void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > {
> > > @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> > > }
> > > EXPORT_SYMBOL(drm_sysfs_hotplug_event);
> > >
> > > +/**
> > > + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> > > + * property status change
> > > + * @connector: connector on which property status changed
> > > + * @property: connector property whoes status changed.
> > > + *
> > > + * Send a uevent for the DRM device specified by @dev. Currently we
> > > + * set HOTPLUG=1 and connector id along with the attached property id
> > > + * related to the status change.
> > > + */
> > > +void
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, May 14, 2019 at 10:18 AM Ser, Simon wrote: > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > > On Mon, 13 May 2019 11:34:58 +0200 > > Daniel Vetter wrote: > > > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > > property. > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > change: > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > > issue and came up with similar ideas: > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > The conclusions of these discussions so far would be to have a more > > > > > > or > > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > > point is that we need to cover different cases: > > > > > > - one or more properties changed; > > > > > > - the connector status changed; > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > For the first case, we can send out: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > PROPERTY= > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > For the second one, something like: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > disconnected; > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > There's no material difference between this one and the previous one. > > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > That's the idea, but we need to handle status changes differently than > > > > properties, since as far as I know, connected/unconnected status is not > > > > exposed as a prop for the connector. > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > into a property. Or another excuse why we should expose the epoch > > > property :-) > > > > Hi Daniel, > > > > just to clarify the first case, specific to one very particular > > property: > > > > With HDCP, there is a property that may change dynamically at runtime > > (the undesired/desired/enabled tristate). Userspace must be notified > > when it changes, I do not want userspace have to poll that property > > with a timer. > > > > When that property alone changes, and userspace is prepared to handle > > that property changing alone, it must not trigger a reprobe of the > > connector. There is no reason to reprobe at that point AFAIU. > > > > How do you ensure that userspace can avoid triggering a reprobe with the > > epoch approach or with any alternate uevent design? > > > > We need an event to userspace that indicates that re-reading the > > properties is enough and reprobe of the connector is not necessary. > > This is complementary to indicating to userspace that only some > > connectors need to be reprobed instead of everything. > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the > reprobing. Would that work? Yes that's the idea, depending upon which property you get you know it's a sink change (needs full reprobe) or something else like hdcp state machinery update. Wrt avoiding the full reprobe for sink changes: I think we should indeed decouple that from the per-connector event for sink changes. That along is a good win already, since you know for which connector you need to call drmGetConnector (which forces the reprobe). It would be nice to only call drmGetConnectorCurrent (avoids the reprobe), but historically speaking every time we tried to rely on this we ended up regretting things. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: > On Mon, 13 May 2019 11:34:58 +0200 > Daniel Vetter wrote: > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > property. > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > change: > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > issue and came up with similar ideas: > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > point is that we need to cover different cases: > > > > > - one or more properties changed; > > > > > - the connector status changed; > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > For the first case, we can send out: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > PROPERTY= > > > > > > > > > > and no reprobe is required. > > > > > > > > > > For the second one, something like: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > STATUS=Connected/Disconnected > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > disconnected; > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > There's no material difference between this one and the previous one. > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > That's the idea, but we need to handle status changes differently than > > > properties, since as far as I know, connected/unconnected status is not > > > exposed as a prop for the connector. > > > > Oops, totally missed that. "Everything is a property" is kinda > > new-ish, at least compared to kms. Kinda tempted to just make status > > into a property. Or another excuse why we should expose the epoch > > property :-) > > Hi Daniel, > > just to clarify the first case, specific to one very particular > property: > > With HDCP, there is a property that may change dynamically at runtime > (the undesired/desired/enabled tristate). Userspace must be notified > when it changes, I do not want userspace have to poll that property > with a timer. > > When that property alone changes, and userspace is prepared to handle > that property changing alone, it must not trigger a reprobe of the > connector. There is no reason to reprobe at that point AFAIU. > > How do you ensure that userspace can avoid triggering a reprobe with the > epoch approach or with any alternate uevent design? > > We need an event to userspace that indicates that re-reading the > properties is enough and reprobe of the connector is not necessary. > This is complementary to indicating to userspace that only some > connectors need to be reprobed instead of everything. Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the reprobing. Would that work? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 13 May 2019 11:34:58 +0200 Daniel Vetter wrote: > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > wrote: > > > > Hi, > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > DRM API for generating uevent for a status changes of connector's > > > > > property. > > > > > > > > > > This uevent will have following details related to the status change: > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > issue and came up with similar ideas: > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > less fine grain of uevent reporting depending on what happened. The > > > > point is that we need to cover different cases: > > > > - one or more properties changed; > > > > - the connector status changed; > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > For the first case, we can send out: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > PROPERTY= > > > > > > > > and no reprobe is required. > > > > > > > > For the second one, something like: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > STATUS=Connected/Disconnected > > > > > > > > and a connector probe is needed for connected, but not for > > > > disconnected; > > > > > > > > For the third one, we can only indicate the connector: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > > > > > and a reprobe of the connector is always needed > > > > > > There's no material difference between this one and the previous one. > > > Plus there's no beenfit in supplying the actual value of the property, > > > i.e. we can reuse the same PROPERTY= trick. > > > > That's the idea, but we need to handle status changes differently than > > properties, since as far as I know, connected/unconnected status is not > > exposed as a prop for the connector. > > Oops, totally missed that. "Everything is a property" is kinda > new-ish, at least compared to kms. Kinda tempted to just make status > into a property. Or another excuse why we should expose the epoch > property :-) Hi Daniel, just to clarify the first case, specific to one very particular property: With HDCP, there is a property that may change dynamically at runtime (the undesired/desired/enabled tristate). Userspace must be notified when it changes, I do not want userspace have to poll that property with a timer. When that property alone changes, and userspace is prepared to handle that property changing alone, it must not trigger a reprobe of the connector. There is no reason to reprobe at that point AFAIU. How do you ensure that userspace can avoid triggering a reprobe with the epoch approach or with any alternate uevent design? We need an event to userspace that indicates that re-reading the properties is enough and reprobe of the connector is not necessary. This is complementary to indicating to userspace that only some connectors need to be reprobed instead of everything. Thanks, pq pgp4l3Gi3LThT.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 2019-05-13 at 17:04 +0200, Daniel Vetter wrote: > On Mon, May 13, 2019 at 10:11:01AM +, Ser, Simon wrote: > > On Mon, 2019-05-13 at 11:34 +0200, Daniel Vetter wrote: > > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > > wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > > property. > > > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > > change: > > > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > > issue and came up with similar ideas: > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > > > The conclusions of these discussions so far would be to have a more > > > > > > or > > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > > point is that we need to cover different cases: > > > > > > - one or more properties changed; > > > > > > - the connector status changed; > > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > > > For the first case, we can send out: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > PROPERTY= > > > > > > > > > > > > and no reprobe is required. > > > > > > > > > > > > For the second one, something like: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > STATUS=Connected/Disconnected > > > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > > disconnected; > > > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > > HOTPLUG=1 > > > > > > CONNECTOR= > > > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > > > There's no material difference between this one and the previous one. > > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > > > That's the idea, but we need to handle status changes differently than > > > > properties, since as far as I know, connected/unconnected status is not > > > > exposed as a prop for the connector. > > > > > > Oops, totally missed that. "Everything is a property" is kinda > > > new-ish, at least compared to kms. Kinda tempted to just make status > > > into a property. Or another excuse why we should expose the epoch > > > property :-) > > > > > > > > Here's why: > > > > > - A side effect of forcing a probe on a connector is that you get to > > > > > read all the properties, so supplying them is kinda pointless. > > > > > > > > Agreed, except for the status case where it's useful to know it's a > > > > disconnect, because we don't need any probe step in that case. > > > > > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > > reasonable unfortunately, but oh well. > > > > > > > > How would that be retreived then? From the looks of it, that's a > > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > > does the full reprobe. > > > > > > drmGetConnector vs drmGetConnectorCurrent. > > > > > > > Not sure what issues could arise in case of disconnect without reprobe > > > > -- at least I don't see why userspace should have to do anything in > > > > particular except no longer using the connector, even in complex DP MST > > > > cases. > > > > > > connector->status might be a lie without a full reprobe, and wrongly > > > indicate that the connector is disconnected while there's still > > > something plugged in. I'm not sure we've fixed those bugs by now > > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > > > connected, and we can't break this because every intel platform ever > > > shipped has a few devices where this is somehow broken, irrespective > > > of the sink). > > > > > > > > - There's no way to only reprobe status, you can only ever reprobe > > > > > everything with the current ioctl and implementations. Having an > > > > > option to reprobe only parts of it doesn't seem useful to me (we need > > > > > to read the EDID anyway, and that's the expensive part of reprobing in > > > > > almost all cases). > > > > > > > > Agreed. > > > > > > > > > In a way PROPERTY= simply tells userspace that it > > > > > needs to reprobe this connector. > > > >
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi-just wanted to give some general thoughts here.
First off I'm 100% behind the epoch idea, that was one of the ideas I had been
thinking of proposing here in the first place but probably forgot at some
point down the road.
A couple of other things:
* I think it would also probably be good to have events for when connectors
are added or removed from the system (mainly for MST)
* Have we considered having any sort of SYNC event, like what evdev uses for
signaling the end of a frame of events for input devices?
On Fri, 2019-05-10 at 14:12 +0200, Paul Kocialkowski wrote:
> Hi,
>
> On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:
> > DRM API for generating uevent for a status changes of connector's
> > property.
> >
> > This uevent will have following details related to the status change:
> >
> > HOTPLUG=1, CONNECTOR= and PROPERTY=
> >
> > Need ACK from this uevent from userspace consumer.
>
> So we just had some discussions over on IRC and at about the hotplug
> issue and came up with similar ideas:
> https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html
>
> The conclusions of these discussions so far would be to have a more or
> less fine grain of uevent reporting depending on what happened. The
> point is that we need to cover different cases:
> - one or more properties changed;
> - the connector status changed;
> - something else about the connector changed (e.g. EDID/modes)
>
> For the first case, we can send out:
> HOTPLUG=1
> CONNECTOR=
> PROPERTY=
>
> and no reprobe is required.
>
> For the second one, something like:
> HOTPLUG=1
> CONNECTOR=
> STATUS=Connected/Disconnected
>
> and a connector probe is needed for connected, but not for
> disconnected;
>
> For the third one, we can only indicate the connector:
> HOTPLUG=1
> CONNECTOR=
>
> and a reprobe of the connector is always needed
>
> Then we still have the legacy case:
> HOTPLUG=1
>
> where userspace is expected to reprobe all the connectors.
>
> I think this would deserve to be a separate series on its own. So I am
> proposing to take this one off your plate and come up with another
> seres implementing this proposal. What do you think?
>
> Cheers,
>
> Paul
>
> > v2:
> > Minor fixes at KDoc comments [Daniel]
> > v3:
> > Check the property is really attached with connector [Daniel]
> >
> > Signed-off-by: Ramalingam C
> > Reviewed-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/drm_sysfs.c | 35 +++
> > include/drm/drm_sysfs.h | 5 -
> > 2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 18b1ac442997..63fa951a20db 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> > #include "drm_internal.h"
> > +#include "drm_crtc_internal.h"
> >
> > #define to_drm_minor(d) dev_get_drvdata(d)
> > #define to_drm_connector(d) dev_get_drvdata(d)
> > @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> > * Send a uevent for the DRM device specified by @dev. Currently we only
> > * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> > * deal with other types of events.
> > + *
> > + * Any new uapi should be using the drm_sysfs_connector_status_event()
> > + * for uevents on connector status change.
> > */
> > void drm_sysfs_hotplug_event(struct drm_device *dev)
> > {
> > @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> > }
> > EXPORT_SYMBOL(drm_sysfs_hotplug_event);
> >
> > +/**
> > + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> > + * property status change
> > + * @connector: connector on which property status changed
> > + * @property: connector property whoes status changed.
> > + *
> > + * Send a uevent for the DRM device specified by @dev. Currently we
> > + * set HOTPLUG=1 and connector id along with the attached property id
> > + * related to the status change.
> > + */
> > +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> > + struct drm_property *property)
> > +{
> > + struct drm_device *dev = connector->dev;
> > + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> > + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> > +
> > + WARN_ON(!drm_mode_obj_find_prop_id(>base,
> > + property->base.id));
> > +
> > + snprintf(conn_id, ARRAY_SIZE(conn_id),
> > +"CONNECTOR=%u", connector->base.id);
> > + snprintf(prop_id, ARRAY_SIZE(prop_id),
> > +"PROPERTY=%u", property->base.id);
> > +
> > + DRM_DEBUG("generating connector status event\n");
> > +
> > + kobject_uevent_env(>primary->kdev->kobj, KOBJ_CHANGE, envp);
> > +}
> > +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> > +
> > static void drm_sysfs_release(struct device *dev)
>
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hey, Le lundi 13 mai 2019 à 11:34 +0200, Daniel Vetter a écrit : > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > wrote: > > Hi, > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > DRM API for generating uevent for a status changes of connector's > > > > > property. > > > > > > > > > > This uevent will have following details related to the status change: > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > issue and came up with similar ideas: > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > less fine grain of uevent reporting depending on what happened. The > > > > point is that we need to cover different cases: > > > > - one or more properties changed; > > > > - the connector status changed; > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > For the first case, we can send out: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > PROPERTY= > > > > > > > > and no reprobe is required. > > > > > > > > For the second one, something like: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > STATUS=Connected/Disconnected > > > > > > > > and a connector probe is needed for connected, but not for > > > > disconnected; > > > > > > > > For the third one, we can only indicate the connector: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > > > > > and a reprobe of the connector is always needed > > > > > > There's no material difference between this one and the previous one. > > > Plus there's no beenfit in supplying the actual value of the property, > > > i.e. we can reuse the same PROPERTY= trick. > > > > That's the idea, but we need to handle status changes differently than > > properties, since as far as I know, connected/unconnected status is not > > exposed as a prop for the connector. > > Oops, totally missed that. "Everything is a property" is kinda > new-ish, at least compared to kms. Kinda tempted to just make status > into a property. Or another excuse why we should expose the epoch > property :-) Well I think it would make sense anyway, as long as we can make sure it stays consistent with the one reported in the connector struct. > > > Here's why: > > > - A side effect of forcing a probe on a connector is that you get to > > > read all the properties, so supplying them is kinda pointless. > > > > Agreed, except for the status case where it's useful to know it's a > > disconnect, because we don't need any probe step in that case. > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > reasonable unfortunately, but oh well. > > > > How would that be retreived then? From the looks of it, that's a > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > does the full reprobe. > > drmGetConnector vs drmGetConnectorCurrent. Ah right, forgot about that one, thanks. > > Not sure what issues could arise in case of disconnect without reprobe > > -- at least I don't see why userspace should have to do anything in > > particular except no longer using the connector, even in complex DP MST > > cases. > > connector->status might be a lie without a full reprobe, and wrongly > indicate that the connector is disconnected while there's still > something plugged in. I'm not sure we've fixed those bugs by now > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > connected, and we can't break this because every intel platform ever > shipped has a few devices where this is somehow broken, irrespective > of the sink). Mhh either way, I think it's up to the driver to report that and make it consistent. I think we have poll helpers to make up for cases where hotplug is not available too. So I'm not sure why a full reprobe would be needed: drivers just need to do the right thing. > > > - There's no way to only reprobe status, you can only ever reprobe > > > everything with the current ioctl and implementations. Having an > > > option to reprobe only parts of it doesn't seem useful to me (we need > > > to read the EDID anyway, and that's the expensive part of reprobing in > > > almost all cases). > > > > Agreed. > > > > > In a way PROPERTY= simply tells userspace that it > > > needs to reprobe this connector. > > > > I thought we could access the props alone, which avoids doing a reprobe > > when the kernel knows that only a prop or a set
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, May 13, 2019 at 10:11:01AM +, Ser, Simon wrote: > On Mon, 2019-05-13 at 11:34 +0200, Daniel Vetter wrote: > > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > > DRM API for generating uevent for a status changes of connector's > > > > > > property. > > > > > > > > > > > > This uevent will have following details related to the status > > > > > > change: > > > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > > issue and came up with similar ideas: > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > > less fine grain of uevent reporting depending on what happened. The > > > > > point is that we need to cover different cases: > > > > > - one or more properties changed; > > > > > - the connector status changed; > > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > > > For the first case, we can send out: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > PROPERTY= > > > > > > > > > > and no reprobe is required. > > > > > > > > > > For the second one, something like: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > STATUS=Connected/Disconnected > > > > > > > > > > and a connector probe is needed for connected, but not for > > > > > disconnected; > > > > > > > > > > For the third one, we can only indicate the connector: > > > > > HOTPLUG=1 > > > > > CONNECTOR= > > > > > > > > > > and a reprobe of the connector is always needed > > > > > > > > There's no material difference between this one and the previous one. > > > > Plus there's no beenfit in supplying the actual value of the property, > > > > i.e. we can reuse the same PROPERTY= trick. > > > > > > That's the idea, but we need to handle status changes differently than > > > properties, since as far as I know, connected/unconnected status is not > > > exposed as a prop for the connector. > > > > Oops, totally missed that. "Everything is a property" is kinda > > new-ish, at least compared to kms. Kinda tempted to just make status > > into a property. Or another excuse why we should expose the epoch > > property :-) > > > > > > Here's why: > > > > - A side effect of forcing a probe on a connector is that you get to > > > > read all the properties, so supplying them is kinda pointless. > > > > > > Agreed, except for the status case where it's useful to know it's a > > > disconnect, because we don't need any probe step in that case. > > > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > > reasonable unfortunately, but oh well. > > > > > > How would that be retreived then? From the looks of it, that's a > > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > > does the full reprobe. > > > > drmGetConnector vs drmGetConnectorCurrent. > > > > > Not sure what issues could arise in case of disconnect without reprobe > > > -- at least I don't see why userspace should have to do anything in > > > particular except no longer using the connector, even in complex DP MST > > > cases. > > > > connector->status might be a lie without a full reprobe, and wrongly > > indicate that the connector is disconnected while there's still > > something plugged in. I'm not sure we've fixed those bugs by now > > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > > connected, and we can't break this because every intel platform ever > > shipped has a few devices where this is somehow broken, irrespective > > of the sink). > > > > > > - There's no way to only reprobe status, you can only ever reprobe > > > > everything with the current ioctl and implementations. Having an > > > > option to reprobe only parts of it doesn't seem useful to me (we need > > > > to read the EDID anyway, and that's the expensive part of reprobing in > > > > almost all cases). > > > > > > Agreed. > > > > > > > In a way PROPERTY= simply tells userspace that it > > > > needs to reprobe this connector. > > > > > > I thought we could access the props alone, which avoids doing a reprobe > > > when the kernel knows that only a prop or a set of props changed and do > > > not require a full reprobe. That's the first case I was mentionning. > > > > > > > At that point we need to figure out
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, 2019-05-13 at 11:34 +0200, Daniel Vetter wrote: > On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski > wrote: > > Hi, > > > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > > DRM API for generating uevent for a status changes of connector's > > > > > property. > > > > > > > > > > This uevent will have following details related to the status change: > > > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > > issue and came up with similar ideas: > > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > > > The conclusions of these discussions so far would be to have a more or > > > > less fine grain of uevent reporting depending on what happened. The > > > > point is that we need to cover different cases: > > > > - one or more properties changed; > > > > - the connector status changed; > > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > > > For the first case, we can send out: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > PROPERTY= > > > > > > > > and no reprobe is required. > > > > > > > > For the second one, something like: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > STATUS=Connected/Disconnected > > > > > > > > and a connector probe is needed for connected, but not for > > > > disconnected; > > > > > > > > For the third one, we can only indicate the connector: > > > > HOTPLUG=1 > > > > CONNECTOR= > > > > > > > > and a reprobe of the connector is always needed > > > > > > There's no material difference between this one and the previous one. > > > Plus there's no beenfit in supplying the actual value of the property, > > > i.e. we can reuse the same PROPERTY= trick. > > > > That's the idea, but we need to handle status changes differently than > > properties, since as far as I know, connected/unconnected status is not > > exposed as a prop for the connector. > > Oops, totally missed that. "Everything is a property" is kinda > new-ish, at least compared to kms. Kinda tempted to just make status > into a property. Or another excuse why we should expose the epoch > property :-) > > > > Here's why: > > > - A side effect of forcing a probe on a connector is that you get to > > > read all the properties, so supplying them is kinda pointless. > > > > Agreed, except for the status case where it's useful to know it's a > > disconnect, because we don't need any probe step in that case. > > > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > > the reprobe for disconnected. I'd kinda not recommend that though, > > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > > dp) reprobing a disconnected output is fast. HDMI is ... less > > > reasonable unfortunately, but oh well. > > > > How would that be retreived then? From the looks of it, that's a > > MODE_GETCONNECTOR ioctl and I was under the impression this is what > > does the full reprobe. > > drmGetConnector vs drmGetConnectorCurrent. > > > Not sure what issues could arise in case of disconnect without reprobe > > -- at least I don't see why userspace should have to do anything in > > particular except no longer using the connector, even in complex DP MST > > cases. > > connector->status might be a lie without a full reprobe, and wrongly > indicate that the connector is disconnected while there's still > something plugged in. I'm not sure we've fixed those bugs by now > (usually it's around "hpd indicates disconnected" vs. "i2c indicates > connected, and we can't break this because every intel platform ever > shipped has a few devices where this is somehow broken, irrespective > of the sink). > > > > - There's no way to only reprobe status, you can only ever reprobe > > > everything with the current ioctl and implementations. Having an > > > option to reprobe only parts of it doesn't seem useful to me (we need > > > to read the EDID anyway, and that's the expensive part of reprobing in > > > almost all cases). > > > > Agreed. > > > > > In a way PROPERTY= simply tells userspace that it > > > needs to reprobe this connector. > > > > I thought we could access the props alone, which avoids doing a reprobe > > when the kernel knows that only a prop or a set of props changed and do > > not require a full reprobe. That's the first case I was mentionning. > > > > > At that point we need to figure out whether this is a good uapi or > > > not, and that's where the epoch comes in. There's two reasons for an > > > epoch: > > > - We need it internally because I'm not goinig to wire a new return > > > value through hundreds of connector probe functions. It's much easier > > > to have an epoch counter
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Mon, May 13, 2019 at 11:02 AM Paul Kocialkowski wrote: > > Hi, > > On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > > DRM API for generating uevent for a status changes of connector's > > > > property. > > > > > > > > This uevent will have following details related to the status change: > > > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > > > Need ACK from this uevent from userspace consumer. > > > > > > So we just had some discussions over on IRC and at about the hotplug > > > issue and came up with similar ideas: > > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > > > The conclusions of these discussions so far would be to have a more or > > > less fine grain of uevent reporting depending on what happened. The > > > point is that we need to cover different cases: > > > - one or more properties changed; > > > - the connector status changed; > > > - something else about the connector changed (e.g. EDID/modes) > > > > > > For the first case, we can send out: > > > HOTPLUG=1 > > > CONNECTOR= > > > PROPERTY= > > > > > > and no reprobe is required. > > > > > > For the second one, something like: > > > HOTPLUG=1 > > > CONNECTOR= > > > STATUS=Connected/Disconnected > > > > > > and a connector probe is needed for connected, but not for > > > disconnected; > > > > > > For the third one, we can only indicate the connector: > > > HOTPLUG=1 > > > CONNECTOR= > > > > > > and a reprobe of the connector is always needed > > > > There's no material difference between this one and the previous one. > > Plus there's no beenfit in supplying the actual value of the property, > > i.e. we can reuse the same PROPERTY= trick. > > That's the idea, but we need to handle status changes differently than > properties, since as far as I know, connected/unconnected status is not > exposed as a prop for the connector. Oops, totally missed that. "Everything is a property" is kinda new-ish, at least compared to kms. Kinda tempted to just make status into a property. Or another excuse why we should expose the epoch property :-) > > Here's why: > > - A side effect of forcing a probe on a connector is that you get to > > read all the properties, so supplying them is kinda pointless. > > Agreed, except for the status case where it's useful to know it's a > disconnect, because we don't need any probe step in that case. > > > - You can read STATUS without forcing a reprobe, if you want to avoid > > the reprobe for disconnected. I'd kinda not recommend that though, > > feels a bit like overoptimizing. And for reasonable connectors (i.e. > > dp) reprobing a disconnected output is fast. HDMI is ... less > > reasonable unfortunately, but oh well. > > How would that be retreived then? From the looks of it, that's a > MODE_GETCONNECTOR ioctl and I was under the impression this is what > does the full reprobe. drmGetConnector vs drmGetConnectorCurrent. > Not sure what issues could arise in case of disconnect without reprobe > -- at least I don't see why userspace should have to do anything in > particular except no longer using the connector, even in complex DP MST > cases. connector->status might be a lie without a full reprobe, and wrongly indicate that the connector is disconnected while there's still something plugged in. I'm not sure we've fixed those bugs by now (usually it's around "hpd indicates disconnected" vs. "i2c indicates connected, and we can't break this because every intel platform ever shipped has a few devices where this is somehow broken, irrespective of the sink). > > - There's no way to only reprobe status, you can only ever reprobe > > everything with the current ioctl and implementations. Having an > > option to reprobe only parts of it doesn't seem useful to me (we need > > to read the EDID anyway, and that's the expensive part of reprobing in > > almost all cases). > > Agreed. > > > In a way PROPERTY= simply tells userspace that it > > needs to reprobe this connector. > > I thought we could access the props alone, which avoids doing a reprobe > when the kernel knows that only a prop or a set of props changed and do > not require a full reprobe. That's the first case I was mentionning. > > > At that point we need to figure out whether this is a good uapi or > > not, and that's where the epoch comes in. There's two reasons for an > > epoch: > > - We need it internally because I'm not goinig to wire a new return > > value through hundreds of connector probe functions. It's much easier > > to have an epoch counter which we set from e.g. drm_set_edid and > > similar functions that update probe state. > > I don't think I'm following what issue this is trying to solve > internally. So I'm assuming that if we handle a hotplug, we only want to generate one uevent for that, not one for every little thing that changed. There's
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi, On Fri, 2019-05-10 at 16:54 +0200, Daniel Vetter wrote: > On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski > wrote: > > Hi, > > > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > > DRM API for generating uevent for a status changes of connector's > > > property. > > > > > > This uevent will have following details related to the status change: > > > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > > > Need ACK from this uevent from userspace consumer. > > > > So we just had some discussions over on IRC and at about the hotplug > > issue and came up with similar ideas: > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > > > The conclusions of these discussions so far would be to have a more or > > less fine grain of uevent reporting depending on what happened. The > > point is that we need to cover different cases: > > - one or more properties changed; > > - the connector status changed; > > - something else about the connector changed (e.g. EDID/modes) > > > > For the first case, we can send out: > > HOTPLUG=1 > > CONNECTOR= > > PROPERTY= > > > > and no reprobe is required. > > > > For the second one, something like: > > HOTPLUG=1 > > CONNECTOR= > > STATUS=Connected/Disconnected > > > > and a connector probe is needed for connected, but not for > > disconnected; > > > > For the third one, we can only indicate the connector: > > HOTPLUG=1 > > CONNECTOR= > > > > and a reprobe of the connector is always needed > > There's no material difference between this one and the previous one. > Plus there's no beenfit in supplying the actual value of the property, > i.e. we can reuse the same PROPERTY= trick. That's the idea, but we need to handle status changes differently than properties, since as far as I know, connected/unconnected status is not exposed as a prop for the connector. > Here's why: > - A side effect of forcing a probe on a connector is that you get to > read all the properties, so supplying them is kinda pointless. Agreed, except for the status case where it's useful to know it's a disconnect, because we don't need any probe step in that case. > - You can read STATUS without forcing a reprobe, if you want to avoid > the reprobe for disconnected. I'd kinda not recommend that though, > feels a bit like overoptimizing. And for reasonable connectors (i.e. > dp) reprobing a disconnected output is fast. HDMI is ... less > reasonable unfortunately, but oh well. How would that be retreived then? From the looks of it, that's a MODE_GETCONNECTOR ioctl and I was under the impression this is what does the full reprobe. Not sure what issues could arise in case of disconnect without reprobe -- at least I don't see why userspace should have to do anything in particular except no longer using the connector, even in complex DP MST cases. > - There's no way to only reprobe status, you can only ever reprobe > everything with the current ioctl and implementations. Having an > option to reprobe only parts of it doesn't seem useful to me (we need > to read the EDID anyway, and that's the expensive part of reprobing in > almost all cases). Agreed. > In a way PROPERTY= simply tells userspace that it > needs to reprobe this connector. I thought we could access the props alone, which avoids doing a reprobe when the kernel knows that only a prop or a set of props changed and do not require a full reprobe. That's the first case I was mentionning. > At that point we need to figure out whether this is a good uapi or > not, and that's where the epoch comes in. There's two reasons for an > epoch: > - We need it internally because I'm not goinig to wire a new return > value through hundreds of connector probe functions. It's much easier > to have an epoch counter which we set from e.g. drm_set_edid and > similar functions that update probe state. I don't think I'm following what issue this is trying to solve internally. > - If userspace misses an event and there's no epoch, we're forcing > userspace to reprobe everything. Use case would be if a compositor is > switched away we probably don't want to piss of the current compositor > by blocking it's own probe kernel calls by doing our own (probe is > single-threaded in the kernel through the dev->mode_config.mutex). If > it can read the epoch property (which it can do without forcing a > reprobe) userspace would know which connectors it needs to check and > reprobe. > > Hence why epoch, it's a bit more robust userspace api. Ofc you could > also require that userspace needs to keep parsing all uevents and make > a list of all connectors it needs to reprobe when it's back to being > the active compositor. But just comparing a current epoch with the one > you cached from the last full probe is much easier. Fair enough, I think it's a fine idea for robustness yes, but I think we could also provide extra info in the uevent when relevant and not rely on that entirely. > Another thing: None of this we can for
Re: [PATCH v7 09/11] drm: uevent for connector status change
On Fri, May 10, 2019 at 2:12 PM Paul Kocialkowski wrote: > > Hi, > > On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote: > > DRM API for generating uevent for a status changes of connector's > > property. > > > > This uevent will have following details related to the status change: > > > > HOTPLUG=1, CONNECTOR= and PROPERTY= > > > > Need ACK from this uevent from userspace consumer. > > So we just had some discussions over on IRC and at about the hotplug > issue and came up with similar ideas: > https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html > > The conclusions of these discussions so far would be to have a more or > less fine grain of uevent reporting depending on what happened. The > point is that we need to cover different cases: > - one or more properties changed; > - the connector status changed; > - something else about the connector changed (e.g. EDID/modes) > > For the first case, we can send out: > HOTPLUG=1 > CONNECTOR= > PROPERTY= > > and no reprobe is required. > > For the second one, something like: > HOTPLUG=1 > CONNECTOR= > STATUS=Connected/Disconnected > > and a connector probe is needed for connected, but not for > disconnected; > > For the third one, we can only indicate the connector: > HOTPLUG=1 > CONNECTOR= > > and a reprobe of the connector is always needed There's no material difference between this one and the previous one. Plus there's no beenfit in supplying the actual value of the property, i.e. we can reuse the same PROPERTY= trick. Here's why: - A side effect of forcing a probe on a connector is that you get to read all the properties, so supplying them is kinda pointless. - You can read STATUS without forcing a reprobe, if you want to avoid the reprobe for disconnected. I'd kinda not recommend that though, feels a bit like overoptimizing. And for reasonable connectors (i.e. dp) reprobing a disconnected output is fast. HDMI is ... less reasonable unfortunately, but oh well. - There's no way to only reprobe status, you can only ever reprobe everything with the current ioctl and implementations. Having an option to reprobe only parts of it doesn't seem useful to me (we need to read the EDID anyway, and that's the expensive part of reprobing in almost all cases). In a way PROPERTY= simply tells userspace that it needs to reprobe this connector. At that point we need to figure out whether this is a good uapi or not, and that's where the epoch comes in. There's two reasons for an epoch: - We need it internally because I'm not goinig to wire a new return value through hundreds of connector probe functions. It's much easier to have an epoch counter which we set from e.g. drm_set_edid and similar functions that update probe state. - If userspace misses an event and there's no epoch, we're forcing userspace to reprobe everything. Use case would be if a compositor is switched away we probably don't want to piss of the current compositor by blocking it's own probe kernel calls by doing our own (probe is single-threaded in the kernel through the dev->mode_config.mutex). If it can read the epoch property (which it can do without forcing a reprobe) userspace would know which connectors it needs to check and reprobe. Hence why epoch, it's a bit more robust userspace api. Ofc you could also require that userspace needs to keep parsing all uevents and make a list of all connectors it needs to reprobe when it's back to being the active compositor. But just comparing a current epoch with the one you cached from the last full probe is much easier. Another thing: None of this we can for connectors with unreliable hdp. Or at least you'll piss of users if you cache always. The sad thing is that HDMI is unreliable, at least on some machines/screen combos (you never get a hpd irq if you plug in/unplug). So real compositors still need to reprobe when the user asks for it. igt can probably get away without reprobing. -Daniel > Then we still have the legacy case: > HOTPLUG=1 > > where userspace is expected to reprobe all the connectors. > > I think this would deserve to be a separate series on its own. So I am > proposing to take this one off your plate and come up with another > seres implementing this proposal. What do you think? > > Cheers, > > Paul > > > v2: > > Minor fixes at KDoc comments [Daniel] > > v3: > > Check the property is really attached with connector [Daniel] > > > > Signed-off-by: Ramalingam C > > Reviewed-by: Daniel Vetter > > --- > > drivers/gpu/drm/drm_sysfs.c | 35 +++ > > include/drm/drm_sysfs.h | 5 - > > 2 files changed, 39 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > index 18b1ac442997..63fa951a20db 100644 > > --- a/drivers/gpu/drm/drm_sysfs.c > > +++ b/drivers/gpu/drm/drm_sysfs.c > > @@ -21,6 +21,7 @@ > > #include > > #include > > #include "drm_internal.h" > > +#include "drm_crtc_internal.h" > > > > #define to_drm_minor(d)
Re: [PATCH v7 09/11] drm: uevent for connector status change
Hi,
On Tue, 2019-05-07 at 21:57 +0530, Ramalingam C wrote:
> DRM API for generating uevent for a status changes of connector's
> property.
>
> This uevent will have following details related to the status change:
>
> HOTPLUG=1, CONNECTOR= and PROPERTY=
>
> Need ACK from this uevent from userspace consumer.
So we just had some discussions over on IRC and at about the hotplug
issue and came up with similar ideas:
https://lists.freedesktop.org/archives/dri-devel/2019-May/217408.html
The conclusions of these discussions so far would be to have a more or
less fine grain of uevent reporting depending on what happened. The
point is that we need to cover different cases:
- one or more properties changed;
- the connector status changed;
- something else about the connector changed (e.g. EDID/modes)
For the first case, we can send out:
HOTPLUG=1
CONNECTOR=
PROPERTY=
and no reprobe is required.
For the second one, something like:
HOTPLUG=1
CONNECTOR=
STATUS=Connected/Disconnected
and a connector probe is needed for connected, but not for
disconnected;
For the third one, we can only indicate the connector:
HOTPLUG=1
CONNECTOR=
and a reprobe of the connector is always needed
Then we still have the legacy case:
HOTPLUG=1
where userspace is expected to reprobe all the connectors.
I think this would deserve to be a separate series on its own. So I am
proposing to take this one off your plate and come up with another
seres implementing this proposal. What do you think?
Cheers,
Paul
> v2:
> Minor fixes at KDoc comments [Daniel]
> v3:
> Check the property is really attached with connector [Daniel]
>
> Signed-off-by: Ramalingam C
> Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_sysfs.c | 35 +++
> include/drm/drm_sysfs.h | 5 -
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 18b1ac442997..63fa951a20db 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include "drm_internal.h"
> +#include "drm_crtc_internal.h"
>
> #define to_drm_minor(d) dev_get_drvdata(d)
> #define to_drm_connector(d) dev_get_drvdata(d)
> @@ -320,6 +321,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)
> * Send a uevent for the DRM device specified by @dev. Currently we only
> * set HOTPLUG=1 in the uevent environment, but this could be expanded to
> * deal with other types of events.
> + *
> + * Any new uapi should be using the drm_sysfs_connector_status_event()
> + * for uevents on connector status change.
> */
> void drm_sysfs_hotplug_event(struct drm_device *dev)
> {
> @@ -332,6 +336,37 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_sysfs_hotplug_event);
>
> +/**
> + * drm_sysfs_connector_status_event - generate a DRM uevent for connector
> + * property status change
> + * @connector: connector on which property status changed
> + * @property: connector property whoes status changed.
> + *
> + * Send a uevent for the DRM device specified by @dev. Currently we
> + * set HOTPLUG=1 and connector id along with the attached property id
> + * related to the status change.
> + */
> +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> + struct drm_property *property)
> +{
> + struct drm_device *dev = connector->dev;
> + char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
> + char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
> +
> + WARN_ON(!drm_mode_obj_find_prop_id(>base,
> +property->base.id));
> +
> + snprintf(conn_id, ARRAY_SIZE(conn_id),
> + "CONNECTOR=%u", connector->base.id);
> + snprintf(prop_id, ARRAY_SIZE(prop_id),
> + "PROPERTY=%u", property->base.id);
> +
> + DRM_DEBUG("generating connector status event\n");
> +
> + kobject_uevent_env(>primary->kdev->kobj, KOBJ_CHANGE, envp);
> +}
> +EXPORT_SYMBOL(drm_sysfs_connector_status_event);
> +
> static void drm_sysfs_release(struct device *dev)
> {
> kfree(dev);
> diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h
> index 4f311e836cdc..d454ef617b2c 100644
> --- a/include/drm/drm_sysfs.h
> +++ b/include/drm/drm_sysfs.h
> @@ -4,10 +4,13 @@
>
> struct drm_device;
> struct device;
> +struct drm_connector;
> +struct drm_property;
>
> int drm_class_device_register(struct device *dev);
> void drm_class_device_unregister(struct device *dev);
>
> void drm_sysfs_hotplug_event(struct drm_device *dev);
> -
> +void drm_sysfs_connector_status_event(struct drm_connector *connector,
> + struct drm_property *property);
> #endif
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
___
dri-devel mailing list
