[Intel-gfx] [PATCH v2 2/4] drm/i915: Add lspcon support for I915 driver
Rodrigo Vivi
rodrigo.vivi at gmail.com
Sat Jul 2 00:13:53 UTC 2016
On Thu, Jun 30, 2016 at 11:22 PM, Sharma, Shashank
<shashank.sharma at intel.com> wrote:
> 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.
you are right... there is where the interfaces matter although LSPCON
we just have here for now.
Anyway no strong opinion from my part.
>
>>
>>> +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 ?
oh I see...
well, if setting with ioctl they probably know what they request
right... so ignore me ;)
>>
>>
>>> + 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 :).
Oh that is true.... now I got why you did the mode like this! ;)
>
>>> + 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.
if it falls back automatically I agree this is the best cleaner approach.
so, with or without the doc feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
> 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
>>
>>
>>
>>
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
More information about the Intel-gfx
mailing list