Re: [PATCH] drm/amdgpu/display: add support for LVDS (v5)
On 2018-08-21 03:45 PM, Alex Deucher wrote:
> On Tue, Aug 21, 2018 at 3:36 PM Harry Wentland wrote:
>>
>>
>>
>> On 2018-08-16 09:31 AM, Alex Deucher wrote:
>>> This adds support for LVDS displays.
>>>
>>> v2: add support for spread spectrum, sink detect
>>> v3: clean up enable_lvds_output
>>> v4: fix up link_detect
>>> v5: remove assert on 888 format
>>>
>>> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=105880
>>> Signed-off-by: Alex Deucher
>>> ---
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +
>>> drivers/gpu/drm/amd/display/dc/core/dc_link.c | 45
>>> ++
>>> .../gpu/drm/amd/display/dc/dce/dce_clock_source.c | 10 +
>>> .../gpu/drm/amd/display/dc/dce/dce_clock_source.h | 2 +
>>> .../gpu/drm/amd/display/dc/dce/dce_link_encoder.c | 34
>>> .../gpu/drm/amd/display/dc/dce/dce_link_encoder.h | 6 +++
>>> .../drm/amd/display/dc/dce/dce_stream_encoder.c| 24
>>> .../gpu/drm/amd/display/dc/inc/hw/link_encoder.h | 3 ++
>>> .../gpu/drm/amd/display/dc/inc/hw/stream_encoder.h | 4 ++
>>> drivers/gpu/drm/amd/display/include/signal_types.h | 5 +++
>>> 10 files changed, 135 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 1828e4382b24..818b5ec32f37 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -3360,6 +3360,8 @@ static int to_drm_connector_type(enum signal_type st)
>>> return DRM_MODE_CONNECTOR_HDMIA;
>>> case SIGNAL_TYPE_EDP:
>>> return DRM_MODE_CONNECTOR_eDP;
>>> + case SIGNAL_TYPE_LVDS:
>>> + return DRM_MODE_CONNECTOR_LVDS;
>>> case SIGNAL_TYPE_RGB:
>>> return DRM_MODE_CONNECTOR_VGA;
>>> case SIGNAL_TYPE_DISPLAY_PORT:
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> index 981f7cbd31cc..0f044fd5baf4 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> @@ -203,6 +203,11 @@ static bool detect_sink(struct dc_link *link, enum
>>> dc_connection_type *type)
>>> uint32_t is_hpd_high = 0;
>>> struct gpio *hpd_pin;
>>>
>>> + if (link->connector_signal == SIGNAL_TYPE_LVDS) {
>>> + *type = dc_connection_single;
>>> + return true;
>>> + }
>>> +
>>
>> Would it be better to still do HPD detection? Or is this failing here?
>
> It fails there. I don't think LVDS normally has a HPD pin assigned.
> There's no hotplug per se.
>
Ah, I thought so. We can take care of this if it ever becomes an issue. It
looks like DAL2 code still checked HPD but I only had a cursory look.
>>
>>> /* todo: may need to lock gpio access */
>>> hpd_pin = get_hpd_gpio(link->ctx->dc_bios, link->link_id,
>>> link->ctx->gpio_service);
>>> if (hpd_pin == NULL)
>>> @@ -616,6 +621,10 @@ bool dc_link_detect(struct dc_link *link, enum
>>> dc_detect_reason reason)
>>> link->local_sink)
>>> return true;
>>>
>>> + if (link->connector_signal == SIGNAL_TYPE_LVDS &&
>>> + link->local_sink)
>>> + return true;
>>> +
>>> prev_sink = link->local_sink;
>>> if (prev_sink != NULL) {
>>> dc_sink_retain(prev_sink);
>>> @@ -649,6 +658,12 @@ bool dc_link_detect(struct dc_link *link, enum
>>> dc_detect_reason reason)
>>> break;
>>> }
>>>
>>> + case SIGNAL_TYPE_LVDS: {
>>> + sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C;
>>> + sink_caps.signal = SIGNAL_TYPE_LVDS;
>>> + break;
>>> + }
>>> +
>>> case SIGNAL_TYPE_EDP: {
>>> detect_edp_sink_caps(link);
>>> sink_caps.transaction_type =
>>> @@ -1087,6 +1102,9 @@ static bool construct(
>>> dal_irq_get_rx_source(hpd_gpio);
>>> }
>>> break;
>>> + case CONNECTOR_ID_LVDS:
>>> + link->connector_signal = SIGNAL_TYPE_LVDS;
>>> + break;
>>> default:
>>> DC_LOG_WARNING("Unsupported Connector type:%d!\n",
>>> link->link_id.id);
>>> goto create_fail;
>>> @@ -1920,6 +1938,24 @@ static void enable_link_hdmi(struct pipe_ctx
>>> *pipe_ctx)
>>> dal_ddc_service_read_scdc_data(link->ddc);
>>> }
>>>
>>> +static void enable_link_lvds(struct pipe_ctx *pipe_ctx)
>>> +{
>>> + struct dc_stream_state *stream = pipe_ctx->stream;
>>> + struct dc_link *link = stream->sink->link;
>>> +
>>> + if (stream->phy_pix_clk == 0)
>>> + stream->phy_pix_clk = stream->timing.pix_clk_khz;
>>> +
>>> + memset(>sink->link->cur_link_settings, 0,
>>> +
Re: [PATCH] drm/amdgpu/display: add support for LVDS (v5)
On Tue, Aug 21, 2018 at 3:36 PM Harry Wentland wrote:
>
>
>
> On 2018-08-16 09:31 AM, Alex Deucher wrote:
> > This adds support for LVDS displays.
> >
> > v2: add support for spread spectrum, sink detect
> > v3: clean up enable_lvds_output
> > v4: fix up link_detect
> > v5: remove assert on 888 format
> >
> > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=105880
> > Signed-off-by: Alex Deucher
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +
> > drivers/gpu/drm/amd/display/dc/core/dc_link.c | 45
> > ++
> > .../gpu/drm/amd/display/dc/dce/dce_clock_source.c | 10 +
> > .../gpu/drm/amd/display/dc/dce/dce_clock_source.h | 2 +
> > .../gpu/drm/amd/display/dc/dce/dce_link_encoder.c | 34
> > .../gpu/drm/amd/display/dc/dce/dce_link_encoder.h | 6 +++
> > .../drm/amd/display/dc/dce/dce_stream_encoder.c| 24
> > .../gpu/drm/amd/display/dc/inc/hw/link_encoder.h | 3 ++
> > .../gpu/drm/amd/display/dc/inc/hw/stream_encoder.h | 4 ++
> > drivers/gpu/drm/amd/display/include/signal_types.h | 5 +++
> > 10 files changed, 135 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 1828e4382b24..818b5ec32f37 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3360,6 +3360,8 @@ static int to_drm_connector_type(enum signal_type st)
> > return DRM_MODE_CONNECTOR_HDMIA;
> > case SIGNAL_TYPE_EDP:
> > return DRM_MODE_CONNECTOR_eDP;
> > + case SIGNAL_TYPE_LVDS:
> > + return DRM_MODE_CONNECTOR_LVDS;
> > case SIGNAL_TYPE_RGB:
> > return DRM_MODE_CONNECTOR_VGA;
> > case SIGNAL_TYPE_DISPLAY_PORT:
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > index 981f7cbd31cc..0f044fd5baf4 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > @@ -203,6 +203,11 @@ static bool detect_sink(struct dc_link *link, enum
> > dc_connection_type *type)
> > uint32_t is_hpd_high = 0;
> > struct gpio *hpd_pin;
> >
> > + if (link->connector_signal == SIGNAL_TYPE_LVDS) {
> > + *type = dc_connection_single;
> > + return true;
> > + }
> > +
>
> Would it be better to still do HPD detection? Or is this failing here?
It fails there. I don't think LVDS normally has a HPD pin assigned.
There's no hotplug per se.
>
> > /* todo: may need to lock gpio access */
> > hpd_pin = get_hpd_gpio(link->ctx->dc_bios, link->link_id,
> > link->ctx->gpio_service);
> > if (hpd_pin == NULL)
> > @@ -616,6 +621,10 @@ bool dc_link_detect(struct dc_link *link, enum
> > dc_detect_reason reason)
> > link->local_sink)
> > return true;
> >
> > + if (link->connector_signal == SIGNAL_TYPE_LVDS &&
> > + link->local_sink)
> > + return true;
> > +
> > prev_sink = link->local_sink;
> > if (prev_sink != NULL) {
> > dc_sink_retain(prev_sink);
> > @@ -649,6 +658,12 @@ bool dc_link_detect(struct dc_link *link, enum
> > dc_detect_reason reason)
> > break;
> > }
> >
> > + case SIGNAL_TYPE_LVDS: {
> > + sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C;
> > + sink_caps.signal = SIGNAL_TYPE_LVDS;
> > + break;
> > + }
> > +
> > case SIGNAL_TYPE_EDP: {
> > detect_edp_sink_caps(link);
> > sink_caps.transaction_type =
> > @@ -1087,6 +1102,9 @@ static bool construct(
> > dal_irq_get_rx_source(hpd_gpio);
> > }
> > break;
> > + case CONNECTOR_ID_LVDS:
> > + link->connector_signal = SIGNAL_TYPE_LVDS;
> > + break;
> > default:
> > DC_LOG_WARNING("Unsupported Connector type:%d!\n",
> > link->link_id.id);
> > goto create_fail;
> > @@ -1920,6 +1938,24 @@ static void enable_link_hdmi(struct pipe_ctx
> > *pipe_ctx)
> > dal_ddc_service_read_scdc_data(link->ddc);
> > }
> >
> > +static void enable_link_lvds(struct pipe_ctx *pipe_ctx)
> > +{
> > + struct dc_stream_state *stream = pipe_ctx->stream;
> > + struct dc_link *link = stream->sink->link;
> > +
> > + if (stream->phy_pix_clk == 0)
> > + stream->phy_pix_clk = stream->timing.pix_clk_khz;
> > +
> > + memset(>sink->link->cur_link_settings, 0,
> > + sizeof(struct dc_link_settings));
> > +
>
> The cur_link_settings. should only by used by eDP/DP/MST. Shouldn't need to
> touch them here.
Ok. enable_link_hdmi() clears that structure as well, so I did it
Re: [PATCH] drm/amdgpu/display: add support for LVDS (v5)
On 2018-08-16 09:31 AM, Alex Deucher wrote:
> This adds support for LVDS displays.
>
> v2: add support for spread spectrum, sink detect
> v3: clean up enable_lvds_output
> v4: fix up link_detect
> v5: remove assert on 888 format
>
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=105880
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +
> drivers/gpu/drm/amd/display/dc/core/dc_link.c | 45
> ++
> .../gpu/drm/amd/display/dc/dce/dce_clock_source.c | 10 +
> .../gpu/drm/amd/display/dc/dce/dce_clock_source.h | 2 +
> .../gpu/drm/amd/display/dc/dce/dce_link_encoder.c | 34
> .../gpu/drm/amd/display/dc/dce/dce_link_encoder.h | 6 +++
> .../drm/amd/display/dc/dce/dce_stream_encoder.c| 24
> .../gpu/drm/amd/display/dc/inc/hw/link_encoder.h | 3 ++
> .../gpu/drm/amd/display/dc/inc/hw/stream_encoder.h | 4 ++
> drivers/gpu/drm/amd/display/include/signal_types.h | 5 +++
> 10 files changed, 135 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1828e4382b24..818b5ec32f37 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3360,6 +3360,8 @@ static int to_drm_connector_type(enum signal_type st)
> return DRM_MODE_CONNECTOR_HDMIA;
> case SIGNAL_TYPE_EDP:
> return DRM_MODE_CONNECTOR_eDP;
> + case SIGNAL_TYPE_LVDS:
> + return DRM_MODE_CONNECTOR_LVDS;
> case SIGNAL_TYPE_RGB:
> return DRM_MODE_CONNECTOR_VGA;
> case SIGNAL_TYPE_DISPLAY_PORT:
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index 981f7cbd31cc..0f044fd5baf4 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -203,6 +203,11 @@ static bool detect_sink(struct dc_link *link, enum
> dc_connection_type *type)
> uint32_t is_hpd_high = 0;
> struct gpio *hpd_pin;
>
> + if (link->connector_signal == SIGNAL_TYPE_LVDS) {
> + *type = dc_connection_single;
> + return true;
> + }
> +
Would it be better to still do HPD detection? Or is this failing here?
> /* todo: may need to lock gpio access */
> hpd_pin = get_hpd_gpio(link->ctx->dc_bios, link->link_id,
> link->ctx->gpio_service);
> if (hpd_pin == NULL)
> @@ -616,6 +621,10 @@ bool dc_link_detect(struct dc_link *link, enum
> dc_detect_reason reason)
> link->local_sink)
> return true;
>
> + if (link->connector_signal == SIGNAL_TYPE_LVDS &&
> + link->local_sink)
> + return true;
> +
> prev_sink = link->local_sink;
> if (prev_sink != NULL) {
> dc_sink_retain(prev_sink);
> @@ -649,6 +658,12 @@ bool dc_link_detect(struct dc_link *link, enum
> dc_detect_reason reason)
> break;
> }
>
> + case SIGNAL_TYPE_LVDS: {
> + sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C;
> + sink_caps.signal = SIGNAL_TYPE_LVDS;
> + break;
> + }
> +
> case SIGNAL_TYPE_EDP: {
> detect_edp_sink_caps(link);
> sink_caps.transaction_type =
> @@ -1087,6 +1102,9 @@ static bool construct(
> dal_irq_get_rx_source(hpd_gpio);
> }
> break;
> + case CONNECTOR_ID_LVDS:
> + link->connector_signal = SIGNAL_TYPE_LVDS;
> + break;
> default:
> DC_LOG_WARNING("Unsupported Connector type:%d!\n",
> link->link_id.id);
> goto create_fail;
> @@ -1920,6 +1938,24 @@ static void enable_link_hdmi(struct pipe_ctx *pipe_ctx)
> dal_ddc_service_read_scdc_data(link->ddc);
> }
>
> +static void enable_link_lvds(struct pipe_ctx *pipe_ctx)
> +{
> + struct dc_stream_state *stream = pipe_ctx->stream;
> + struct dc_link *link = stream->sink->link;
> +
> + if (stream->phy_pix_clk == 0)
> + stream->phy_pix_clk = stream->timing.pix_clk_khz;
> +
> + memset(>sink->link->cur_link_settings, 0,
> + sizeof(struct dc_link_settings));
> +
The cur_link_settings. should only by used by eDP/DP/MST. Shouldn't need to
touch them here.
Otherwise the change looks good.
Harry
> + link->link_enc->funcs->enable_lvds_output(
> + link->link_enc,
> + pipe_ctx->clock_source->id,
> + stream->phy_pix_clk);
> +
> +}
> +
> /enable_link***/
> static enum dc_status enable_link(
> struct dc_state *state,
> @@ -1943,6 +1979,10 @@ static enum
[PATCH] drm/amdgpu/display: add support for LVDS (v5)
This adds support for LVDS displays.
v2: add support for spread spectrum, sink detect
v3: clean up enable_lvds_output
v4: fix up link_detect
v5: remove assert on 888 format
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=105880
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +
drivers/gpu/drm/amd/display/dc/core/dc_link.c | 45 ++
.../gpu/drm/amd/display/dc/dce/dce_clock_source.c | 10 +
.../gpu/drm/amd/display/dc/dce/dce_clock_source.h | 2 +
.../gpu/drm/amd/display/dc/dce/dce_link_encoder.c | 34
.../gpu/drm/amd/display/dc/dce/dce_link_encoder.h | 6 +++
.../drm/amd/display/dc/dce/dce_stream_encoder.c| 24
.../gpu/drm/amd/display/dc/inc/hw/link_encoder.h | 3 ++
.../gpu/drm/amd/display/dc/inc/hw/stream_encoder.h | 4 ++
drivers/gpu/drm/amd/display/include/signal_types.h | 5 +++
10 files changed, 135 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1828e4382b24..818b5ec32f37 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3360,6 +3360,8 @@ static int to_drm_connector_type(enum signal_type st)
return DRM_MODE_CONNECTOR_HDMIA;
case SIGNAL_TYPE_EDP:
return DRM_MODE_CONNECTOR_eDP;
+ case SIGNAL_TYPE_LVDS:
+ return DRM_MODE_CONNECTOR_LVDS;
case SIGNAL_TYPE_RGB:
return DRM_MODE_CONNECTOR_VGA;
case SIGNAL_TYPE_DISPLAY_PORT:
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 981f7cbd31cc..0f044fd5baf4 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -203,6 +203,11 @@ static bool detect_sink(struct dc_link *link, enum
dc_connection_type *type)
uint32_t is_hpd_high = 0;
struct gpio *hpd_pin;
+ if (link->connector_signal == SIGNAL_TYPE_LVDS) {
+ *type = dc_connection_single;
+ return true;
+ }
+
/* todo: may need to lock gpio access */
hpd_pin = get_hpd_gpio(link->ctx->dc_bios, link->link_id,
link->ctx->gpio_service);
if (hpd_pin == NULL)
@@ -616,6 +621,10 @@ bool dc_link_detect(struct dc_link *link, enum
dc_detect_reason reason)
link->local_sink)
return true;
+ if (link->connector_signal == SIGNAL_TYPE_LVDS &&
+ link->local_sink)
+ return true;
+
prev_sink = link->local_sink;
if (prev_sink != NULL) {
dc_sink_retain(prev_sink);
@@ -649,6 +658,12 @@ bool dc_link_detect(struct dc_link *link, enum
dc_detect_reason reason)
break;
}
+ case SIGNAL_TYPE_LVDS: {
+ sink_caps.transaction_type = DDC_TRANSACTION_TYPE_I2C;
+ sink_caps.signal = SIGNAL_TYPE_LVDS;
+ break;
+ }
+
case SIGNAL_TYPE_EDP: {
detect_edp_sink_caps(link);
sink_caps.transaction_type =
@@ -1087,6 +1102,9 @@ static bool construct(
dal_irq_get_rx_source(hpd_gpio);
}
break;
+ case CONNECTOR_ID_LVDS:
+ link->connector_signal = SIGNAL_TYPE_LVDS;
+ break;
default:
DC_LOG_WARNING("Unsupported Connector type:%d!\n",
link->link_id.id);
goto create_fail;
@@ -1920,6 +1938,24 @@ static void enable_link_hdmi(struct pipe_ctx *pipe_ctx)
dal_ddc_service_read_scdc_data(link->ddc);
}
+static void enable_link_lvds(struct pipe_ctx *pipe_ctx)
+{
+ struct dc_stream_state *stream = pipe_ctx->stream;
+ struct dc_link *link = stream->sink->link;
+
+ if (stream->phy_pix_clk == 0)
+ stream->phy_pix_clk = stream->timing.pix_clk_khz;
+
+ memset(>sink->link->cur_link_settings, 0,
+ sizeof(struct dc_link_settings));
+
+ link->link_enc->funcs->enable_lvds_output(
+ link->link_enc,
+ pipe_ctx->clock_source->id,
+ stream->phy_pix_clk);
+
+}
+
/enable_link***/
static enum dc_status enable_link(
struct dc_state *state,
@@ -1943,6 +1979,10 @@ static enum dc_status enable_link(
enable_link_hdmi(pipe_ctx);
status = DC_OK;
break;
+ case SIGNAL_TYPE_LVDS:
+ enable_link_lvds(pipe_ctx);
+ status = DC_OK;
+ break;
case SIGNAL_TYPE_VIRTUAL:
status = DC_OK;
break;
@@ -2492,6 +2532,11 @@ void
