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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.