[PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Doug Anderson]

If you plug in a DVI monitor to your HDMI port, you need to filter out
clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
we can run single-link DVI at.

If you want to run high resolutions to DVI, you'd need some type of an
active adapter that pretended that it was HDMI, interpreted the
signal, and produced a new dual link DVI signal at a lower clock rate.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Note: this patch was tested against a 3.14 kernel with backports.  It
was only compile tested against linuxnext, but the code is
sufficiently similar that I'm convinced it will work there.

 drivers/gpu/drm/bridge/dw_hdmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 816d104..48b8532 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1421,8 +1421,12 @@ dw_hdmi_connector_mode_valid(struct drm_connector *connector,
 {
 	struct dw_hdmi *hdmi = container_of(connector,
 					   struct dw_hdmi, connector);
+	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
 	enum drm_mode_status mode_status = MODE_OK;
 
+	if (mode->clock > 165000 && vmode->mdvi)
+		return MODE_BAD;
+
 	if (hdmi->plat_data->mode_valid)
 		mode_status = hdmi->plat_data->mode_valid(connector, mode);
 
-- 
2.4.3.573.g4eafbef

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Wed, Jun 17, 2015 at 04:14:07PM -0700, Doug Anderson wrote:
> If you plug in a DVI monitor to your HDMI port, you need to filter out
> clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
> we can run single-link DVI at.
> 
> If you want to run high resolutions to DVI, you'd need some type of an
> active adapter that pretended that it was HDMI, interpreted the
> signal, and produced a new dual link DVI signal at a lower clock rate.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Note: this patch was tested against a 3.14 kernel with backports.  It
> was only compile tested against linuxnext, but the code is
> sufficiently similar that I'm convinced it will work there.

Really?  I have to wonder what your testing was...

        hdmi->vic = drm_match_cea_mode(mode);

        if (!hdmi->vic) {
                dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
                hdmi->hdmi_data.video_mode.mdvi = true;
        } else {
                dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
                hdmi->hdmi_data.video_mode.mdvi = false;
        }

mdvi indicates whether the _currently set mode_ is a CEA mode or not (imho,
it's mis-named).  It doesn't indicate whether we have a HDMI display device
or a DVI display device connected, which seems to be what you want to use
it for below.

To sort that, what you need to do is detect a HDMI display device using
drm_detect_hdmi_monitor() on the EDID received from the device before
parsing the modes, and save that value in a dw_hdmi struct member, and
I'd suggest that it's a top-level struct member, not buried in 'hdmi_data'
or 'video_mode'.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Wed, Jun 17, 2015 at 04:14:07PM -0700, Doug Anderson wrote:
> If you plug in a DVI monitor to your HDMI port, you need to filter out
> clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
> we can run single-link DVI at.
> 
> If you want to run high resolutions to DVI, you'd need some type of an
> active adapter that pretended that it was HDMI, interpreted the
> signal, and produced a new dual link DVI signal at a lower clock rate.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Note: this patch was tested against a 3.14 kernel with backports.  It
> was only compile tested against linuxnext, but the code is
> sufficiently similar that I'm convinced it will work there.

Really?  I have to wonder what your testing was...

        hdmi->vic = drm_match_cea_mode(mode);

        if (!hdmi->vic) {
                dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
                hdmi->hdmi_data.video_mode.mdvi = true;
        } else {
                dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
                hdmi->hdmi_data.video_mode.mdvi = false;
        }

mdvi indicates whether the _currently set mode_ is a CEA mode or not (imho,
it's mis-named).  It doesn't indicate whether we have a HDMI display device
or a DVI display device connected, which seems to be what you want to use
it for below.

To sort that, what you need to do is detect a HDMI display device using
drm_detect_hdmi_monitor() on the EDID received from the device before
parsing the modes, and save that value in a dw_hdmi struct member, and
I'd suggest that it's a top-level struct member, not buried in 'hdmi_data'
or 'video_mode'.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Doug Anderson]

Russell,

On Wed, Jun 17, 2015 at 4:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jun 17, 2015 at 04:14:07PM -0700, Doug Anderson wrote:
>> If you plug in a DVI monitor to your HDMI port, you need to filter out
>> clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
>> we can run single-link DVI at.
>>
>> If you want to run high resolutions to DVI, you'd need some type of an
>> active adapter that pretended that it was HDMI, interpreted the
>> signal, and produced a new dual link DVI signal at a lower clock rate.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>> Note: this patch was tested against a 3.14 kernel with backports.  It
>> was only compile tested against linuxnext, but the code is
>> sufficiently similar that I'm convinced it will work there.
>
> Really?  I have to wonder what your testing was...
>
>         hdmi->vic = drm_match_cea_mode(mode);
>
>         if (!hdmi->vic) {
>                 dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
>                 hdmi->hdmi_data.video_mode.mdvi = true;
>         } else {
>                 dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
>                 hdmi->hdmi_data.video_mode.mdvi = false;
>         }
>
> mdvi indicates whether the _currently set mode_ is a CEA mode or not (imho,
> it's mis-named).  It doesn't indicate whether we have a HDMI display device
> or a DVI display device connected, which seems to be what you want to use
> it for below.
>
> To sort that, what you need to do is detect a HDMI display device using
> drm_detect_hdmi_monitor() on the EDID received from the device before
> parsing the modes, and save that value in a dw_hdmi struct member, and
> I'd suggest that it's a top-level struct member, not buried in 'hdmi_data'
> or 'video_mode'.

OK, so clearly my patch won't work against mainline.  I guess it's a
good thing that I pointed out that it was only tested locally (would
have been better to test against mainline, but I don't think that's so
easy since there are several unlanded patches in mainline for
Rockchip).

As pointed out by others at <http://crosreview.com/278255>, locally
our kernel has a slightly older version of
<https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
as needed.

...so I guess my change is blocked on someone reviewing/landing that
series.  If that series is rejected (or is changed sufficiently so
that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
will need to be re-spun.

-Doug

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Wed, Jun 17, 2015 at 07:52:14PM -0700, Doug Anderson wrote:
> Russell,
> 
> On Wed, Jun 17, 2015 at 4:30 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Jun 17, 2015 at 04:14:07PM -0700, Doug Anderson wrote:
> >> If you plug in a DVI monitor to your HDMI port, you need to filter out
> >> clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
> >> we can run single-link DVI at.
> >>
> >> If you want to run high resolutions to DVI, you'd need some type of an
> >> active adapter that pretended that it was HDMI, interpreted the
> >> signal, and produced a new dual link DVI signal at a lower clock rate.
> >>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >> ---
> >> Note: this patch was tested against a 3.14 kernel with backports.  It
> >> was only compile tested against linuxnext, but the code is
> >> sufficiently similar that I'm convinced it will work there.
> >
> > Really?  I have to wonder what your testing was...
> >
> >         hdmi->vic = drm_match_cea_mode(mode);
> >
> >         if (!hdmi->vic) {
> >                 dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
> >                 hdmi->hdmi_data.video_mode.mdvi = true;
> >         } else {
> >                 dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
> >                 hdmi->hdmi_data.video_mode.mdvi = false;
> >         }
> >
> > mdvi indicates whether the _currently set mode_ is a CEA mode or not (imho,
> > it's mis-named).  It doesn't indicate whether we have a HDMI display device
> > or a DVI display device connected, which seems to be what you want to use
> > it for below.
> >
> > To sort that, what you need to do is detect a HDMI display device using
> > drm_detect_hdmi_monitor() on the EDID received from the device before
> > parsing the modes, and save that value in a dw_hdmi struct member, and
> > I'd suggest that it's a top-level struct member, not buried in 'hdmi_data'
> > or 'video_mode'.
> 
> OK, so clearly my patch won't work against mainline.  I guess it's a
> good thing that I pointed out that it was only tested locally (would
> have been better to test against mainline, but I don't think that's so
> easy since there are several unlanded patches in mainline for
> Rockchip).

As far as I'm aware, Freescale's original BSP version was the same, as is
their later BSPs, and Jon's maintained 3.14-stable kernel.

> As pointed out by others at <http://crosreview.com/278255>, locally
> our kernel has a slightly older version of
> <https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
> as needed.

Please don't post unreliable lkml.org URLs, please use some other archive
site.  I can't access this URL at the moment.

> ...so I guess my change is blocked on someone reviewing/landing that
> series.  If that series is rejected (or is changed sufficiently so
> that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
> will need to be re-spun.

That's not what I said.  I said mdvi is set according to whether the mode
being set is a CEA mode or not.  We need something set according to
the return value of drm_detect_hdmi_monitor(), which will tell us if the
connected sink is a HDMI device or a DVI device (based upon the EDID.)

A thought occurs to me this morning though: what happens if you connect
a DVI monitor to an AV receiver which is then connected to this device.
Does the resulting EDID contain the HDMI vendor ID?  If it does, it
means that drm_detect_hdmi_monitor() will return true, indicating that
the connected device is HDMI, and we will still allow modes greater than
165MHz.

That's probably a scenario that should be checked at some point... and
it would throw a question mark over whether this is the correct approach
to limit the video modes.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Wed, Jun 17, 2015 at 07:52:14PM -0700, Doug Anderson wrote:
> Russell,
> 
> On Wed, Jun 17, 2015 at 4:30 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Jun 17, 2015 at 04:14:07PM -0700, Doug Anderson wrote:
> >> If you plug in a DVI monitor to your HDMI port, you need to filter out
> >> clocks > 165MHz.  That's because 165MHz is the maximum clock rate that
> >> we can run single-link DVI at.
> >>
> >> If you want to run high resolutions to DVI, you'd need some type of an
> >> active adapter that pretended that it was HDMI, interpreted the
> >> signal, and produced a new dual link DVI signal at a lower clock rate.
> >>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >> ---
> >> Note: this patch was tested against a 3.14 kernel with backports.  It
> >> was only compile tested against linuxnext, but the code is
> >> sufficiently similar that I'm convinced it will work there.
> >
> > Really?  I have to wonder what your testing was...
> >
> >         hdmi->vic = drm_match_cea_mode(mode);
> >
> >         if (!hdmi->vic) {
> >                 dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
> >                 hdmi->hdmi_data.video_mode.mdvi = true;
> >         } else {
> >                 dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
> >                 hdmi->hdmi_data.video_mode.mdvi = false;
> >         }
> >
> > mdvi indicates whether the _currently set mode_ is a CEA mode or not (imho,
> > it's mis-named).  It doesn't indicate whether we have a HDMI display device
> > or a DVI display device connected, which seems to be what you want to use
> > it for below.
> >
> > To sort that, what you need to do is detect a HDMI display device using
> > drm_detect_hdmi_monitor() on the EDID received from the device before
> > parsing the modes, and save that value in a dw_hdmi struct member, and
> > I'd suggest that it's a top-level struct member, not buried in 'hdmi_data'
> > or 'video_mode'.
> 
> OK, so clearly my patch won't work against mainline.  I guess it's a
> good thing that I pointed out that it was only tested locally (would
> have been better to test against mainline, but I don't think that's so
> easy since there are several unlanded patches in mainline for
> Rockchip).

As far as I'm aware, Freescale's original BSP version was the same, as is
their later BSPs, and Jon's maintained 3.14-stable kernel.

> As pointed out by others at <http://crosreview.com/278255>, locally
> our kernel has a slightly older version of
> <https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
> as needed.

Please don't post unreliable lkml.org URLs, please use some other archive
site.  I can't access this URL at the moment.

> ...so I guess my change is blocked on someone reviewing/landing that
> series.  If that series is rejected (or is changed sufficiently so
> that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
> will need to be re-spun.

That's not what I said.  I said mdvi is set according to whether the mode
being set is a CEA mode or not.  We need something set according to
the return value of drm_detect_hdmi_monitor(), which will tell us if the
connected sink is a HDMI device or a DVI device (based upon the EDID.)

A thought occurs to me this morning though: what happens if you connect
a DVI monitor to an AV receiver which is then connected to this device.
Does the resulting EDID contain the HDMI vendor ID?  If it does, it
means that drm_detect_hdmi_monitor() will return true, indicating that
the connected device is HDMI, and we will still allow modes greater than
165MHz.

That's probably a scenario that should be checked at some point... and
it would throw a question mark over whether this is the correct approach
to limit the video modes.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Doug Anderson]

Russell,

On Thu, Jun 18, 2015 at 1:53 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> OK, so clearly my patch won't work against mainline.  I guess it's a
>> good thing that I pointed out that it was only tested locally (would
>> have been better to test against mainline, but I don't think that's so
>> easy since there are several unlanded patches in mainline for
>> Rockchip).
>
> As far as I'm aware, Freescale's original BSP version was the same, as is
> their later BSPs, and Jon's maintained 3.14-stable kernel.

Was "the same"?  You mean was untested, or was 3.14?  It is probably
not the same "3.14 with backports" that I'm testing on, which includes
backports + things from the mailing list that haven't landed yet, as
many of the unlanded patches are things that make Rockchip HDMI work
more correctly.  Perhaps I should have called my tree "3.14 with
backports + unlanded patches" or "the chromeos 3.14 tree" to make it
clearer.

In general I haven't been posting patches that I've made to HDMI since
it appears that our tree has significant differences from mainline.
In this case the function I was touching looked identical to mainline
so I figured posting a patch seemed reasonable.


>> As pointed out by others at <http://crosreview.com/278255>, locally
>> our kernel has a slightly older version of
>> <https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
>> as needed.
>
> Please don't post unreliable lkml.org URLs, please use some other archive
> site.  I can't access this URL at the moment.

Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>


>> ...so I guess my change is blocked on someone reviewing/landing that
>> series.  If that series is rejected (or is changed sufficiently so
>> that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
>> will need to be re-spun.
>
> That's not what I said.  I said mdvi is set according to whether the mode
> being set is a CEA mode or not.

Perhaps now that you can access the patch with the patchwork link you
can re-read my email.  If/when that patch lands then mdvi _will_ be
set as per drm_detect_hdmi_monitor().


> A thought occurs to me this morning though: what happens if you connect
> a DVI monitor to an AV receiver which is then connected to this device.
> Does the resulting EDID contain the HDMI vendor ID?  If it does, it
> means that drm_detect_hdmi_monitor() will return true, indicating that
> the connected device is HDMI, and we will still allow modes greater than
> 165MHz.

I am nowhere near an HDMI expert.  If you have a better suggestion
then I'm more than happy for you to post it and drop my patch.  In my
non-expert opinion, it would seem awfully strange for an AV receiver
to modify the EDID though unless it was actively interpreting the
signal and generating a whole new signal on the other end.  In any
case, perhaps you can find such a device and that will give insight to
how we should deal with it.  Until such a device is found, it seems
fruitless to speculate.

Personally, I was pointed at "drivers/gpu/drm/i915/intel_hdmi.c".  If
you look there you will find a similar bit of code.


To summarize: I am not planning to spin my patch.  I am hopeful that
folks could review Yakir's series.  Would it help if he re-sent it
with different people in the "To" line?

Maybe when Yakir spins his series next he can include my patch?


-Doug

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 08:26:39AM -0700, Doug Anderson wrote:
> Russell,
> 
> On Thu, Jun 18, 2015 at 1:53 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> OK, so clearly my patch won't work against mainline.  I guess it's a
> >> good thing that I pointed out that it was only tested locally (would
> >> have been better to test against mainline, but I don't think that's so
> >> easy since there are several unlanded patches in mainline for
> >> Rockchip).
> >
> > As far as I'm aware, Freescale's original BSP version was the same, as is
> > their later BSPs, and Jon's maintained 3.14-stable kernel.
> 
> Was "the same"?  You mean was untested, or was 3.14?

I'm saying that the mdvi thing behaves the same in all the kernel sources
I've seen of this driver, and I'm unaware of anything that changes it -
and I've been looking at Jon's 3.14-stable kernels as well as Freescale's
git repository.

> >> As pointed out by others at <http://crosreview.com/278255>, locally
> >> our kernel has a slightly older version of
> >> <https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
> >> as needed.
> >
> > Please don't post unreliable lkml.org URLs, please use some other archive
> > site.  I can't access this URL at the moment.
> 
> Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>

Something like that needs to be done, but let's get rid of the mdvi
thing in struct hdmi_vmode - it doesn't belong there, it isn't part
of the currently set video mode, but becomes a property of the
connected sink.

I'd also prefer it to be called "is_dvi_sink", especially as its
function is changing from "is it a CEA mode" to "is the attached
device a DVI sink".

Even better would be to call it "is_hdmi_sink" to maintain positive
logic with single-negation where required, rather than double-
negation in places.

> >> ...so I guess my change is blocked on someone reviewing/landing that
> >> series.  If that series is rejected (or is changed sufficiently so
> >> that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
> >> will need to be re-spun.
> >
> > That's not what I said.  I said mdvi is set according to whether the mode
> > being set is a CEA mode or not.
> 
> Perhaps now that you can access the patch with the patchwork link you
> can re-read my email.  If/when that patch lands then mdvi _will_ be
> set as per drm_detect_hdmi_monitor().

Well, I object to that patch (see above.)

> I am nowhere near an HDMI expert.  If you have a better suggestion
> then I'm more than happy for you to post it and drop my patch.  In my
> non-expert opinion, it would seem awfully strange for an AV receiver
> to modify the EDID though unless it was actively interpreting the
> signal and generating a whole new signal on the other end.  In any
> case, perhaps you can find such a device and that will give insight to
> how we should deal with it.  Until such a device is found, it seems
> fruitless to speculate.

Neither am I, but I have had the ability to do some testing with AV
receivers in the path of a HDMI device, and I've seen how they behave.
(I made copious notes on this, which I intend to publish when I have
a round tuit.)  Unfortunately, I have no DVI devices to test with,
and DVI devices are a dying breed - most monitors today come with
HDMI sockets instead.

> Personally, I was pointed at "drivers/gpu/drm/i915/intel_hdmi.c".  If
> you look there you will find a similar bit of code.

Yea, I've also been using that for inspiration too, but I put personal
testing above what's in someone elses driver. :)

> To summarize: I am not planning to spin my patch.  I am hopeful that
> folks could review Yakir's series.  Would it help if he re-sent it
> with different people in the "To" line?

That's a shame... I'm not inclined to Ack it as-is - and I'd also like
to see Yakir's patch reworked as I mentioned above.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 08:26:39AM -0700, Doug Anderson wrote:
> Russell,
> 
> On Thu, Jun 18, 2015 at 1:53 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> OK, so clearly my patch won't work against mainline.  I guess it's a
> >> good thing that I pointed out that it was only tested locally (would
> >> have been better to test against mainline, but I don't think that's so
> >> easy since there are several unlanded patches in mainline for
> >> Rockchip).
> >
> > As far as I'm aware, Freescale's original BSP version was the same, as is
> > their later BSPs, and Jon's maintained 3.14-stable kernel.
> 
> Was "the same"?  You mean was untested, or was 3.14?

I'm saying that the mdvi thing behaves the same in all the kernel sources
I've seen of this driver, and I'm unaware of anything that changes it -
and I've been looking at Jon's 3.14-stable kernels as well as Freescale's
git repository.

> >> As pointed out by others at <http://crosreview.com/278255>, locally
> >> our kernel has a slightly older version of
> >> <https://lkml.org/lkml/2015/2/28/291>, which would change mdvi to be
> >> as needed.
> >
> > Please don't post unreliable lkml.org URLs, please use some other archive
> > site.  I can't access this URL at the moment.
> 
> Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>

Something like that needs to be done, but let's get rid of the mdvi
thing in struct hdmi_vmode - it doesn't belong there, it isn't part
of the currently set video mode, but becomes a property of the
connected sink.

I'd also prefer it to be called "is_dvi_sink", especially as its
function is changing from "is it a CEA mode" to "is the attached
device a DVI sink".

Even better would be to call it "is_hdmi_sink" to maintain positive
logic with single-negation where required, rather than double-
negation in places.

> >> ...so I guess my change is blocked on someone reviewing/landing that
> >> series.  If that series is rejected (or is changed sufficiently so
> >> that mdvi no longer is set via drm_detect_hdmi_monitor() then my patch
> >> will need to be re-spun.
> >
> > That's not what I said.  I said mdvi is set according to whether the mode
> > being set is a CEA mode or not.
> 
> Perhaps now that you can access the patch with the patchwork link you
> can re-read my email.  If/when that patch lands then mdvi _will_ be
> set as per drm_detect_hdmi_monitor().

Well, I object to that patch (see above.)

> I am nowhere near an HDMI expert.  If you have a better suggestion
> then I'm more than happy for you to post it and drop my patch.  In my
> non-expert opinion, it would seem awfully strange for an AV receiver
> to modify the EDID though unless it was actively interpreting the
> signal and generating a whole new signal on the other end.  In any
> case, perhaps you can find such a device and that will give insight to
> how we should deal with it.  Until such a device is found, it seems
> fruitless to speculate.

Neither am I, but I have had the ability to do some testing with AV
receivers in the path of a HDMI device, and I've seen how they behave.
(I made copious notes on this, which I intend to publish when I have
a round tuit.)  Unfortunately, I have no DVI devices to test with,
and DVI devices are a dying breed - most monitors today come with
HDMI sockets instead.

> Personally, I was pointed at "drivers/gpu/drm/i915/intel_hdmi.c".  If
> you look there you will find a similar bit of code.

Yea, I've also been using that for inspiration too, but I put personal
testing above what's in someone elses driver. :)

> To summarize: I am not planning to spin my patch.  I am hopeful that
> folks could review Yakir's series.  Would it help if he re-sent it
> with different people in the "To" line?

That's a shame... I'm not inclined to Ack it as-is - and I'd also like
to see Yakir's patch reworked as I mentioned above.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 04:55:45PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 18, 2015 at 08:26:39AM -0700, Doug Anderson wrote:
> > Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>
> 
> Something like that needs to be done, but let's get rid of the mdvi
> thing in struct hdmi_vmode - it doesn't belong there, it isn't part
> of the currently set video mode, but becomes a property of the
> connected sink.
> 
> I'd also prefer it to be called "is_dvi_sink", especially as its
> function is changing from "is it a CEA mode" to "is the attached
> device a DVI sink".
> 
> Even better would be to call it "is_hdmi_sink" to maintain positive
> logic with single-negation where required, rather than double-
> negation in places.

This is actually a _very_ important point.  Changing the function of
mdvi when it's used in multiple places throughout the driver is not on -
it's too big a change:

        /*check csc whether needed activated in HDMI mode */
        cscon = (is_color_space_conversion(hdmi) &&
                        !hdmi->hdmi_data.video_mode.mdvi);

        inv_val |= (vmode->mdvi ?
                HDMI_FC_INVIDCONF_DVI_MODEZ_DVI_MODE :
                HDMI_FC_INVIDCONF_DVI_MODEZ_HDMI_MODE);

        if (hdmi->cable_plugin && !hdmi->hdmi_data.video_mode.mdvi)
                hdmi_enable_overflow_interrupts(hdmi);

It's unclear what the effect would be to change the meaning of mdvi
from "this is a CEA mode" to "the attached device is DVI" in all these
locations, and it's just not on to do this in a patch which merely
says:

   If the monitor support audio, so we should support audio for it, even if
   the display resolution is No-CEA mode.

In other words, doesn't even describe this change.

In any case, this patch has been dropped from more recent audio driver
series.

So, what I'd like to see is a patch series which starts with the change
below, and builds on that, with explainations why each change is needed.
This is important, as this is shared IP, and we need to make sure that
we don't regress non-Rockchip users of this IP.  I'll try and do some
work in this area if nothing crops up in the next month.

 drivers/gpu/drm/bridge/dw_hdmi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb61d290..8834e8142ea6 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -119,6 +119,8 @@ struct dw_hdmi {
 
 	u8 edid[HDMI_EDID_LEN];
 	bool cable_plugin;
+	bool sink_is_hdmi;
+	bool sink_has_audio;
 
 	bool phy_enabled;
 	struct drm_display_mode previous_mode;
@@ -1402,6 +1404,9 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 
 	edid = drm_get_edid(connector, hdmi->ddc);
 	if (edid) {
+		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
+
 		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
 			edid->width_cm, edid->height_cm);
 


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Doug Anderson]

Russell,

On Thu, Jun 18, 2015 at 9:09 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> So, what I'd like to see is a patch series which starts with the change
> below, and builds on that, with explainations why each change is needed.
> This is important, as this is shared IP, and we need to make sure that
> we don't regress non-Rockchip users of this IP.  I'll try and do some
> work in this area if nothing crops up in the next month.

OK.  I've mostly been jumping in here to do a bugfix or two, not to
take over maintenance of the driver.  My general policy is to submit
things upstream if at all possible, but I think in the case of HDMI we
are just too different from upstream for this to be easy.

I'll let you and Yakir figure out the way forward to keep everyone
happy.  If that means you submitting some patches then great.  If that
means Yakir submitting some patches then that's great too.  In such a
case you can consider my patch to be a bug report and I'll be happy
with folks figure out the proper way to do this in the upstream
driver.  :)

-Doug

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Doug Anderson]

Russell,

On Thu, Jun 18, 2015 at 8:55 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>
>
> Something like that needs to be done, but let's get rid of the mdvi
> thing in struct hdmi_vmode - it doesn't belong there, it isn't part
> of the currently set video mode, but becomes a property of the
> connected sink.
>
> I'd also prefer it to be called "is_dvi_sink", especially as its
> function is changing from "is it a CEA mode" to "is the attached
> device a DVI sink".
>
> Even better would be to call it "is_hdmi_sink" to maintain positive
> logic with single-negation where required, rather than double-
> negation in places.

Yakir: sounds like you now have some feedback on your patch now.
Perhaps you can spin it with Russell's feedback?

When you send it next, please make sure you include Russell in the
"To" line.  Based at looking at who committed things to dw_hdmi in the
past, I've been sending my patches "To":
    Philipp Zabel
    Russell King
    Thierry Reding

...so perhaps that would be good for you to do, too?

>> I am nowhere near an HDMI expert.  If you have a better suggestion
>> then I'm more than happy for you to post it and drop my patch.  In my
>> non-expert opinion, it would seem awfully strange for an AV receiver
>> to modify the EDID though unless it was actively interpreting the
>> signal and generating a whole new signal on the other end.  In any
>> case, perhaps you can find such a device and that will give insight to
>> how we should deal with it.  Until such a device is found, it seems
>> fruitless to speculate.
>
> Neither am I, but I have had the ability to do some testing with AV
> receivers in the path of a HDMI device, and I've seen how they behave.
> (I made copious notes on this, which I intend to publish when I have
> a round tuit.)  Unfortunately, I have no DVI devices to test with,
> and DVI devices are a dying breed - most monitors today come with
> HDMI sockets instead.

Ah, OK.  Have you seen any that specifically confuse the DVI vs. HDMI bits?

I still have copious DVI devices around, personally.  I could have
sworn that supporting "older hardware" was actually pretty important
in Linux.  ...and I can still buy plenty of DVI devices out there.


> That's a shame... I'm not inclined to Ack it as-is - and I'd also like
> to see Yakir's patch reworked as I mentioned above.

Perhaps when Yakir spins his series he can include a patch like mine
in it.  It doesn't make sense for me to re-spin it until his is
resolved.

-Doug

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Yakir Yang]

Doug & Russell,

在 2015/6/19 0:10, Doug Anderson 写道:
> Russell,
>
> On Thu, Jun 18, 2015 at 8:55 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>>> Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>
>> Something like that needs to be done, but let's get rid of the mdvi
>> thing in struct hdmi_vmode - it doesn't belong there, it isn't part
>> of the currently set video mode, but becomes a property of the
>> connected sink.
>>
>> I'd also prefer it to be called "is_dvi_sink", especially as its
>> function is changing from "is it a CEA mode" to "is the attached
>> device a DVI sink".
>>
>> Even better would be to call it "is_hdmi_sink" to maintain positive
>> logic with single-negation where required, rather than double-
>> negation in places.
> Yakir: sounds like you now have some feedback on your patch now.
> Perhaps you can spin it with Russell's feedback?
>
> When you send it next, please make sure you include Russell in the
> "To" line.  Based at looking at who committed things to dw_hdmi in the
> past, I've been sending my patches "To":
>      Philipp Zabel
>      Russell King
>      Thierry Reding
>
> ...so perhaps that would be good for you to do, too?

Okay,  thanks for your remind.

>>> I am nowhere near an HDMI expert.  If you have a better suggestion
>>> then I'm more than happy for you to post it and drop my patch.  In my
>>> non-expert opinion, it would seem awfully strange for an AV receiver
>>> to modify the EDID though unless it was actively interpreting the
>>> signal and generating a whole new signal on the other end.  In any
>>> case, perhaps you can find such a device and that will give insight to
>>> how we should deal with it.  Until such a device is found, it seems
>>> fruitless to speculate.
>> Neither am I, but I have had the ability to do some testing with AV
>> receivers in the path of a HDMI device, and I've seen how they behave.
>> (I made copious notes on this, which I intend to publish when I have
>> a round tuit.)  Unfortunately, I have no DVI devices to test with,
>> and DVI devices are a dying breed - most monitors today come with
>> HDMI sockets instead.
> Ah, OK.  Have you seen any that specifically confuse the DVI vs. HDMI bits?
>
> I still have copious DVI devices around, personally.  I could have
> sworn that supporting "older hardware" was actually pretty important
> in Linux.  ...and I can still buy plenty of DVI devices out there.
>
>
>> That's a shame... I'm not inclined to Ack it as-is - and I'd also like
>> to see Yakir's patch reworked as I mentioned above.
> Perhaps when Yakir spins his series he can include a patch like mine
> in it.  It doesn't make sense for me to re-spin it until his is
> resolved.
I will rebase on russell series and re-send my patch today.

- Yakir
> -Doug
>
>
>

Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Yakir Yang]

Doug & Russell,

在 2015/6/19 0:10, Doug Anderson 写道:
> Russell,
>
> On Thu, Jun 18, 2015 at 8:55 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>>> Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>
>> Something like that needs to be done, but let's get rid of the mdvi
>> thing in struct hdmi_vmode - it doesn't belong there, it isn't part
>> of the currently set video mode, but becomes a property of the
>> connected sink.
>>
>> I'd also prefer it to be called "is_dvi_sink", especially as its
>> function is changing from "is it a CEA mode" to "is the attached
>> device a DVI sink".
>>
>> Even better would be to call it "is_hdmi_sink" to maintain positive
>> logic with single-negation where required, rather than double-
>> negation in places.
> Yakir: sounds like you now have some feedback on your patch now.
> Perhaps you can spin it with Russell's feedback?
>
> When you send it next, please make sure you include Russell in the
> "To" line.  Based at looking at who committed things to dw_hdmi in the
> past, I've been sending my patches "To":
>      Philipp Zabel
>      Russell King
>      Thierry Reding
>
> ...so perhaps that would be good for you to do, too?

Okay,  thanks for your remind.

>>> I am nowhere near an HDMI expert.  If you have a better suggestion
>>> then I'm more than happy for you to post it and drop my patch.  In my
>>> non-expert opinion, it would seem awfully strange for an AV receiver
>>> to modify the EDID though unless it was actively interpreting the
>>> signal and generating a whole new signal on the other end.  In any
>>> case, perhaps you can find such a device and that will give insight to
>>> how we should deal with it.  Until such a device is found, it seems
>>> fruitless to speculate.
>> Neither am I, but I have had the ability to do some testing with AV
>> receivers in the path of a HDMI device, and I've seen how they behave.
>> (I made copious notes on this, which I intend to publish when I have
>> a round tuit.)  Unfortunately, I have no DVI devices to test with,
>> and DVI devices are a dying breed - most monitors today come with
>> HDMI sockets instead.
> Ah, OK.  Have you seen any that specifically confuse the DVI vs. HDMI bits?
>
> I still have copious DVI devices around, personally.  I could have
> sworn that supporting "older hardware" was actually pretty important
> in Linux.  ...and I can still buy plenty of DVI devices out there.
>
>
>> That's a shame... I'm not inclined to Ack it as-is - and I'd also like
>> to see Yakir's patch reworked as I mentioned above.
> Perhaps when Yakir spins his series he can include a patch like mine
> in it.  It doesn't make sense for me to re-spin it until his is
> resolved.
I will rebase on russell series and re-send my patch today.

- Yakir
> -Doug
>
>
>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel