| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOw6vbLqb7_mGhZu2gjjhRXZCVnLFiFQ0XVvO9WQZLL5HmGJgQ@mail.gmail.com>
Date: Mon, 9 Jan 2017 11:45:47 -0500
From: Sean Paul <seanpaul@...omium.org>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Ville Syrjälä <ville.syrjala@...ux.intel.com>,
Thierry Reding <thierry.reding@...il.com>,
intel-gfx-trybot@...ts.freedesktop.org,
Emil Velikov <emil.l.velikov@...il.com>,
Daniel Vetter <daniel.vetter@...el.com>,
Benjamin Gaignard <benjamin.gaignard@...aro.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
dri-devel <dri-devel@...ts.freedesktop.org>,
David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs
On Mon, Jan 9, 2017 at 8:32 AM, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
> Adds helpers for starting and stopping capture of frame CRCs through the
> DPCD. When capture is on, a worker waits for vblanks and retrieves the
> frame CRC to put it in the queue on the CRTC that is using the
> eDP connector, so it's passed to userspace.
>
> v2: Reuse drm_crtc_wait_one_vblank
> Update locking, as drm_crtc_add_crc_entry now takes the lock
>
> v3: Don't call wake_up_interruptible directly, that's now done in
> drm_crtc_add_crc_entry.
>
> v4: Style fixes (Sean Paul)
> Reworked retry of CRC reads (Sean Paul)
> Flush worker after stopping CRC generationa (Sean Paul)
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> ---
>
> drivers/gpu/drm/drm_dp_helper.c | 129 ++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_dp_helper.h | 7 +++
> 2 files changed, 136 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e6fe82c6d64..f9018faf7e07 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -981,6 +981,83 @@ static const struct i2c_lock_operations drm_dp_i2c_lock_ops = {
> .unlock_bus = unlock_bus,
> };
>
> +static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
> +{
> + u8 buf, count;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
> + if (ret < 0)
> + return ret;
> +
> + WARN_ON(!(buf & DP_TEST_SINK_START));
> +
> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
> + if (ret < 0)
> + return ret;
> +
> + count = buf & DP_TEST_COUNT_MASK;
> + if (count == aux->crc_count)
> + return -EAGAIN; /* No CRC yet */
> +
> + aux->crc_count = count;
> +
> + /*
> + * At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
> + * per component (RGB or CrYCb).
> + */
> + ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void drm_dp_aux_crc_work(struct work_struct *work)
> +{
> + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> + crc_work);
> + struct drm_crtc *crtc;
> + u8 crc_bytes[6];
> + uint32_t crcs[3];
> + bool retry = false;
> + int ret;
> +
> + if (WARN_ON(!aux->connector))
> + return;
> +
> + crtc = aux->connector->state->crtc;
> + while (crtc->crc.opened) {
> + if (!retry) {
> + drm_crtc_wait_one_vblank(crtc);
> + if (!crtc->crc.opened)
> + break;
> + }
> +
> + ret = drm_dp_aux_get_crc(aux, crc_bytes);
> + if (ret == -EAGAIN) {
> + if (retry)
> + DRM_DEBUG_KMS("Failed to get a CRC even after retrying: %d\n",
> + ret);
> + retry = !retry;
> + usleep_range(1000, 2000);
> + continue;
> + }
> +
> + retry = false;
Yeah, the retry handling gets ugly fast, doesn't it? I think I'm
coming around to your original way of handing things (with the
addition of handling EAGAIN separately from other errors)
Sean
> + if (!ret) {
> + crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
> + crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
> + crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
> + ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
> + if (ret)
> + DRM_DEBUG_KMS("Failed to add crc entry %d\n", ret);
> + } else {
> + DRM_DEBUG_KMS("Get CRC failed: %d\n", ret);
> + }
> + }
> +}
> +
> /**
> * drm_dp_aux_init() - minimally initialise an aux channel
> * @aux: DisplayPort AUX channel
> @@ -993,6 +1070,7 @@ static const struct i2c_lock_operations drm_dp_i2c_lock_ops = {
> void drm_dp_aux_init(struct drm_dp_aux *aux)
> {
> mutex_init(&aux->hw_mutex);
> + INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
>
> aux->ddc.algo = &drm_dp_i2c_algo;
> aux->ddc.algo_data = aux;
> @@ -1081,3 +1159,54 @@ int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
> EXPORT_SYMBOL(drm_dp_psr_setup_time);
>
> #undef PSR_SETUP_TIME
> +
> +/**
> + * drm_dp_start_crc() - start capture of frame CRCs
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_start_crc(struct drm_dp_aux *aux)
> +{
> + u8 buf;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
> + if (ret < 0)
> + return ret;
> +
> + ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
> + if (ret < 0)
> + return ret;
> +
> + aux->crc_count = 0;
> + schedule_work(&aux->crc_work);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_start_crc);
> +
> +/**
> + * drm_dp_stop_crc() - stop capture of frame CRCs
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_stop_crc(struct drm_dp_aux *aux)
> +{
> + u8 buf;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
> + if (ret < 0)
> + return ret;
> +
> + ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf & ~DP_TEST_SINK_START);
> + if (ret < 0)
> + return ret;
> +
> + flush_work(&aux->crc_work);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_stop_crc);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 4fa77b434594..276e1ecd947b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -723,6 +723,8 @@ struct drm_dp_aux_msg {
> * @dev: pointer to struct device that is the parent for this AUX channel
> * @connector: backpointer to connector that uses this AUX channel
> * @hw_mutex: internal mutex used for locking transfers
> + * @crc_work: worker that captures CRCs for each frame
> + * @crc_count: counter of captured frame CRCs
> * @transfer: transfers a message representing a single AUX transaction
> *
> * The .dev field should be set to a pointer to the device that implements
> @@ -760,6 +762,8 @@ struct drm_dp_aux {
> struct device *dev;
> struct drm_connector *connector;
> struct mutex hw_mutex;
> + struct work_struct crc_work;
> + u8 crc_count;
> ssize_t (*transfer)(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg);
> /**
> @@ -838,4 +842,7 @@ void drm_dp_aux_init(struct drm_dp_aux *aux);
> int drm_dp_aux_register(struct drm_dp_aux *aux);
> void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>
> +int drm_dp_start_crc(struct drm_dp_aux *aux);
> +int drm_dp_stop_crc(struct drm_dp_aux *aux);
> +
> #endif /* _DRM_DP_HELPER_H_ */
> --
> 2.9.3
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Powered by blists - more mailing lists