Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 5 Aug 2019 18:24:17 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Brian Starkey <brian.starkey@arm.com>
Cc:     Liviu Dudau <Liviu.Dudau@arm.com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>,
        "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>, nd@arm.com,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit
 interactions
Message-ID: <20190805162417.GS7444@phenom.ffwll.local>
Mail-Followup-To: Brian Starkey <brian.starkey@arm.com>,
        Liviu Dudau <Liviu.Dudau@arm.com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>,
        "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>, nd@arm.com,
        dri-devel@lists.freedesktop.org,
        LKML <linux-kernel@vger.kernel.org>
References: <20190802140910.GN7444@phenom.ffwll.local>
 <20190805151143.12317-1-brian.starkey@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190805151143.12317-1-brian.starkey@arm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch