| Message ID | 1467723950-14093-3-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 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' > + 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. > + > +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. > + > +int lspcon_change_mode(struct intel_lspcon *lspcon, > + enum drm_lspcon_mode mode, bool force) static > +{ > + 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. > + > + 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 > +{ > + 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. > + > +enum drm_lspcon_mode lspcon_probe(struct intel_lspcon *lspcon) static > +{ > + > + /* 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. > + > + 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. > + DRM_ERROR("LSPCON mode change to PCON failed\n"); > + return false; > + } > + } > + > + DRM_DEBUG_KMS("Success: LSPCON init\n"); > + return true; > +} > -- > 1.9.1
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@intel.com> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 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
On Thu, Aug 11, 2016 at 02:36:23PM +0530, Sharma, Shashank wrote: > 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@intel.com> > >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 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 ? I don't think there's any real chance of a mixup there. It's always going to used as lspcon.mode I think, and the types are incompatible anyway. > >> + 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. We don't use it, so clearly it's not needed. > > > >> + > >> +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. Please don't. It should just be automagic. > >> + > >> +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 I can't see any users I can't possibly judge whether it's sufficient or not. Move any infrastructure to the patches that need it. > > 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 ? The caller is lspcon_probe(). > >> + > >> +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. Can't be since we don't even parse that part from the VBT on other platforms. > >> + > >> + 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
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; + 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; +} + +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"); + 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) { + 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. + */ + 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; +}