[Intel-gfx] [PATCH v2 2/4] drm/i915: Add lspcon support for I915 driver
Sharma, Shashank
shashank.sharma at intel.com
Fri Jul 1 06:22:03 UTC 2016
Regards
Shashank
On 7/1/2016 4:00 AM, Rodrigo Vivi wrote:
> On Tue, Jun 21, 2016 at 8:00 AM, Shashank Sharma
> <shashank.sharma at intel.com> wrote:
>> This patch adds a new file, to accommodate lspcon support
>> for I915 driver. These functions probe, detect, initialize
>> and configure an on-board lspcon device during the driver
>> init time.
>>
>> Also, this patch adds a small structure for lspcon device,
>> which will provide the runtime status of the device.
>>
>> V2: addressed ville's review comments
>> - Clean the leftover macros from previous patch set
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma at intel.com>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/intel_drv.h | 13 +++-
>> drivers/gpu/drm/i915/intel_lspcon.c | 145 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 158 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/i915/intel_lspcon.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 276abf1..d40ff7d 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -93,6 +93,7 @@ i915-y += dvo_ch7017.o \
>> intel_dvo.o \
>> intel_hdmi.o \
>> intel_i2c.o \
>> + intel_lspcon.o \
>> intel_lvds.o \
>> intel_panel.o \
>> intel_sdvo.o \
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7d0e071..4e49c16 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -894,12 +894,19 @@ struct intel_dp {
>> bool compliance_test_active;
>> };
>>
>> +struct intel_lspcon {
>> + bool active;
>> + enum drm_lspcon_mode mode_of_op;
>> + struct drm_dp_aux *aux;
>> +};
>> +
>> struct intel_digital_port {
>> struct intel_encoder base;
>> enum port port;
>> u32 saved_port_bits;
>> struct intel_dp dp;
>> struct intel_hdmi hdmi;
>> + struct intel_lspcon lspcon;
>> enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool);
>> bool release_cl2_override;
>> uint8_t max_lanes;
>> @@ -1450,7 +1457,6 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>> struct intel_crtc_state *pipe_config);
>> void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>
>> -
>> /* intel_lvds.c */
>> void intel_lvds_init(struct drm_device *dev);
>> bool intel_is_dual_link_lvds(struct drm_device *dev);
>> @@ -1745,4 +1751,9 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_crtc_state *state);
>> void intel_color_set_csc(struct drm_crtc_state *crtc_state);
>> void intel_color_load_luts(struct drm_crtc_state *crtc_state);
>>
>> +/* intel_lspcon.c */
>> +bool lspcon_init(struct intel_digital_port *intel_dig_port);
>> +enum drm_connector_status
>> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force);
>> +bool is_lspcon_active(struct intel_digital_port *dig_port);
>> #endif /* __INTEL_DRV_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> new file mode 100644
>> index 0000000..fdeff71
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -0,0 +1,145 @@
>> +/*
>> + * Copyright © 2016 Intel Corporation
>> + *
>> + * 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, sublicense,
>> + * 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 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 <drm/drm_edid.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_dp_dual_mode_helper.h>
>> +#include "intel_drv.h"
>> +
>
> Does this new file worth/deserves/needs a Docbook?
Its a wrapper over the drm_dp_dual_mode layer, where most of the job is
being done. But if you suggest, I can make some documentation.
>
>> +bool is_lspcon_active(struct intel_digital_port *dig_port)
>> +{
>> + return dig_port->lspcon.active;
>> +}
>> +
>> +enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>> +{
>> + enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
>> + struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> + if (drm_lspcon_get_mode(adapter, ¤t_mode))
>> + DRM_ERROR("Error reading LSPCON mode\n");
>> + else
>> + DRM_DEBUG_KMS("Current LSPCON mode %s\n",
>> + current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
>> + return current_mode;
>> +}
>> +
>> +int lspcon_change_mode(struct intel_lspcon *lspcon,
>> + enum drm_lspcon_mode mode, bool force)
>> +{
>> + int err;
>> + enum drm_lspcon_mode current_mode;
>> + struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> + err = drm_lspcon_get_mode(adapter, ¤t_mode);
>> + if (err) {
>> + DRM_ERROR("Error reading LSPCON mode\n");
>> + return err;
>> + }
>> +
>> + if (current_mode == mode && !force) {
>> + DRM_DEBUG_KMS("Current mode = desired LSPCON mode\n");\
>
> debug or error?
> maybe print the desired mode here?
>
> in case desired mode is useless to know and if it happens with
> frequency I'd believe we could skip the debug message and just move
> on...
Actually I was leaving scope for a long-term design, where an IOCTL is
being given to the userspace, which can request a LS->PCON or PCON->LS
mode, and in that case, it would be good to print this considering its a
dumb IOCTL, do you think so ?
>
>> + return 0;
>> + }
>> +
>> + err = drm_lspcon_set_mode(adapter, mode);
>> + if (err < 0) {
>> + DRM_ERROR("LSPCON mode change failed\n");
>> + return err;
>> + }
>> +
>> + lspcon->mode_of_op = mode;
>> + DRM_DEBUG_KMS("LSPCON mode changed done\n");
>> + return 0;
>> +}
>> +
>> +bool lspcon_detect_identifier(struct intel_lspcon *lspcon)
>> +{
>> + enum drm_dp_dual_mode_type adaptor_type;
>> + struct i2c_adapter *adapter = &lspcon->aux->ddc;
>> +
>> + /* Lets probe the adaptor and check its type */
>> + adaptor_type = drm_dp_dual_mode_detect(adapter);
>> + if (adaptor_type != DRM_DP_DUAL_MODE_LSPCON) {
>
> shouldn't we do TYPE2 | bit3 here?
>
I have added LSPCON detection case in drm_dp_dual_mode_detect()
function, so it directly returns DRM_DP_DUAL_MODE_LSPCON when detected :).
>> + DRM_DEBUG_KMS("No LSPCON detected, found %s\n",
>> + drm_dp_get_dual_mode_type_name(adaptor_type));
>> + return false;
>> + }
>> +
>> + /* Yay ... got a LSPCON device */
>> + DRM_DEBUG_KMS("LSPCON detected\n");
>> + return true;
>> +}
>> +
>> +enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)
>> +{
>> +
>> + /* Detect a valid lspcon adaptor */
>> + if (!lspcon_detect_identifier(lspcon)) {
>> + DRM_DEBUG_KMS("No LSPCON identifier found\n");
>> + return false;
>> + }
>> +
>> + /* Get LSPCON's mode of operation */
>> + lspcon->mode_of_op = lspcon_get_current_mode(lspcon);
>> + lspcon->active = true;
>> + return true;
>> +}
>> +
>> +bool lspcon_init(struct intel_digital_port *intel_dig_port)
>> +{
>> + struct intel_dp *dp = &intel_dig_port->dp;
>> + struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>> + struct drm_device *dev = intel_dig_port->base.base.dev;
>> +
>> + if (!IS_GEN9(dev)) {
>> + DRM_ERROR("LSPCON is supported on GEN9 only\n");
>> + return false;
>> + }
>> +
>> + lspcon->active = false;
>> + lspcon->mode_of_op = DRM_LSPCON_MODE_INVALID;
>> + lspcon->aux = &dp->aux;
>> +
>> + if (!lspcon_probe(lspcon)) {
>> + DRM_ERROR("Failed to probe lspcon\n");
>> + return false;
>> + }
>> +
>> + /*
>> + * In the SW state machine, lets Put LSPCON in PCON mode only.
>> + * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
>> + * 2.0 sinks.
>> + */
>
> Reading the specs seems to me that for HDMI 1.4 the LS is the
> recommended and for HDMI 2.0 the PCON is required... but I believe we
> don't have a way to determine that right?
>
If you remember the previous design, we were deciding the mode of
operation, on the modeset, depending on the pixel clock (start in LS
mode, if modeset->clock > 297M make it go on PCON, else keep in LS)
But this approach was causing few problems with previous design and code
review suggestion, like:
- in this scenario, we needed to register dual connectors, one HDMI and
one DP, and this was causing on extra detect to swicth between DP->HDMI
Also, ville suggested not to exploit DDI's dual connector personality
anymore.
- second possibility was to create a new connector for LSPCON, and come
up with all the functions for it. But Paulo thought there was too much
code duplication there, which is making code unnecessary complex.
- If monitor is HDCP2.2 capable, you have to always be in PCON mode to
handle HDCP handshaking.
So based on this history, learning from android products, and code
review suggestions, I thought it would be very simple to have it always
running in PCON mode, which is automatically backward compatible to HDMI
1.4 monitors.
Shashank
>> + if (lspcon->active && lspcon->mode_of_op != DRM_LSPCON_MODE_PCON) {
>> + if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
>> + true) < 0) {
>> + DRM_ERROR("LSPCON mode change to PCON failed\n");
>> + return false;
>> + }
>> + }
>> +
>> + DRM_DEBUG_KMS("Success: LSPCON init\n");
>> + return true;
>> +}
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
More information about the Intel-gfx
mailing list