Re: [Intel-gfx] [PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance
2014-07-14 16:10 GMT-03:00 Todd Previte tprev...@gmail.com:
Implements an updated version of the automated testing function that handles
Displayport compliance for EDID operations.
Both the commit message and the code should mention the name of the
specification that defines these tests, and also mention which
specific tests are implemented by this patch/function. I see that
there are multiple tests being implemented here, but reading the 232
pages of the spec will require too much time, so knowing which ones
are implemented really helps the reviewers :)
Also, you should tell us what happens before and after this patch when
you run your own tests. How many tests were we previously passing? How
many tests are we passing now? I see there are some FIXME lines below,
which leads to the question: is the code you provided enough and
complete, or do we still need more adjustments to pass everything we
can with what you built? In other words: is this patch, alone, already
an improvement to the situation?
Signed-off-by: Todd Previte tprev...@gmail.com
---
drivers/gpu/drm/i915/intel_dp.c | 77
-
1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 33b6dc9..88f1bbe 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp
*intel_dp)
static uint8_t
intel_dp_autotest_edid(struct intel_dp *intel_dp)
{
- uint8_t test_result = DP_TEST_NAK;
+ struct drm_connector *connector = intel_dp-attached_connector-base;
+ struct i2c_adapter *adapter = intel_dp-aux.ddc;
+ struct edid *edid_read = NULL;
+ uint8_t *edid_data = NULL;
+ uint8_t test_result = DP_TEST_NAK, checksum = 0;
+ uint32_t i = 0, ret = 0;
+ struct drm_display_mode *use_mode = NULL;
+ int mode_count = 0;
+ struct drm_mode_set modeset;
You have initialized every single variable you defined. This creates
the problem that unused variables are not pointed by the compiler
unless you enable the flags to warn set-but-not-used variables. For
example, i is unused, and ret is set but never used. I also don't
really see the point in, for example, NULL-initializing stuff like
edid_data and edid_read.
+
+ DRM_DEBUG_KMS(Displayport: EDID automated test\n);
+
+ /* Reset the NACK/DEFER counters */
As I said before, this is a great example of the comment says what
the code already says problem.
+ intel_dp-aux.i2c_nack_count = 0;
+ intel_dp-aux.i2c_defer_count = 0;
+ /* Now read out the EDID */
+ edid_read = drm_get_edid(connector, adapter);
+
+ if (edid_read == NULL) {
+ /* Check for NACKs/DEFERs, goto failsafe if detected
+ (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
Our coding standard is to put aligned '*'s on each line of a
multi-line comment. So this should be \t\t * (DP CTS 1.2 etc...
instead of what is above. In theory, the /* and */ strings should
be on their own lines, alone, but we are inconsistent regarding this
(even though Daniel randomly complained about this to me a few times
in the past). As usual, check Documentation/CodingStyle for better
explanations.
+ if (intel_dp-aux.i2c_nack_count 0 ||
+ intel_dp-aux.i2c_defer_count 0)
We tend to align the contents of the extra line with the '(' char on
the line above.
+ DRM_DEBUG_KMS(Displayport: EDID read generated %d
I2C NACKs, %d DEFERs\n,
+
intel_dp-aux.i2c_nack_count,
+
intel_dp-aux.i2c_defer_count);
There are way too many tabs on the 2 lines above.
+ goto failsafe;
+ }
+
+ /* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */
But what is the problem with the current code? What are the
consequences of not implementing the FIXME?
+ edid_data = (uint8_t *) edid_read;
+
+ if (intel_dp_compute_edid_checksum(edid_data, checksum)) {
+ /* Write the checksum to EDID checksum register */
+ ret = drm_dp_dpcd_write(intel_dp-aux,
+
DP_TEST_EDID_CHECKSUM,
+
edid_read-checksum, 1);
Way too many tabs above too.
+ /* Reponse is ACK and and checksum written */
+ test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+ } else {
+ /* Invalid checksum - EDID corruption detection test */
+ goto failsafe;
+ }
+
+ /* Update EDID modes */
+ mode_count = intel_connector_update_modes(connector, edid_read);
+ if (!mode_count) {
+
Re: [Intel-gfx] [PATCH 1/8] drm: Add drm_plane/connector_index
On Tue, Jul 29, 2014 at 11:32:16PM +0200, Daniel Vetter wrote:
In the atomic state we'll have an array of states for crtcs, planes
and connectors and need to be able to at them by their index. We
already have a drm_crtc_index function so add the missing ones for
planes and connectors.
If it later on turns out that the list walking is too expensive we can
add the index to the relevant modeset objects.
Rob Clark doesn't like the loops too much, but we can always add an
obj-idx parameter later on. And for now reiterating is actually safer
since nowadays we have hotpluggable connectors (thanks to DP MST).
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/drm_crtc.c | 46
++
include/drm/drm_crtc.h | 2 ++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 805240b11229..5a494caa8c9a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -937,6 +937,29 @@ void drm_connector_cleanup(struct drm_connector
*connector)
EXPORT_SYMBOL(drm_connector_cleanup);
/**
+ * drm_plane_index - find the index of a registered CRTC
Looks like some copy/paste that needs an update...your kerneldoc calls
the function drm_*PLANE*_index and then talks about CRTC's, but the
actual function is for connectors...
+ * @plane: CRTC to find index for
+ *
+ * Given a registered CRTC, return the index of that CRTC within a DRM
+ * device's list of CRTCs.
+ */
+unsigned int drm_connector_index(struct drm_connector *connector)
+{
+ unsigned int index = 0;
+ struct drm_connector *tmp;
+
+ list_for_each_entry(tmp, connector-dev-mode_config.connector_list,
head) {
+ if (tmp == connector)
+ return index;
+
+ index++;
+ }
+
+ BUG();
+}
+EXPORT_SYMBOL(drm_connector_index);
+
+/**
* drm_connector_register - register a connector
* @connector: the connector to register
*
@@ -1239,6 +1262,29 @@ void drm_plane_cleanup(struct drm_plane *plane)
EXPORT_SYMBOL(drm_plane_cleanup);
/**
+ * drm_plane_index - find the index of a registered CRTC
+ * @plane: CRTC to find index for
+ *
+ * Given a registered CRTC, return the index of that CRTC within a DRM
+ * device's list of CRTCs.
+ */
More copy/paste referenecs to CRTC's.
+unsigned int drm_plane_index(struct drm_plane *plane)
+{
+ unsigned int index = 0;
+ struct drm_plane *tmp;
+
+ list_for_each_entry(tmp, plane-dev-mode_config.plane_list, head) {
+ if (tmp == plane)
+ return index;
+
+ index++;
+ }
+
+ BUG();
+}
+EXPORT_SYMBOL(drm_plane_index);
+
+/**
* drm_plane_force_disable - Forcibly disable a plane
* @plane: plane to disable
*
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f1105d0da059..4cae44611ab0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -903,6 +903,7 @@ int drm_connector_register(struct drm_connector
*connector);
void drm_connector_unregister(struct drm_connector *connector);
extern void drm_connector_cleanup(struct drm_connector *connector);
+extern unsigned int drm_connector_index(struct drm_connector *crtc);
connector?
/* helper to unplug all connectors from sysfs for device */
extern void drm_connector_unplug_all(struct drm_device *dev);
@@ -942,6 +943,7 @@ extern int drm_plane_init(struct drm_device *dev,
const uint32_t *formats, uint32_t format_count,
bool is_primary);
extern void drm_plane_cleanup(struct drm_plane *plane);
+extern unsigned int drm_plane_index(struct drm_plane *crtc);
plane?
extern void drm_plane_force_disable(struct drm_plane *plane);
extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
int x, int y,
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] drm: Move modeset_lock_all helpers to drm_modeset_lock.[hc]
On 30 July 2014 07:32, Daniel Vetter daniel.vet...@ffwll.ch wrote: Somehow we've forgotten about this little bit of OCD. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Reviewed-by: Dave Airlie airl...@redhat.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/8] drm: Handle legacy per-crtc locking with full acquire ctx
---
drivers/gpu/drm/drm_crtc.c | 8 ++--
drivers/gpu/drm/drm_modeset_lock.c | 84
++
include/drm/drm_crtc.h | 6 +++
include/drm/drm_modeset_lock.h | 5 +++
4 files changed, 99 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ff583bec31f9..c09374038f9a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2714,7 +2714,7 @@ static int drm_mode_cursor_common(struct drm_device
*dev,
if (crtc-cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);
- drm_modeset_lock(crtc-mutex, NULL);
+ drm_modeset_lock_crtc(crtc);
if (req-flags DRM_MODE_CURSOR_BO) {
if (!crtc-funcs-cursor_set !crtc-funcs-cursor_set2) {
ret = -ENXIO;
@@ -2738,7 +2738,7 @@ static int drm_mode_cursor_common(struct drm_device
*dev,
}
}
out:
- drm_modeset_unlock(crtc-mutex);
+ drm_modeset_unlock_crtc(crtc);
return ret;
@@ -4474,7 +4474,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
if (!crtc)
return -ENOENT;
- drm_modeset_lock(crtc-mutex, NULL);
+ drm_modeset_lock_crtc(crtc);
if (crtc-primary-fb == NULL) {
/* The framebuffer is currently unbound, presumably
* due to a hotplug event, that userspace has not
@@ -4558,7 +4558,7 @@ out:
drm_framebuffer_unreference(fb);
if (old_fb)
drm_framebuffer_unreference(old_fb);
- drm_modeset_unlock(crtc-mutex);
+ drm_modeset_unlock_crtc(crtc);
return ret;
}
diff --git a/drivers/gpu/drm/drm_modeset_lock.c
b/drivers/gpu/drm/drm_modeset_lock.c
index 73e6534fd0aa..4d2aa549c614 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -130,6 +130,90 @@ void drm_modeset_unlock_all(struct drm_device *dev)
EXPORT_SYMBOL(drm_modeset_unlock_all);
/**
+ * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
+ * @crtc: drm crtc
+ *
+ * This function locks the given crtc using a hidden acquire context. This is
+ * necessary so that drivers internally using the atomic interfaces can grab
+ * furether locks with the lock acquire context.
^ typo - further
Otherwise
Reviewed-by: Dave Airlie airl...@redhat.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/8] drm: Move -old_fb from crtc to plane
On 30 July 2014 07:32, Daniel Vetter daniel.vet...@ffwll.ch wrote: Atomic implemenations for legacy ioctls must be able to drop locks. Which doesn't cause havoc since we only do that while constructing the new state, so no driver or hardware state change has happened. The only troubling bit is the fb refcounting the core does - if someone else has snuck in then it might potentially unref an outdated framebuffer. To fix that move the old_fb temporary storage into struct drm_plane for all ioctls, so that the atomic helpers can update it. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Seems to make sense to me. Reviewed-by: Dave Airlie airl...@redhat.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/8] drm: Move -old_fb from crtc to plane
On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
Atomic implemenations for legacy ioctls must be able to drop locks.
Which doesn't cause havoc since we only do that while constructing
the new state, so no driver or hardware state change has happened.
The only troubling bit is the fb refcounting the core does - if
someone else has snuck in then it might potentially unref an
outdated framebuffer. To fix that move the old_fb temporary storage
into struct drm_plane for all ioctls, so that the atomic helpers can
update it.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/drm_crtc.c | 40
include/drm/drm_crtc.h | 8
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c09374038f9a..bacf565449d5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
*/
void drm_plane_force_disable(struct drm_plane *plane)
{
- struct drm_framebuffer *old_fb = plane-fb;
int ret;
- if (!old_fb)
+ if (!plane-fb)
return;
+ plane-old_fb = plane-fb;
ret = plane-funcs-disable_plane(plane);
if (ret) {
DRM_ERROR(failed to disable plane with busy fb\n);
+ plane-old_fb = NULL;
return;
}
/* disconnect the plane from the fb and crtc: */
- __drm_framebuffer_unreference(old_fb);
+ __drm_framebuffer_unreference(plane-old_fb);
+ plane-old_fb = NULL;
plane-fb = NULL;
plane-crtc = NULL;
}
@@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
uint32_t src_w, uint32_t src_h)
{
struct drm_device *dev = plane-dev;
- struct drm_framebuffer *old_fb = NULL;
+ struct drm_framebuffer *old_fb;
I think there may be cases where old_fb gets unref'd without ever being
set if we drop the NULL assignment. E.g., if the possible_crtcs test or
the format test fail, we jump down to out and then test the value +
unref which could be garbage.
Would it be simpler to just drm_modeset_lock_all() unconditionally at
the start of the function and then just unlock after the unrefs at the
end of the function so that we don't need a local old_fb?
int ret = 0;
unsigned int fb_width, fb_height;
int i;
@@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
/* No fb means shut it down */
if (!fb) {
drm_modeset_lock_all(dev);
- old_fb = plane-fb;
+ plane-old_fb = plane-fb;
ret = plane-funcs-disable_plane(plane);
if (!ret) {
plane-crtc = NULL;
plane-fb = NULL;
} else {
- old_fb = NULL;
+ plane-old_fb = NULL;
}
+ old_fb = plane-old_fb;
+ plane-old_fb = NULL;
drm_modeset_unlock_all(dev);
goto out;
}
@@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
}
drm_modeset_lock_all(dev);
- old_fb = plane-fb;
+ plane-old_fb = plane-fb;
ret = plane-funcs-update_plane(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,
src_x, src_y, src_w, src_h);
@@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
plane-fb = fb;
fb = NULL;
} else {
- old_fb = NULL;
+ plane-old_fb = NULL;
}
+ old_fb = plane-old_fb;
+ plane-old_fb = NULL;
drm_modeset_unlock_all(dev);
out:
@@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set
*set)
* crtcs. Atomic modeset will have saner semantics ...
*/
list_for_each_entry(tmp, crtc-dev-mode_config.crtc_list, head)
- tmp-old_fb = tmp-primary-fb;
+ tmp-primary-old_fb = tmp-primary-fb;
fb = set-fb;
@@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set
*set)
list_for_each_entry(tmp, crtc-dev-mode_config.crtc_list, head) {
if (tmp-primary-fb)
drm_framebuffer_reference(tmp-primary-fb);
- if (tmp-old_fb)
- drm_framebuffer_unreference(tmp-old_fb);
+ if (tmp-primary-old_fb)
+ drm_framebuffer_unreference(tmp-primary-old_fb);
+ tmp-primary-old_fb = NULL;
}
return ret;
@@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
{
struct drm_mode_crtc_page_flip *page_flip = data;
struct drm_crtc *crtc;
- struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+ struct
[Intel-gfx] [PATCH 1/2] drm/i915/hdmi: call intel_hdmi_prepare for CHV
From: Libin Yang libin.y...@intel.com call the intel_hdmi_prepare() in chv_hdmi_pre_enable() for hdmi audio. Signed-off-by: Libin Yang libin.y...@intel.com --- drivers/gpu/drm/i915/intel_hdmi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 5f8f4ca..5a65e0c 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1374,6 +1374,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder) int data, i; u32 val; + intel_hdmi_prepare(encoder); mutex_lock(dev_priv-dpio_lock); /* Deassert soft data lane reset*/ -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
On 30.07.2014 06:32, Daniel Vetter wrote:
As usual in both a crtc index and a struct drm_crtc * version.
The function assumes that no one drivers their display below 10Hz, and
it will complain if the vblank wait takes longer than that.
v2: Also check dev-max_vblank_counter since some drivers register a
fake get_vblank_counter function.
What does that refer to? Can't find any other reference to
max_vblank_counter in the patch.
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123afdb34..76024fdde452 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
EXPORT_SYMBOL(drm_crtc_vblank_put);
/**
+ * drm_vblank_wait - wait for one vblank
+ * @dev: DRM device
+ * @crtc: crtc index
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver
interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable,
e.g.
Spelling: 'disabled'
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_vblank_wait(struct drm_device *dev, int crtc)
+{
+ int ret;
+ u32 last;
+
+ ret = drm_vblank_get(dev, crtc);
+ if (WARN_ON(ret))
+ return;
+
+ last = drm_vblank_count(dev, crtc);
+
+#define C (last != drm_vblank_count(dev, crtc))
+ ret = wait_event_timeout(dev-vblank[crtc].queue,
+ C, msecs_to_jiffies(100));
+
+ WARN_ON(ret == 0);
+#undef C
What's the point of the C macro?
+ drm_vblank_put(dev, crtc);
+}
+EXPORT_SYMBOL(drm_vblank_wait);
+
+/**
+ * drm_crtc_vblank_wait - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver
interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable,
e.g.
Same typo as above.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_vblank_wait(struct drm_crtc *crtc)
+{
+ drm_vblank_wait(crtc-dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait);
+
+/**
Maybe the function names should be *_vblank_wait_next() or something to
clarify the purpose and reduce potential confusion versus drm_wait_vblank().
Looks good to me other than that.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
