[Intel-gfx] [PATCH v4 1/6] drm: Add SCDC helpers

Sharma, Shashank shashank.sharma at intel.com
Thu Feb 23 12:09:24 UTC 2017


Regards

Shashank


On 2/23/2017 5:11 PM, Ville Syrjälä wrote:
> On Thu, Feb 23, 2017 at 08:51:03AM +0530, Sharma, Shashank wrote:
>> Thanks for the review Ville, my comments inline.
>>
>> Regards
>>
>> Shashank
>>
>>
>> On 2/22/2017 10:39 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 22, 2017 at 06:48:26PM +0530, Shashank Sharma wrote:
>>>> From: Thierry Reding <treding at nvidia.com>
>>>>
>>>> SCDC is a mechanism defined in the HDMI 2.0 specification that allows
>>>> the source and sink devices to communicate.
>>>>
>>>> This commit introduces helpers to access the SCDC and provides the
>>>> symbolic names for the various registers defined in the specification.
>>>>
>>>> V2: Rebase.
>>>> V3: Added R-B from Jose.
>>>> V4: Rebase
>>>>
>>>> Signed-off-by: Thierry Reding <treding at nvidia.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> Reviewed-by: Jose Abreu <joabreu at synopsys.com>
>>>> ---
>>>>    Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>>>>    drivers/gpu/drm/Makefile              |   3 +-
>>>>    drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
>>>>    include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 257 insertions(+), 1 deletion(-)
>>>>    create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
>>>>    create mode 100644 include/drm/drm_scdc_helper.h
>>>>
>>>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
>>>> index 03040aa..7592756 100644
>>>> --- a/Documentation/gpu/drm-kms-helpers.rst
>>>> +++ b/Documentation/gpu/drm-kms-helpers.rst
>>>> @@ -217,6 +217,18 @@ EDID Helper Functions Reference
>>>>    .. kernel-doc:: drivers/gpu/drm/drm_edid.c
>>>>       :export:
>>>>    
>>>> +SCDC Helper Functions Reference
>>>> +===============================
>>>> +
>>>> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
>>>> +   :doc: scdc helpers
>>>> +
>>>> +.. kernel-doc:: include/drm/drm_scdc_helper.h
>>>> +   :internal:
>>>> +
>>>> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
>>>> +   :export:
>>>> +
>>>>    Rectangle Utilities Reference
>>>>    =============================
>>>>    
>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>> index 92de399..ad91cd9 100644
>>>> --- a/drivers/gpu/drm/Makefile
>>>> +++ b/drivers/gpu/drm/Makefile
>>>> @@ -31,7 +31,8 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>>>>    drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>>>>    		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>>>>    		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>>>> -		drm_simple_kms_helper.o drm_modeset_helper.o
>>>> +		drm_simple_kms_helper.o drm_modeset_helper.o \
>>>> +		drm_scdc_helper.o
>>>>    
>>>>    drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>>>    drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>>> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
>>>> new file mode 100644
>>>> index 0000000..fe0e121
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/drm_scdc_helper.c
>>>> @@ -0,0 +1,111 @@
>>>> +/*
>>>> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the "Software"),
>>>> + * to deal in the Software without restriction, including without limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
>>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> + * next paragraph) shall be included in all copies or substantial portions
>>>> + * of the Software.
>>>> + *
>>>> + * 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 NON-INFRINGEMENT. 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 <linux/slab.h>
>>>> +
>>>> +#include <drm/drm_scdc_helper.h>
>>>> +
>>>> +/**
>>>> + * DOC: scdc helpers
>>>> + *
>>>> + * Status and Control Data Channel (SCDC) is a mechanism introduced by the
>>>> + * HDMI 2.0 specification. It is a point-to-point protocol that allows the
>>>> + * HDMI source and HDMI sink to exchange data. The same I2C interface that
>>>> + * is used to access EDID serves as the transport mechanism for SCDC.
>>>> + */
>>>> +
>>>> +#define SCDC_I2C_SLAVE_ADDRESS 0x54
>>>> +
>>>> +/**
>>>> + * drm_scdc_read - read a block of data from SCDC
>>>> + * @adapter: I2C controller
>>>> + * @offset: start offset of block to read
>>>> + * @buffer: return location for the block to read
>>>> + * @size: size of the block to read
>>>> + *
>>>> + * Reads a block of data from SCDC, starting at a given offset.
>>>> + *
>>>> + * Returns:
>>>> + * The number of bytes read from SCDC or a negative error code on failure.
>>>> + */
>>>> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
>>>> +		      size_t size)
>>>> +{
>>>> +	struct i2c_msg msgs[2] = {
>>>> +		{
>>>> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
>>>> +			.flags = 0,
>>>> +			.len = 1,
>>>> +			.buf = &offset,
>>>> +		}, {
>>>> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
>>>> +			.flags = I2C_M_RD,
>>>> +			.len = size,
>>>> +			.buf = buffer,
>>>> +		}
>>>> +	};
>>>> +
>>>> +	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>>> That still disagrees with the documentation. I would suggest doing what
>>> the DP dual mode helper does.
>> Sure, I guess this is about how we are handling the return values from
>> I2C layer, I will change it
>> the way its handled in drm_dp_dual_mode functions
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_read);
>>>> +
>>>> +/**
>>>> + * drm_scdc_write - write a block of data to SCDC
>>>> + * @adapter: I2C controller
>>>> + * @offset: start offset of block to write
>>>> + * @buffer: block of data to write
>>>> + * @size: size of the block to write
>>>> + *
>>>> + * Writes a block of data to SCDC, starting at a given offset.
>>>> + *
>>>> + * Returns:
>>>> + * The number of bytes written to SCDC or a negative error code on failure.
>>>> + */
>>>> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>>> +		       const void *buffer, size_t size)
>>>> +{
>>>> +	struct i2c_msg msg = {
>>>> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
>>>> +		.flags = 0,
>>>> +		.len = 1 + size,
>>>> +		.buf = NULL,
>>>> +	};
>>>> +	void *data;
>>>> +	int err;
>>>> +
>>>> +	data = kmalloc(1 + size, GFP_TEMPORARY);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	msg.buf = data;
>>>> +
>>>> +	memcpy(data, &offset, sizeof(offset));
>>>> +	memcpy(data + 1, buffer, size);
>>>> +
>>>> +	err = i2c_transfer(adapter, &msg, 1);
>>>> +
>>>> +	kfree(data);
>>>> +
>>>> +	return err;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_write);
>>>> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
>>>> new file mode 100644
>>>> index 0000000..93b07bc
>>>> --- /dev/null
>>>> +++ b/include/drm/drm_scdc_helper.h
>>>> @@ -0,0 +1,132 @@
>>>> +/*
>>>> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the "Software"),
>>>> + * to deal in the Software without restriction, including without limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
>>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> + * next paragraph) shall be included in all copies or substantial portions
>>>> + * of the Software.
>>>> + *
>>>> + * 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 NON-INFRINGEMENT. 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.
>>>> + */
>>>> +
>>>> +#ifndef DRM_SCDC_HELPER_H
>>>> +#define DRM_SCDC_HELPER_H
>>>> +
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +#define SCDC_SINK_VERSION 0x01
>>>> +
>>>> +#define SCDC_SOURCE_VERSION 0x02
>>>> +
>>>> +#define SCDC_UPDATE_0 0x10
>>>> +#define  SCDC_READ_REQUEST_TEST (1 << 2)
>>>> +#define  SCDC_CED_UPDATE (1 << 1)
>>>> +#define  SCDC_STATUS_UPDATE (1 << 0)
>>>> +
>>>> +#define SCDC_UPDATE_1 0x11
>>>> +
>>>> +#define SCDC_TMDS_CONFIG 0x20
>>>> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1)
>>>> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
>>>> +#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
>>>> +
>>>> +#define SCDC_SCRAMBLER_STATUS 0x21
>>>> +#define  SCDC_SCRAMBLING_STATUS (1 << 0)
>>>> +
>>>> +#define SCDC_CONFIG_0 0x30
>>>> +#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
>>>> +
>>>> +#define SCDC_STATUS_FLAGS_0 0x40
>>>> +#define  SCDC_CH2_LOCK (1 < 3)
>>>> +#define  SCDC_CH1_LOCK (1 < 2)
>>>> +#define  SCDC_CH0_LOCK (1 < 1)
>>>> +#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | SCDC_CH0_LOCK)
>>>> +#define  SCDC_CLOCK_DETECT (1 << 0)
>>>> +
>>>> +#define SCDC_STATUS_FLAGS_1 0x41
>>>> +
>>>> +#define SCDC_ERR_DET_0_L 0x50
>>>> +#define SCDC_ERR_DET_0_H 0x51
>>>> +#define SCDC_ERR_DET_1_L 0x52
>>>> +#define SCDC_ERR_DET_1_H 0x53
>>>> +#define SCDC_ERR_DET_2_L 0x54
>>>> +#define SCDC_ERR_DET_2_H 0x55
>>>> +#define  SCDC_CHANNEL_VALID (1 << 7)
>>>> +
>>>> +#define SCDC_ERR_DET_CHECKSUM 0x56
>>>> +
>>>> +#define SCDC_TEST_CONFIG_0 0xc0
>>>> +#define  SCDC_TEST_READ_REQUEST (1 << 7)
>>>> +#define  SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
>>>> +
>>>> +#define SCDC_MANUFACTURER_IEEE_OUI 0xd0
>>>> +#define SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
>>>> +
>>>> +#define SCDC_DEVICE_ID 0xd3
>>>> +#define SCDC_DEVICE_ID_SIZE 8
>>>> +
>>>> +#define SCDC_DEVICE_HARDWARE_REVISION 0xdb
>>>> +#define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf)
>>>> +#define  SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
>>> We don't usually define macros like this to extract information from the
>>> register. It's easy to confuse them with the normal macros for setting
>>> bits.
>> How about if I add another sub macro called _MAJOR, and use it like this:
>>
>> #define _MAJOR(x) (x & 0xf)
>> #define  SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (_MAJOR((x) >> 4))
>>
>> Or will you prefer no macro for this job at all ?
> I think the usual pattern is a SHIFT + MASK macros for extracting
> things from register values. But that is a little error prone and rather
> verbose to use, so I do actually like the idea of having macros for
> extracting bits from registers.
>
> Hmm. After looking around a bit, I do see that DP_GET_SINK_COUNT() is an
> extraction macro similar to what you have here. If we follow that example
> then just adding _GET_ to the name should be good enough to distinguish
> this from any macro to set the bits.
Cool, I will change the macro names as suggested.
Thanks

- Shashank
>> - Shashank
>>>> +
>>>> +#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc
>>>> +#define SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
>>>> +
>>>> +#define SCDC_MANUFACTURER_SPECIFIC 0xde
>>>> +#define SCDC_MANUFACTURER_SPECIFIC_SIZE 34
>>>> +
>>>> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
>>>> +		      size_t size);
>>>> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>>> +		       const void *buffer, size_t size);
>>>> +
>>>> +/**
>>>> + * drm_scdc_readb - read a single byte from SCDC
>>>> + * @adapter: I2C adapter
>>>> + * @offset: offset of register to read
>>>> + * @value: return location for the register value
>>>> + *
>>>> + * Reads a single byte from SCDC. This is a convenience wrapper around the
>>>> + * drm_scdc_read() function.
>>>> + *
>>>> + * Returns:
>>>> + * 0 on success or a negative error code on failure.
>>>> + */
>>>> +static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
>>>> +				 u8 *value)
>>>> +{
>>>> +	return drm_scdc_read(adapter, offset, value, sizeof(*value));
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_scdc_writeb - write a single byte to SCDC
>>>> + * @adapter: I2C adapter
>>>> + * @offset: offset of register to read
>>>> + * @value: return location for the register value
>>>> + *
>>>> + * Writes a single byte to SCDC. This is a convenience wrapper around the
>>>> + * drm_scdc_write() function.
>>>> + *
>>>> + * Returns:
>>>> + * 0 on success or a negative error code on failure.
>>>> + */
>>>> +static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
>>>> +				  u8 value)
>>>> +{
>>>> +	return drm_scdc_write(adapter, offset, &value, sizeof(value));
>>>> +}
>>>> +
>>>> +#endif
>>>> -- 
>>>> 1.9.1



More information about the Intel-gfx mailing list