Re: [PATCH v3 2/4] drm/i915: Add lspcon support for I915 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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 ?

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, &current_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, &current_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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux