| Message ID | 20190506144629.5976-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/doc: Improve docs for conn_state->best_encoder | expand |
Hi Daniel, Thank you for the patch. On Mon, May 06, 2019 at 04:46:29PM +0200, Daniel Vetter wrote: > It's mandatory and considered core state since ioctls rely on this > working. > > Thanks to Laurent for pointing out this gap. > > v2: Clarify to "atomic drivers" only. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Sean Paul <sean@poorly.run> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > include/drm/drm_connector.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 02a131202add..f43f40d5888a 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -517,6 +517,10 @@ struct drm_connector_state { > * Used by the atomic helpers to select the encoder, through the > * &drm_connector_helper_funcs.atomic_best_encoder or > * &drm_connector_helper_funcs.best_encoder callbacks. How about updating this part as well ? "Used by both the DRM core and the atomic helpers to select the encoder (through the &drm_connector_helper_funcs.atomic_best_encoder), access it and report it to userspace (through the GETCONNECTOR and GETENCODER ioctls). This field shall be set by all atomic drivers, either directly or through atomic helpers." > + * > + * NOTE: Atomic drivers must fill this out (either themselves or through > + * helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will > + * not return correct data to userspace. > */ > struct drm_encoder *best_encoder; >
On Mon, May 06, 2019 at 05:57:53PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, May 06, 2019 at 04:46:29PM +0200, Daniel Vetter wrote: > > It's mandatory and considered core state since ioctls rely on this > > working. > > > > Thanks to Laurent for pointing out this gap. > > > > v2: Clarify to "atomic drivers" only. > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Sean Paul <sean@poorly.run> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > include/drm/drm_connector.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index 02a131202add..f43f40d5888a 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -517,6 +517,10 @@ struct drm_connector_state { > > * Used by the atomic helpers to select the encoder, through the > > * &drm_connector_helper_funcs.atomic_best_encoder or > > * &drm_connector_helper_funcs.best_encoder callbacks. > > How about updating this part as well ? > > "Used by both the DRM core and the atomic helpers to select the encoder > (through the &drm_connector_helper_funcs.atomic_best_encoder), access it > and report it to userspace (through the GETCONNECTOR and GETENCODER > ioctls). This field shall be set by all atomic drivers, either directly > or through atomic helpers." It's kinda two things, I think best to describe in two paragraphs. But I can move the core usage up, since arguable more important. Otoh most drivers won't care since helpers handle this, and they care more about how @best_encoder is used. E.g. core never calls the helper callbacks @atomic_best_endoer/best_encoder, which isn't clear anymore with your wording. And I have a sticker for core/helper splits :-) -Daniel > > > + * > > + * NOTE: Atomic drivers must fill this out (either themselves or through > > + * helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will > > + * not return correct data to userspace. > > */ > > struct drm_encoder *best_encoder; > > > > -- > Regards, > > Laurent Pinchart
On Mon, May 06, 2019 at 05:27:24PM +0200, Daniel Vetter wrote: > On Mon, May 06, 2019 at 05:57:53PM +0300, Laurent Pinchart wrote: > > Hi Daniel, > > > > Thank you for the patch. > > > > On Mon, May 06, 2019 at 04:46:29PM +0200, Daniel Vetter wrote: > > > It's mandatory and considered core state since ioctls rely on this > > > working. > > > > > > Thanks to Laurent for pointing out this gap. > > > > > > v2: Clarify to "atomic drivers" only. > > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > include/drm/drm_connector.h | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > index 02a131202add..f43f40d5888a 100644 > > > --- a/include/drm/drm_connector.h > > > +++ b/include/drm/drm_connector.h > > > @@ -517,6 +517,10 @@ struct drm_connector_state { > > > * Used by the atomic helpers to select the encoder, through the > > > * &drm_connector_helper_funcs.atomic_best_encoder or > > > * &drm_connector_helper_funcs.best_encoder callbacks. > > > > How about updating this part as well ? > > > > "Used by both the DRM core and the atomic helpers to select the encoder > > (through the &drm_connector_helper_funcs.atomic_best_encoder), access it > > and report it to userspace (through the GETCONNECTOR and GETENCODER > > ioctls). This field shall be set by all atomic drivers, either directly > > or through atomic helpers." > > It's kinda two things, I think best to describe in two paragraphs. But I > can move the core usage up, since arguable more important. Otoh most > drivers won't care since helpers handle this, and they care more about how > @best_encoder is used. > > E.g. core never calls the helper callbacks > @atomic_best_endoer/best_encoder, which isn't clear anymore with your > wording. And I have a sticker for core/helper splits :-) Went ahead with Sean's irc ack and merged this. We can bikeshed more later on I guess with more patches. I think sprinkling a pile of cross references once Sean's and yours patches all land would be good. -Daniel > -Daniel > > > > > + * > > > + * NOTE: Atomic drivers must fill this out (either themselves or through > > > + * helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will > > > + * not return correct data to userspace. > > > */ > > > struct drm_encoder *best_encoder; > > > > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 02a131202add..f43f40d5888a 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -517,6 +517,10 @@ struct drm_connector_state { * Used by the atomic helpers to select the encoder, through the * &drm_connector_helper_funcs.atomic_best_encoder or * &drm_connector_helper_funcs.best_encoder callbacks. + * + * NOTE: Atomic drivers must fill this out (either themselves or through + * helpers), for otherwise the GETCONNECTOR and GETENCODER IOCTLs will + * not return correct data to userspace. */ struct drm_encoder *best_encoder;
It's mandatory and considered core state since ioctls rely on this working. Thanks to Laurent for pointing out this gap. v2: Clarify to "atomic drivers" only. Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Sean Paul <sean@poorly.run> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- include/drm/drm_connector.h | 4 ++++ 1 file changed, 4 insertions(+)