Regards
Shashank
On 8/11/2016 12:33 PM, Ville Syrjälä wrote:
On Tue, Jul 05, 2016 at 06:35:48PM +0530, Shashank Sharma 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
V3: Rebase
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
Signed-off-by: Akashdeep Sharma <akashdeep.sharma@xxxxxxxxx>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
---
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 618293c..64cd373 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -95,6 +95,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 e6a24d2..e6982cf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -922,12 +922,19 @@ struct intel_dp {
bool compliance_test_active;
};
+struct intel_lspcon {
+ bool active;
+ enum drm_lspcon_mode mode_of_op;
I'd call this just 'mode'
I dont want reader to get confused this with a videomode, so made it
more clear :)
Do you think we can keep it this way ?
+ 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;
@@ -1478,7 +1485,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);
struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
@@ -1779,4 +1785,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"
+
+bool is_lspcon_active(struct intel_digital_port *dig_port)
+{
+ return dig_port->lspcon.active;
+}
unused -> kill it
In fact the whole lspcon.active flags seems pretty pointless, so I'd
just kill that too.
Please note that, I tried to address both motherboard-down mode as well
as adapter-mode LSPCON
in this design. In case of adapter mode lspcon, we will need this.
+
+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;
+}
I was going to suggest killing this one too, but I guess the debug
output might be useful. Make it static at least.
Sure, will make it static. Also we have a plan to expose one DRM
property to handle adapter mode lspcon
at that time this will help.
+
+int lspcon_change_mode(struct intel_lspcon *lspcon,
+ enum drm_lspcon_mode mode, bool force)
static
Sure.
+{
+ 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");
+ return 0;
+ }
We always call this with force==true, so it looks like a pointless
parameter. Also if force==true always, it doesn't necessary to even
query the current mode. Oh and I think we've already queried the mode
when this gets called.
As mentioned, I wanted to keep the scope of switching lspcon modes, in
the design infrastructure.
So I kept these things as possibility.
IF we create a DRM property to query LSPCON mode, that get_property
function will call this
function with force = false, coz for this query we dont want to probe
the HW again.
+
+ 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)
static
got it.
+{
+ 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) {
+ 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;
+}
This is small function and called only once, so I'd just inline it into
the caller actually.
Can be done, but the caller is intelI_ddi_init, which I dont want to
populate with lspcon specific code, when we have a separate file
for lspcon. Do you think its a good idea ? or you prefer to have that in
ddi_init only ?
+
+enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon)
static
Got it.
+{
+
+ /* 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;
+ }
Just remove this check? If it's not present, we won't detect it.
Faulty VBT ? Just being paranoid.
+
+ 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.
+ */
+ if (lspcon->active && lspcon->mode_of_op != DRM_LSPCON_MODE_PCON) {
+ if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
+ true) < 0) {
indent fail.
same as previous.
Regards
Shashank
+ 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
