* Re: [PATCH] drm/dp: add drm_dp_link_power_down() helper
2014-12-02 15:46 [PATCH] drm/dp: add drm_dp_link_power_down() helper Rob Clark
@ 2014-12-02 15:58 ` Thierry Reding
2014-12-02 16:40 ` Alex Deucher
2014-12-03 10:11 ` Jani Nikula
2 siblings, 0 replies; 4+ messages in thread
From: Thierry Reding @ 2014-12-02 15:58 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 497 bytes --]
On Tue, Dec 02, 2014 at 10:46:42AM -0500, Rob Clark wrote:
> We had _power_up(), but drivers also need to be able to power down.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> This lets me drop edp_sink_power_state() from msm eDP code, and use
> the helpers instead.
>
> drivers/gpu/drm/drm_dp_helper.c | 31 +++++++++++++++++++++++++++++++
> include/drm/drm_dp_helper.h | 1 +
> 2 files changed, 32 insertions(+)
Reviewed-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/dp: add drm_dp_link_power_down() helper
2014-12-02 15:46 [PATCH] drm/dp: add drm_dp_link_power_down() helper Rob Clark
2014-12-02 15:58 ` Thierry Reding
@ 2014-12-02 16:40 ` Alex Deucher
2014-12-03 10:11 ` Jani Nikula
2 siblings, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2014-12-02 16:40 UTC (permalink / raw)
To: Rob Clark; +Cc: Maling list - DRI developers
On Tue, Dec 2, 2014 at 10:46 AM, Rob Clark <robdclark@gmail.com> wrote:
> We had _power_up(), but drivers also need to be able to power down.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> This lets me drop edp_sink_power_state() from msm eDP code, and use
> the helpers instead.
>
> drivers/gpu/drm/drm_dp_helper.c | 31 +++++++++++++++++++++++++++++++
> include/drm/drm_dp_helper.h | 1 +
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 959e207..82122ec 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -353,6 +353,37 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
> EXPORT_SYMBOL(drm_dp_link_power_up);
>
> /**
> + * drm_dp_link_power_down() - power down a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> + u8 value;
> + int err;
> +
> + /* DP_SET_POWER register is only available on DPCD v1.1 and later */
> + if (link->revision < 0x11)
> + return 0;
> +
> + err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
> + if (err < 0)
> + return err;
> +
> + value &= ~DP_SET_POWER_MASK;
> + value |= DP_SET_POWER_D3;
> +
> + err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_link_power_down);
> +
> +/**
> * drm_dp_link_configure() - configure a DisplayPort link
> * @aux: DisplayPort AUX channel
> * @link: pointer to a structure containing the link configuration
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 11f8c84..7e25030 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -586,6 +586,7 @@ struct drm_dp_link {
>
> int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
> int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>
> int drm_dp_aux_register(struct drm_dp_aux *aux);
> --
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/dp: add drm_dp_link_power_down() helper
2014-12-02 15:46 [PATCH] drm/dp: add drm_dp_link_power_down() helper Rob Clark
2014-12-02 15:58 ` Thierry Reding
2014-12-02 16:40 ` Alex Deucher
@ 2014-12-03 10:11 ` Jani Nikula
2 siblings, 0 replies; 4+ messages in thread
From: Jani Nikula @ 2014-12-03 10:11 UTC (permalink / raw)
To: Rob Clark, dri-devel; +Cc: Syrjala, Ville
On Tue, 02 Dec 2014, Rob Clark <robdclark@gmail.com> wrote:
> We had _power_up(), but drivers also need to be able to power down.
Patch looks good, I'm just hijacking the thread to talk about the
_power_up() counterpart. Sorry. ;)
First, I'm not sure it's all right or sensible to read DP_SET_POWER
first when the sink is sleeping. Shouldn't it just write DP_SET_POWER_D0
to DP_SET_POWER?
Second, I think _power_up() should retry the wake up to ensure an AUX
reply. (If we decide it's all right to read DP_SET_POWER first, I think
it should have a retry too.)
DP v1.2 section 5.1.5 (section 5.2.5 is also relevant):
"""
The Source device must write the value of 1h to DPCD Address 600h of the
downstream device via AUX CH to switch the uPacket RX of the downstream
device out of power-save mode. A uPacket RX of a downstream device in a
power-save mode may not reply to this AUX request transaction. The
uPacket RX of a downstream device in a power-save mode for an open,
box-to-box connection is allowed to take up to 1ms till it is ready to
reply to the AUX request transaction. Therefore, the Source device must
retry until the 1ms wake-up timeout period of the Sink device expires.
"""
This also means that _probe() probably should not be used on sinks that
are in sleep. Therefore _power_up() after _probe() seems redundant too
(one example in tegra_output_sor_enable).
I seem to have reviewed the _power_up() and _probe() patch, so you can
blame me just as well...
BR,
Jani.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> This lets me drop edp_sink_power_state() from msm eDP code, and use
> the helpers instead.
>
> drivers/gpu/drm/drm_dp_helper.c | 31 +++++++++++++++++++++++++++++++
> include/drm/drm_dp_helper.h | 1 +
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 959e207..82122ec 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -353,6 +353,37 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
> EXPORT_SYMBOL(drm_dp_link_power_up);
>
> /**
> + * drm_dp_link_power_down() - power down a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> + u8 value;
> + int err;
> +
> + /* DP_SET_POWER register is only available on DPCD v1.1 and later */
> + if (link->revision < 0x11)
> + return 0;
> +
> + err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
> + if (err < 0)
> + return err;
> +
> + value &= ~DP_SET_POWER_MASK;
> + value |= DP_SET_POWER_D3;
> +
> + err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_link_power_down);
> +
> +/**
> * drm_dp_link_configure() - configure a DisplayPort link
> * @aux: DisplayPort AUX channel
> * @link: pointer to a structure containing the link configuration
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 11f8c84..7e25030 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -586,6 +586,7 @@ struct drm_dp_link {
>
> int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
> int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>
> int drm_dp_aux_register(struct drm_dp_aux *aux);
> --
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread