Re: [RFC PATCH 6/7] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-05-31 Thread Hans Verkuil 

On 05/31/2017 01:57 AM, Clint Taylor wrote:



On 05/26/2017 12:18 AM, Daniel Vetter wrote:

On Thu, May 25, 2017 at 05:06:25PM +0200, Hans Verkuil wrote:

From: Hans Verkuil 

This adds support for the DisplayPort CEC-Tunneling-over-AUX
feature that is part of the DisplayPort 1.3 standard.

Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
chip that has this capability actually hook up the CEC pin, so
even though a CEC device is created, it may not actually work.

Signed-off-by: Hans Verkuil 
---
   drivers/gpu/drm/Kconfig  |   3 +
   drivers/gpu/drm/Makefile |   1 +
   drivers/gpu/drm/drm_dp_cec.c | 196 
+++
   include/drm/drm_dp_helper.h  |  24 ++
   4 files changed, 224 insertions(+)
   create mode 100644 drivers/gpu/drm/drm_dp_cec.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 78d7fc0ebb57..dd771ce8a3d0 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -120,6 +120,9 @@ config DRM_LOAD_EDID_FIRMWARE
  default case is N. Details and instructions how to build your own
  EDID data are given in Documentation/EDID/HOWTO.txt.
   
+config DRM_DP_CEC

+   bool

We generally don't bother with a Kconfig for every little bit in drm, not
worth the trouble (yes I know there's some exceptions, but somehow they're
all from soc people). Just smash this into the KMS_HELPER one and live is
much easier for drivers. Also allows you to drop the dummy inline
functions.

All of the functions like cec_register_adapter() require
CONFIG_MEDIA_CEC_SUPPORT.
This will add a new dependency to the DRM drivers. All instances of
CONFIG_DRM_DP_CEC should be changed to CONFIG_MEDIA_CEC_SUPPORT so drm
can still be used without the media CEC drivers.


This has changed in the next version. I realized the same thing and there
are CEC core patches pending for 4.12 to solve this.

I plan on posting a new patch series for this later this week, and that
will include those patches for 4.12 so it is easier to test this.

Regards,

Hans

Re: [RFC PATCH 6/7] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-05-30 Thread Clint Taylor 



On 05/26/2017 12:18 AM, Daniel Vetter wrote:

On Thu, May 25, 2017 at 05:06:25PM +0200, Hans Verkuil wrote:

From: Hans Verkuil 

This adds support for the DisplayPort CEC-Tunneling-over-AUX
feature that is part of the DisplayPort 1.3 standard.

Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
chip that has this capability actually hook up the CEC pin, so
even though a CEC device is created, it may not actually work.

Signed-off-by: Hans Verkuil 
---
  drivers/gpu/drm/Kconfig  |   3 +
  drivers/gpu/drm/Makefile |   1 +
  drivers/gpu/drm/drm_dp_cec.c | 196 +++
  include/drm/drm_dp_helper.h  |  24 ++
  4 files changed, 224 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_dp_cec.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 78d7fc0ebb57..dd771ce8a3d0 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -120,6 +120,9 @@ config DRM_LOAD_EDID_FIRMWARE
  default case is N. Details and instructions how to build your own
  EDID data are given in Documentation/EDID/HOWTO.txt.
  
+config DRM_DP_CEC

+   bool

We generally don't bother with a Kconfig for every little bit in drm, not
worth the trouble (yes I know there's some exceptions, but somehow they're
all from soc people). Just smash this into the KMS_HELPER one and live is
much easier for drivers. Also allows you to drop the dummy inline
functions.
All of the functions like cec_register_adapter() require 
CONFIG_MEDIA_CEC_SUPPORT.
This will add a new dependency to the DRM drivers. All instances of 
CONFIG_DRM_DP_CEC should be changed to CONFIG_MEDIA_CEC_SUPPORT so drm 
can still be used without the media CEC drivers.


-Clint



The other nitpick: Pls kernel-doc the functions exported to drivers, and
then pull them into Documentation/gpu/drm-kms-helpers.rst, next to the
existing DP helper section.

Thanks, Daniel


+
  config DRM_TTM
tristate
depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 59f0f9b696eb..22f1a17dfc8a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -34,6 +34,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
drm_probe_helper.o \
drm_simple_kms_helper.o drm_modeset_helper.o \
drm_scdc_helper.o
  
+drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o

  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
