| Message ID | 1517998657-17188-5-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 07 Feb 2018, Shashank Sharma <shashank.sharma@intel.com> wrote: > From: "Sharma, Shashank" <shashank.sharma@intel.com> > > Intel LSPCON chip is provided by 2 vendors: > - Megachips America (MCA) > - Parade technologies (Parade tech) > > Its important to know the vendor of this chip, as the address to > write AVI infoframes is different for those two. > > This patch reads the vendor OUI signature, and marks into LSPCON > encoder structure for future usages. > > This patch also does a small re-arrangement of the code, by moving > lspcon mode change into probe function. > > V2: Use dp->desc for OUI detection, dont add a helper for this > (Ville) > V3: Rebase, Added r-b from Maarten > V4: Rebase > V5: Rebase > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 6 ++++ > drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++-------- > 2 files changed, 61 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 87516b8..0b46f09 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1137,9 +1137,15 @@ struct intel_dp { > struct intel_dp_compliance compliance; > }; > > +enum lspcon_vendor { > + LSPCON_VENDOR_MCA, > + LSPCON_VENDOR_PARADE > +}; > + > struct intel_lspcon { > bool active; > enum drm_lspcon_mode mode; > + enum lspcon_vendor vendor; > }; > > struct intel_digital_port { > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > index 8ae8f42..40929c2 100644 > --- a/drivers/gpu/drm/i915/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > @@ -27,6 +27,10 @@ > #include <drm/drm_dp_dual_mode_helper.h> > #include "intel_drv.h" > > +/* LSPCON OUI Vendor ID(signatures) */ > +#define LSPCON_VENDOR_PARADE_OUI 0x001CF8 > +#define LSPCON_VENDOR_MCA_OUI 0x0060AD > + > static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon) > { > struct intel_digital_port *dig_port = > @@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode) > } > } > > +static bool lspcon_detect_vendor(struct intel_lspcon *lspcon) > +{ > + struct intel_dp *dp = lspcon_to_intel_dp(lspcon); > + struct drm_dp_dpcd_ident *ident; > + u32 vendor_oui; > + > + if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) { > + DRM_ERROR("Can't read description\n"); > + return false; > + } > + > + ident = &dp->desc.ident; > + vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) | > + ident->oui[2]; > + > + switch (vendor_oui) { > + case LSPCON_VENDOR_MCA_OUI: > + lspcon->vendor = LSPCON_VENDOR_MCA; > + DRM_DEBUG_KMS("Vendor: Mega Chips\n"); > + break; > + > + case LSPCON_VENDOR_PARADE_OUI: > + lspcon->vendor = LSPCON_VENDOR_PARADE; > + DRM_DEBUG_KMS("Vendor: Parade Tech\n"); > + break; > + > + default: > + DRM_ERROR("Invalid/Unknown vendor OUI\n"); > + return false; Sorry to jump in at such a late stage of review, but I'm a bit uneasy about this. I wonder if we should just default to, say, MCA behaviour when we don't identify the chip. Or skip the vendor specific parts. Just generally limp along as much as we can instead of detecting some sort of LSPCON but failing to use it because we can't identify it. BR, Jani. > + } > + > + return true; > +} > + > static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) > { > enum drm_lspcon_mode current_mode; > @@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon) > /* Yay ... got a LSPCON device */ > DRM_DEBUG_KMS("LSPCON detected\n"); > lspcon->mode = lspcon_wait_mode(lspcon, expected_mode); > - lspcon->active = true; > + > + /* > + * 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 != DRM_LSPCON_MODE_PCON) { > + if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { > + DRM_ERROR("LSPCON mode change to PCON failed\n"); > + return false; > + } > + } > return true; > } > > @@ -230,25 +279,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > 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 != DRM_LSPCON_MODE_PCON) { > - if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { > - DRM_ERROR("LSPCON mode change to PCON failed\n"); > - return false; > - } > - } > - > if (!intel_dp_read_dpcd(dp)) { > DRM_ERROR("LSPCON DPCD read failed\n"); > return false; > } > > - drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd)); > + if (!lspcon_detect_vendor(lspcon)) { > + DRM_ERROR("LSPCON vendor detection failed\n"); > + return false; > + } > > + lspcon->active = true; > DRM_DEBUG_KMS("Success: LSPCON init\n"); > return true; > }
Hello Jani, Thanks for the review comments, mine inline. Regards Shashank On 2/7/2018 4:49 PM, Jani Nikula wrote: > On Wed, 07 Feb 2018, Shashank Sharma <shashank.sharma@intel.com> wrote: >> From: "Sharma, Shashank" <shashank.sharma@intel.com> >> >> Intel LSPCON chip is provided by 2 vendors: >> - Megachips America (MCA) >> - Parade technologies (Parade tech) >> >> Its important to know the vendor of this chip, as the address to >> write AVI infoframes is different for those two. >> >> This patch reads the vendor OUI signature, and marks into LSPCON >> encoder structure for future usages. >> >> This patch also does a small re-arrangement of the code, by moving >> lspcon mode change into probe function. >> >> V2: Use dp->desc for OUI detection, dont add a helper for this >> (Ville) >> V3: Rebase, Added r-b from Maarten >> V4: Rebase >> V5: Rebase >> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Reviewed-by: Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_drv.h | 6 ++++ >> drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++-------- >> 2 files changed, 61 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 87516b8..0b46f09 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1137,9 +1137,15 @@ struct intel_dp { >> struct intel_dp_compliance compliance; >> }; >> >> +enum lspcon_vendor { >> + LSPCON_VENDOR_MCA, >> + LSPCON_VENDOR_PARADE >> +}; >> + >> struct intel_lspcon { >> bool active; >> enum drm_lspcon_mode mode; >> + enum lspcon_vendor vendor; >> }; >> >> struct intel_digital_port { >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >> index 8ae8f42..40929c2 100644 >> --- a/drivers/gpu/drm/i915/intel_lspcon.c >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >> @@ -27,6 +27,10 @@ >> #include <drm/drm_dp_dual_mode_helper.h> >> #include "intel_drv.h" >> >> +/* LSPCON OUI Vendor ID(signatures) */ >> +#define LSPCON_VENDOR_PARADE_OUI 0x001CF8 >> +#define LSPCON_VENDOR_MCA_OUI 0x0060AD >> + >> static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon) >> { >> struct intel_digital_port *dig_port = >> @@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode) >> } >> } >> >> +static bool lspcon_detect_vendor(struct intel_lspcon *lspcon) >> +{ >> + struct intel_dp *dp = lspcon_to_intel_dp(lspcon); >> + struct drm_dp_dpcd_ident *ident; >> + u32 vendor_oui; >> + >> + if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) { >> + DRM_ERROR("Can't read description\n"); >> + return false; >> + } >> + >> + ident = &dp->desc.ident; >> + vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) | >> + ident->oui[2]; >> + >> + switch (vendor_oui) { >> + case LSPCON_VENDOR_MCA_OUI: >> + lspcon->vendor = LSPCON_VENDOR_MCA; >> + DRM_DEBUG_KMS("Vendor: Mega Chips\n"); >> + break; >> + >> + case LSPCON_VENDOR_PARADE_OUI: >> + lspcon->vendor = LSPCON_VENDOR_PARADE; >> + DRM_DEBUG_KMS("Vendor: Parade Tech\n"); >> + break; >> + >> + default: >> + DRM_ERROR("Invalid/Unknown vendor OUI\n"); >> + return false; > Sorry to jump in at such a late stage of review, but I'm a bit uneasy > about this. I wonder if we should just default to, say, MCA behaviour > when we don't identify the chip. Or skip the vendor specific parts. > > Just generally limp along as much as we can instead of detecting some > sort of LSPCON but failing to use it because we can't identify it. On the first thought, I really liked this idea, that probably we can drive minimum stuff assuming a MCA lspcon, instead of failing it altogether, But It might be difficult what to define as minimum features. We can surely not drive YCBCR outputs or HDR stuff, as anything which is dependent on AVI infoframes, becomes vendor specific. Also there are few type-2 dongles which register themselves as generic dp-hdmi adapters, and they do not implement the stuff as per the spec. If we drive them forward, we never know where are we going to fail, and that might be difficult to debug at that stage. I remember debugging of one such type-1 HDMI jitter handler adapter with a Lenovo laptop, which was causing chaos as it dint implement the stuff properly. But at the same time I was thinking can we drive them with a warning, or debug message which says we are trying our best but this device is non-reliable .... you think so ? - Shashank > BR, > Jani. > > >> + } >> + >> + return true; >> +} >> + >> static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) >> { >> enum drm_lspcon_mode current_mode; >> @@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon) >> /* Yay ... got a LSPCON device */ >> DRM_DEBUG_KMS("LSPCON detected\n"); >> lspcon->mode = lspcon_wait_mode(lspcon, expected_mode); >> - lspcon->active = true; >> + >> + /* >> + * 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 != DRM_LSPCON_MODE_PCON) { >> + if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { >> + DRM_ERROR("LSPCON mode change to PCON failed\n"); >> + return false; >> + } >> + } >> return true; >> } >> >> @@ -230,25 +279,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >> 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 != DRM_LSPCON_MODE_PCON) { >> - if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { >> - DRM_ERROR("LSPCON mode change to PCON failed\n"); >> - return false; >> - } >> - } >> - >> if (!intel_dp_read_dpcd(dp)) { >> DRM_ERROR("LSPCON DPCD read failed\n"); >> return false; >> } >> >> - drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd)); >> + if (!lspcon_detect_vendor(lspcon)) { >> + DRM_ERROR("LSPCON vendor detection failed\n"); >> + return false; >> + } >> >> + lspcon->active = true; >> DRM_DEBUG_KMS("Success: LSPCON init\n"); >> return true; >> }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 87516b8..0b46f09 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1137,9 +1137,15 @@ struct intel_dp { struct intel_dp_compliance compliance; }; +enum lspcon_vendor { + LSPCON_VENDOR_MCA, + LSPCON_VENDOR_PARADE +}; + struct intel_lspcon { bool active; enum drm_lspcon_mode mode; + enum lspcon_vendor vendor; }; struct intel_digital_port { diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c index 8ae8f42..40929c2 100644 --- a/drivers/gpu/drm/i915/intel_lspcon.c +++ b/drivers/gpu/drm/i915/intel_lspcon.c @@ -27,6 +27,10 @@ #include <drm/drm_dp_dual_mode_helper.h> #include "intel_drv.h" +/* LSPCON OUI Vendor ID(signatures) */ +#define LSPCON_VENDOR_PARADE_OUI 0x001CF8 +#define LSPCON_VENDOR_MCA_OUI 0x0060AD + static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon) { struct intel_digital_port *dig_port = @@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode) } } +static bool lspcon_detect_vendor(struct intel_lspcon *lspcon) +{ + struct intel_dp *dp = lspcon_to_intel_dp(lspcon); + struct drm_dp_dpcd_ident *ident; + u32 vendor_oui; + + if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) { + DRM_ERROR("Can't read description\n"); + return false; + } + + ident = &dp->desc.ident; + vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) | + ident->oui[2]; + + switch (vendor_oui) { + case LSPCON_VENDOR_MCA_OUI: + lspcon->vendor = LSPCON_VENDOR_MCA; + DRM_DEBUG_KMS("Vendor: Mega Chips\n"); + break; + + case LSPCON_VENDOR_PARADE_OUI: + lspcon->vendor = LSPCON_VENDOR_PARADE; + DRM_DEBUG_KMS("Vendor: Parade Tech\n"); + break; + + default: + DRM_ERROR("Invalid/Unknown vendor OUI\n"); + return false; + } + + return true; +} + static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) { enum drm_lspcon_mode current_mode; @@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon) /* Yay ... got a LSPCON device */ DRM_DEBUG_KMS("LSPCON detected\n"); lspcon->mode = lspcon_wait_mode(lspcon, expected_mode); - lspcon->active = true; + + /* + * 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 != DRM_LSPCON_MODE_PCON) { + if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { + DRM_ERROR("LSPCON mode change to PCON failed\n"); + return false; + } + } return true; } @@ -230,25 +279,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) 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 != DRM_LSPCON_MODE_PCON) { - if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) { - DRM_ERROR("LSPCON mode change to PCON failed\n"); - return false; - } - } - if (!intel_dp_read_dpcd(dp)) { DRM_ERROR("LSPCON DPCD read failed\n"); return false; } - drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd)); + if (!lspcon_detect_vendor(lspcon)) { + DRM_ERROR("LSPCON vendor detection failed\n"); + return false; + } + lspcon->active = true; DRM_DEBUG_KMS("Success: LSPCON init\n"); return true; }