From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Daniel Vetter" <daniel@ffwll.ch>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Michael Davidson <md@google.com>,
Grant Grundler <grundler@chromium.org>,
Matthias Kaehlcke <mka@chromium.org>,
Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches
Date: Mon, 08 May 2017 11:17:37 +0300 [thread overview]
Message-ID: <87wp9rahjy.fsf@intel.com> (raw)
In-Reply-To: <20170508072144.vcydqrmty7zkscw6@phenom.ffwll.local>
On Mon, 08 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 05, 2017 at 08:40:43PM +0300, Ville Syrjälä wrote:
>> On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> >
>> > > In several instances the driver passes an 'enum pipe' value to a
>> > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
>> > > TRANSCODER_x have the same values this doesn't cause functional
>> > > problems. Still it is incorrect and causes clang to generate warnings
>> > > like this:
>> > >
>> > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> > > conversion from enumeration type 'enum transcoder' to different
>> > > enumeration type 'enum pipe' [-Wenum-conversion]
>> > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> > >
>> > > Change the code to pass values of the type expected by the callee.
>> > >
>> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> > > ---
>> > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
>> > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
>> > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
>> > > 4 files changed, 14 insertions(+), 8 deletions(-)
>> >
>> > Ping, any comments on this patch?
>>
>> I'm not convinced the patch is making things any better really. To
>> fix this really properly, I think we'd need to introduce a new enum
>> pch_transcoder and thus avoid the confusion of which type of
>> transcoder we're talking about. Currently most places expect an
>> enum pipe when dealing with PCH transcoders, and enum transcoder
>> when dealing with CPU transcoders. But there are some exceptions
>> of course.
>
> enum transcoder is wrong for the pch, that enum is only for cpu
> transcoders introduced in hsw+. PCH should always use enum pipe.
For background, below is the commit message for introduction of enum
transcoder.
> So a patch to switch the enum to enum pipe for all the pch functions
> sounds like the right thing to do here. Plus maybe rename enum transcoder
> to enum cpu_transcoder, but that'd be tons of work. A comment instead
> might be easier ...
The enum pipe conversion might be the right thing to do *if* you must do
something. But I'm not convinced you must. It's a bunch of churn that's
not just purely mechanical conversion.
BR,
Jani.
commit a5c961d1f3a9ab5ba0e5706e866192f8108143fe
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date: Wed Oct 24 15:59:34 2012 -0200
drm/i915: add TRANSCODER_EDP
Before Haswell we used to have the CPU pipes and the PCH transcoders.
We had the same amount of pipes and transcoders, and there was a 1:1
mapping between them. After Haswell what we used to call CPU pipe was
split into CPU pipe and CPU transcoder. So now we have 3 CPU pipes (A,
B and C), 4 CPU transcoders (A, B, C and EDP) and 1 PCH transcoder
(only used for VGA).
For all the outputs except for EDP we have an 1:1 mapping on the CPU
pipes and CPU transcoders, so if you're using CPU pipe A you have to
use CPU transcoder A. When have an eDP output you have to use
transcoder EDP and you can attach this CPU transcoder to any of the 3
CPU pipes. When using VGA you need to select a pair of matching CPU
pipes/transcoders (A/A, B/B, C/C) and you also need to enable/use the
PCH transcoder.
For now we're just creating the cpu_transcoder definitions and setting
cpu_transcoder to TRANSCODER_EDP on DDI eDP code, but none of the
registers was ported to use transcoder instead of pipe. The goal is to
keep the code backwards-compatible since on all cases except when
using eDP we must have pipe == cpu_transcoder.
V2: Comment the haswell_crtc_off chunk, suggested by Damien Lespiau
and Daniel Vetter.
We currently need the haswell_crtc_off chunk because TRANSCODER_EDP
can be used by any CRTC, so when you stop using it you have to stop
saying you're using it, otherwise you may have at some point 2 CRTCs
claiming they're using TRANSCODER_EDP (a disabled CRTC and an enabled
one), then the HW state readout code will get completely confused.
In other words:
Imagine the following case:
xrandr --output eDP1 --auto --crtc 0
xrandr --output eDP1 --off
xrandr --output eDP1 --auto --crtc 2
After the last command you could get a "pipe A assertion failure
(expected off, current on)" because CRTC 0 still claims it's using
TRANSCODER_EDP, so the HW state readout function will read it
(through PIPECONF) and expect it to be off, when it's actually on
because it's being used by CRTC 2.
So when we make "intel_crtc->cpu_transcoder = intel_crtc->pipe" we
make sure we're pointing to our own original CRTC which is certainly
not used by any other CRTC.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
Jani Nikula, Intel Open Source Technology Center
next prev parent reply other threads:[~2017-05-08 8:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-20 21:56 [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Matthias Kaehlcke
2017-05-05 17:26 ` Matthias Kaehlcke
2017-05-05 17:40 ` [Intel-gfx] " Ville Syrjälä
2017-05-05 19:12 ` Grant Grundler
2017-05-05 20:08 ` Ville Syrjälä
2017-05-05 20:29 ` Grant Grundler
2017-05-05 21:37 ` Matthias Kaehlcke
2017-05-08 7:21 ` Daniel Vetter
2017-05-08 8:17 ` Jani Nikula [this message]
2017-07-13 2:28 ` Stéphane Marchesin
2017-07-13 10:13 ` Ville Syrjälä
2017-07-13 12:24 ` Daniel Vetter
2017-07-13 16:23 ` Stéphane Marchesin
2017-07-13 17:42 ` Ville Syrjälä
2017-07-13 19:58 ` Stéphane Marchesin
2017-07-14 9:11 ` Jani Nikula
2017-07-14 17:32 ` Grant Grundler
2017-07-14 21:35 ` Daniel Vetter
2017-07-14 22:43 ` Grant Grundler
2017-07-14 23:38 ` Matthias Kaehlcke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wp9rahjy.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=daniel.vetter@intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=grundler@chromium.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=md@google.com \
--cc=mka@chromium.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).