new file mode 100644
index ..44c544ba53cb
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -0,0 +1,196 @@
+/*
+ * DisplayPort CEC-Tunneling-over-AUX support
+ *
+ * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights 
reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+   struct drm_dp_aux *aux = cec_get_drvdata(adap);
+   ssize_t err = 0;
+
+   if (enable)
+   err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL,
+DP_CEC_TUNNELING_ENABLE);
+   else
+   err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, 0);
+   return (enable && err < 0) ? err : 0;
+}
+
+static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
+{
+   struct drm_dp_aux *aux = cec_get_drvdata(adap);
+   u8 mask[2] = { 0x00, 0x80 };
+   ssize_t err;
+
+   if (addr != CEC_LOG_ADDR_INVALID) {
+   u16 la_mask = adap->log_addrs.log_addr_mask;
+
+   la_mask |= 0x8000 | (1 << addr);
+   mask[0] = la_mask & 0xff;
+   mask[1] = la_mask >> 8;
+   }
+   err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
+   return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
+}
+
+static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+   u32 signal_free_time, struct cec_msg *msg)
+{
+   struct drm_dp_aux

Re: [RFC PATCH 6/7] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-05-26 Thread Daniel Vetter 
On Fri, May 26, 2017 at 12:34 PM, Hans Verkuil  wrote:
>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>> index 78d7fc0ebb57..dd771ce8a3d0 100644
>>> --- a/drivers/gpu/drm/Kconfig
>>> +++ b/drivers/gpu/drm/Kconfig
>>> @@ -120,6 +120,9 @@ config DRM_LOAD_EDID_FIRMWARE
>>>default case is N. Details and instructions how to build your own
>>>EDID data are given in Documentation/EDID/HOWTO.txt.
>>>
>>> +config DRM_DP_CEC
>>> +bool
>>
>> We generally don't bother with a Kconfig for every little bit in drm, not
>> worth the trouble (yes I know there's some exceptions, but somehow they're
>> all from soc people). Just smash this into the KMS_HELPER one and live is
>> much easier for drivers. Also allows you to drop the dummy inline
>> functions.
>
> For all other CEC implementations I have placed it under a config option. The
> reason is that 1) CEC is an optional feature of HDMI and you may not actually
> want it, and 2) enabling CEC also pulls in the cec module.
>
> I still think turning this into a drm config option makes sense. This would
> replace the i915 config option I made in the next patch, i.e. this config 
> option
> is moved up one level.
>
> Your choice, though.

If there is a CEC option already, can we just reuse that one? I.e.
when it's enabled, we compile the drm dp cec helpers, if it's not, you
get the pile of dummy functions. drm_dp_cec.c should still be part of
drm_kms_helper.ko though I think (since the dp aux stuff is in there
anyway, doesn't make sense to split it).

I'm still not sold on Kconfig proliferation for optional features
(have one for the driver, that's imo enough), but if it exists already
not going to block it's use in drm. As long as it's minimally invasive
on the code and drivers don't have to care at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 6/7] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-05-26 Thread Hans Verkuil 
On 05/26/2017 09:18 AM, Daniel Vetter wrote:
> On Thu, May 25, 2017 at 05:06:25PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> This adds support for the DisplayPort CEC-Tunneling-over-AUX
>> feature that is part of the DisplayPort 1.3 standard.
>>
>> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
>> chip that has this capability actually hook up the CEC pin, so
>> even though a CEC device is created, it may not actually work.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/gpu/drm/Kconfig  |   3 +
>>  drivers/gpu/drm/Makefile |   1 +
>>  drivers/gpu/drm/drm_dp_cec.c | 196 
>> +++
>>  include/drm/drm_dp_helper.h  |  24 ++
>>  4 files changed, 224 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 78d7fc0ebb57..dd771ce8a3d0 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -120,6 +120,9 @@ config DRM_LOAD_EDID_FIRMWARE
>>default case is N. Details and instructions how to build your own
>>EDID data are given in Documentation/EDID/HOWTO.txt.
>>  
>> +config DRM_DP_CEC
>> +bool
> 
> We generally don't bother with a Kconfig for every little bit in drm, not
> worth the trouble (yes I know there's some exceptions, but somehow they're
> all from soc people). Just smash this into the KMS_HELPER one and live is
> much easier for drivers. Also allows you to drop the dummy inline
> functions.

For all other CEC implementations I have placed it under a config option. The
reason is that 1) CEC is an optional feature of HDMI and you may not actually
want it, and 2) enabling CEC also pulls in the cec module.

I still think turning this into a drm config option makes sense. This would
replace the i915 config option I made in the next patch, i.e. this config option
is moved up one level.

Your choice, though.

> The other nitpick: Pls kernel-doc the functions exported to drivers, and
> then pull them into Documentation/gpu/drm-kms-helpers.rst, next to the
> existing DP helper section.

Will do.

BTW, do you know if it is possible to detect when a DP-to-HDMI adapter is
connected as I discussed in my cover letter? That's my main open question
for this patch series.

Regarding the other thing I discussed in the cover letter about detecting if
the CEC pin is really hooked up: I think I shouldn't try to be smart. Yes, I
can try to poll for a TV, but that doesn't really say anything about whether
CEC is working or not since the TV itself may not have enabled CEC (actually
quite common).

One alternative might be to poll and, if no TV is detected, call dev_info to
let the user know that either there is no CEC-enabled TV, or the CEC pin isn't
connected.

I'm not sure if that helps the user or not.

Regards,

Hans

> 
> Thanks, Daniel

Re: [RFC PATCH 6/7] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-05-26 Thread Daniel Vetter 
On Thu, May 25, 2017 at 05:06:25PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This adds support for the DisplayPort CEC-Tunneling-over-AUX
> feature that is part of the DisplayPort 1.3 standard.
> 
> Unfortunately, not all DisplayPort/USB-C to HDMI adapters with a
> chip that has this capability actually hook up the CEC pin, so
> even though a CEC device is created, it may not actually work.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  drivers/gpu/drm/Kconfig  |   3 +
>  drivers/gpu/drm/Makefile |   1 +
>  drivers/gpu/drm/drm_dp_cec.c | 196 
> +++
>  include/drm/drm_dp_helper.h  |  24 ++
>  4 files changed, 224 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_dp_cec.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 78d7fc0ebb57..dd771ce8a3d0 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -120,6 +120,9 @@ config DRM_LOAD_EDID_FIRMWARE
> default case is N. Details and instructions how to build your own
> EDID data are given in Documentation/EDID/HOWTO.txt.
>  
> +config DRM_DP_CEC
> + bool

We generally don't bother with a Kconfig for every little bit in drm, not
worth the trouble (yes I know there's some exceptions, but somehow they're
all from soc people). Just smash this into the KMS_HELPER one and live is
much easier for drivers. Also allows you to drop the dummy inline
functions.

The other nitpick: Pls kernel-doc the functions exported to drivers, and
then pull them into Documentation/gpu/drm-kms-helpers.rst, next to the
existing DP helper section.

Thanks, Daniel

> +
>  config DRM_TTM
>   tristate
>   depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 59f0f9b696eb..22f1a17dfc8a 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -34,6 +34,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
> drm_probe_helper.o \
>   drm_simple_kms_helper.o drm_modeset_helper.o \
>   drm_scdc_helper.o
>  
> +drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> new file mode 100644
> index ..44c544ba53cb
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -0,0 +1,196 @@
> +/*
> + * DisplayPort CEC-Tunneling-over-AUX support
> + *
> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights 
> reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int drm_dp_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> + struct drm_dp_aux *aux = cec_get_drvdata(adap);
> + ssize_t err = 0;
> +
> + if (enable)
> + err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL,
> +  DP_CEC_TUNNELING_ENABLE);
> + else
> + err = drm_dp_dpcd_writeb(aux, DP_CEC_TUNNELING_CONTROL, 0);
> + return (enable && err < 0) ? err : 0;
> +}
> +
> +static int drm_dp_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> +{
> + struct drm_dp_aux *aux = cec_get_drvdata(adap);
> + u8 mask[2] = { 0x00, 0x80 };
> + ssize_t err;
> +
> + if (addr != CEC_LOG_ADDR_INVALID) {
> + u16 la_mask = adap->log_addrs.log_addr_mask;
> +
> + la_mask |= 0x8000 | (1 << addr);
> + mask[0] = la_mask & 0xff;
> + mask[1] = la_mask >> 8;
> + }
> + err = drm_dp_dpcd_write(aux, DP_CEC_LOGICAL_ADDRESS_MASK, mask, 2);
> + return (addr != CEC_LOG_ADDR_INVALID && err < 0) ? err : 0;
> +}
> +
> +static int drm_dp_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> + u32 signal_free_time, struct cec_msg *msg)
> +{
> + struct drm_dp_aux *aux = cec_get_drvdata(adap);
> + unsigned int retries = attempts - 1;
> + ssize_t err;
> +
> + err = drm_dp_dpcd_write(aux, DP_CEC_TX_MESSAGE_BUFFER,
> +