| Message ID | 20190805151143.12317-1-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/crc-debugfs: Add notes about CRC<->commit interactions | expand |
On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote: > CRC generation can be impacted by commits coming from userspace, and > enabling CRC generation may itself trigger a commit. Add notes about > this to the kerneldoc. > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > > I might have got the wrong end of the stick, but this is what I > understood from what you said. > > Cheers, > -Brian > > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++---- > include/drm/drm_crtc.h | 3 +++ > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c > index 7ca486d750e9..1dff956bcc74 100644 > --- a/drivers/gpu/drm/drm_debugfs_crc.c > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > @@ -65,10 +65,17 @@ > * it submits. In this general case, the maximum userspace can do is to compare > * the reported CRCs of frames that should have the same contents. > * > - * On the driver side the implementation effort is minimal, drivers only need to > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically > - * set up if that vfunc is set. CRC samples need to be captured in the driver by > - * calling drm_crtc_add_crc_entry(). > + * On the driver side the implementation effort is minimal, drivers only need > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are > + * automatically set up if that vfunc is set. CRC samples need to be captured > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit > + * (even a full modeset). > + * > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the > + * legacy paths may interfere with CRC generation. So, in the general case, > + * userspace can't rely on the values in crtc-N/crc/data being valid > + * across a commit. It's not just the values, but the generation itself might get disabled (e.g. on i915 if you select "auto" on some chips you get the DP port sampling point, but for HDMI mode you get a different sampling ploint, and if you get the wrong one there won't be any crc for you). Also it's not just any atomic commit, only the ones with ALLOW_MODESET. Maybe something like the below text: "Please note that an atomic modeset commit with the DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl (which amounts to the same), can destry the CRC setup due to hardware requirements. This results in inconsistent CRC values or not even any CRC values generated. Generic userspace therefore needs to re-setup the CRC after each such modeset." > > static int crc_control_show(struct seq_file *m, void *data) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 128d8b210621..0f7ea094a900 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -756,6 +756,8 @@ struct drm_crtc_funcs { > * provided from the configured source. Drivers must accept an "auto" > * source name that will select a default source for this CRTC. > * > + * This may trigger a commit if necessary, to enable CRC generation. I'd clarify this as "atomic modeset commit" just to be sure. With these two comments addressed somehow: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + * > * Note that "auto" can depend upon the current modeset configuration, > * e.g. it could pick an encoder or output specific CRC sampling point. > * > @@ -767,6 +769,7 @@ struct drm_crtc_funcs { > * 0 on success or a negative error code on failure. > */ > int (*set_crc_source)(struct drm_crtc *crtc, const char *source); > + > /** > * @verify_crc_source: > * > -- > 2.17.1 >
On Mon, Aug 05, 2019 at 06:24:17PM +0200, Daniel Vetter wrote: > On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote: > > CRC generation can be impacted by commits coming from userspace, and > > enabling CRC generation may itself trigger a commit. Add notes about > > this to the kerneldoc. > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > --- > > > > I might have got the wrong end of the stick, but this is what I > > understood from what you said. > > > > Cheers, > > -Brian > > > > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++---- > > include/drm/drm_crtc.h | 3 +++ > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c > > index 7ca486d750e9..1dff956bcc74 100644 > > --- a/drivers/gpu/drm/drm_debugfs_crc.c > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > > @@ -65,10 +65,17 @@ > > * it submits. In this general case, the maximum userspace can do is to compare > > * the reported CRCs of frames that should have the same contents. > > * > > - * On the driver side the implementation effort is minimal, drivers only need to > > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically > > - * set up if that vfunc is set. CRC samples need to be captured in the driver by > > - * calling drm_crtc_add_crc_entry(). > > + * On the driver side the implementation effort is minimal, drivers only need > > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are > > + * automatically set up if that vfunc is set. CRC samples need to be captured > > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver > > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit > > + * (even a full modeset). > > + * > > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the > > + * legacy paths may interfere with CRC generation. So, in the general case, > > + * userspace can't rely on the values in crtc-N/crc/data being valid > > + * across a commit. > > It's not just the values, but the generation itself might get disabled > (e.g. on i915 if you select "auto" on some chips you get the DP port > sampling point, but for HDMI mode you get a different sampling ploint, and > if you get the wrong one there won't be any crc for you). Also it's not > just any atomic commit, only the ones with ALLOW_MODESET. Is the meaning of ALLOW_MODESET actually defined somewhere then? I thought it was broadly speaking "anything that would cause a flicker on the output" - but that needn't be the same set of things that break CRC generation. > > Maybe something like the below text: > > "Please note that an atomic modeset commit with the > DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl > (which amounts to the same), can destry the CRC setup due to hardware > requirements. This results in inconsistent CRC values or not even any CRC > values generated. Generic userspace therefore needs to re-setup the CRC > after each such modeset." > > > > > static int crc_control_show(struct seq_file *m, void *data) > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 128d8b210621..0f7ea094a900 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -756,6 +756,8 @@ struct drm_crtc_funcs { > > * provided from the configured source. Drivers must accept an "auto" > > * source name that will select a default source for this CRTC. > > * > > + * This may trigger a commit if necessary, to enable CRC generation. > > I'd clarify this as "atomic modeset commit" just to be sure. Ack. Thanks, -Brian > > With these two comments addressed somehow: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > + * > > * Note that "auto" can depend upon the current modeset configuration, > > * e.g. it could pick an encoder or output specific CRC sampling point. > > * > > @@ -767,6 +769,7 @@ struct drm_crtc_funcs { > > * 0 on success or a negative error code on failure. > > */ > > int (*set_crc_source)(struct drm_crtc *crtc, const char *source); > > + > > /** > > * @verify_crc_source: > > * > > -- > > 2.17.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, Aug 05, 2019 at 04:54:15PM +0000, Brian Starkey wrote: > On Mon, Aug 05, 2019 at 06:24:17PM +0200, Daniel Vetter wrote: > > On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote: > > > CRC generation can be impacted by commits coming from userspace, and > > > enabling CRC generation may itself trigger a commit. Add notes about > > > this to the kerneldoc. > > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > --- > > > > > > I might have got the wrong end of the stick, but this is what I > > > understood from what you said. > > > > > > Cheers, > > > -Brian > > > > > > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++---- > > > include/drm/drm_crtc.h | 3 +++ > > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c > > > index 7ca486d750e9..1dff956bcc74 100644 > > > --- a/drivers/gpu/drm/drm_debugfs_crc.c > > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > > > @@ -65,10 +65,17 @@ > > > * it submits. In this general case, the maximum userspace can do is to compare > > > * the reported CRCs of frames that should have the same contents. > > > * > > > - * On the driver side the implementation effort is minimal, drivers only need to > > > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically > > > - * set up if that vfunc is set. CRC samples need to be captured in the driver by > > > - * calling drm_crtc_add_crc_entry(). > > > + * On the driver side the implementation effort is minimal, drivers only need > > > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are > > > + * automatically set up if that vfunc is set. CRC samples need to be captured > > > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver > > > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit > > > + * (even a full modeset). > > > + * > > > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the > > > + * legacy paths may interfere with CRC generation. So, in the general case, > > > + * userspace can't rely on the values in crtc-N/crc/data being valid > > > + * across a commit. > > > > It's not just the values, but the generation itself might get disabled > > (e.g. on i915 if you select "auto" on some chips you get the DP port > > sampling point, but for HDMI mode you get a different sampling ploint, and > > if you get the wrong one there won't be any crc for you). Also it's not > > just any atomic commit, only the ones with ALLOW_MODESET. > > Is the meaning of ALLOW_MODESET actually defined somewhere then? I > thought it was broadly speaking "anything that would cause a flicker > on the output" - but that needn't be the same set of things that break > CRC generation. It's the inverse, I think we should require that crc keeps working for non-ALLOW_MODESET. Otherwise crc are essentially useless for validating stuff. And yeah allow_modeset is "could flicker and/or take enormous amounts of time". -Daniel > > Maybe something like the below text: > > > > "Please note that an atomic modeset commit with the > > DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl > > (which amounts to the same), can destry the CRC setup due to hardware > > requirements. This results in inconsistent CRC values or not even any CRC > > values generated. Generic userspace therefore needs to re-setup the CRC > > after each such modeset." > > > > > > > > static int crc_control_show(struct seq_file *m, void *data) > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > > index 128d8b210621..0f7ea094a900 100644 > > > --- a/include/drm/drm_crtc.h > > > +++ b/include/drm/drm_crtc.h > > > @@ -756,6 +756,8 @@ struct drm_crtc_funcs { > > > * provided from the configured source. Drivers must accept an "auto" > > > * source name that will select a default source for this CRTC. > > > * > > > + * This may trigger a commit if necessary, to enable CRC generation. > > > > I'd clarify this as "atomic modeset commit" just to be sure. > > Ack. > > Thanks, > -Brian > > > > > With these two comments addressed somehow: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > + * > > > * Note that "auto" can depend upon the current modeset configuration, > > > * e.g. it could pick an encoder or output specific CRC sampling point. > > > * > > > @@ -767,6 +769,7 @@ struct drm_crtc_funcs { > > > * 0 on success or a negative error code on failure. > > > */ > > > int (*set_crc_source)(struct drm_crtc *crtc, const char *source); > > > + > > > /** > > > * @verify_crc_source: > > > * > > > -- > > > 2.17.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 7ca486d750e9..1dff956bcc74 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -65,10 +65,17 @@ * it submits. In this general case, the maximum userspace can do is to compare * the reported CRCs of frames that should have the same contents. * - * On the driver side the implementation effort is minimal, drivers only need to - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically - * set up if that vfunc is set. CRC samples need to be captured in the driver by - * calling drm_crtc_add_crc_entry(). + * On the driver side the implementation effort is minimal, drivers only need + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are + * automatically set up if that vfunc is set. CRC samples need to be captured + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit + * (even a full modeset). + * + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the + * legacy paths may interfere with CRC generation. So, in the general case, + * userspace can't rely on the values in crtc-N/crc/data being valid + * across a commit. */ static int crc_control_show(struct seq_file *m, void *data) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 128d8b210621..0f7ea094a900 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -756,6 +756,8 @@ struct drm_crtc_funcs { * provided from the configured source. Drivers must accept an "auto" * source name that will select a default source for this CRTC. * + * This may trigger a commit if necessary, to enable CRC generation. + * * Note that "auto" can depend upon the current modeset configuration, * e.g. it could pick an encoder or output specific CRC sampling point. * @@ -767,6 +769,7 @@ struct drm_crtc_funcs { * 0 on success or a negative error code on failure. */ int (*set_crc_source)(struct drm_crtc *crtc, const char *source); + /** * @verify_crc_source: *
CRC generation can be impacted by commits coming from userspace, and enabling CRC generation may itself trigger a commit. Add notes about this to the kerneldoc. Signed-off-by: Brian Starkey <brian.starkey@arm.com> --- I might have got the wrong end of the stick, but this is what I understood from what you said. Cheers, -Brian drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++---- include/drm/drm_crtc.h | 3 +++ 2 files changed, 14 insertions(+), 4 deletions(-)