[4.4-rc1][Regression] drm/i915: Check live status before reading edid
On 2/26/2016 12:11 AM, Joseph Salisbury wrote:
> On 02/24/2016 10:53 PM, Jindal, Sonika wrote:
>> Hi Joe,
>>
>> Yes, first thing to try is to increase the tries.
> We testing with 300 retries, but the second monitor still did not show
> up. However, it did show up in lspci.
>
>
>> Can you please point me to the bug and provide more details like platform,
>> monitor, cable.
> The bug is at: http://pad.lv/1543683 . All the hardware details should
> be in the bug report. The cable is a single link dvi-d cable.
> Unfortunately the bug reporter does not have a dual link cable to test.
> If you need any additional info, we can ask the bug reporter.
If this is with single link cable, the issue could be the same.
As Ville suggested for the other issue to use video=HDMI-A-1:e as
command line argument, can you please give it a try?
The logs shared in the bug doesn't have drm logs enabled, so couldnt get
much out of it.
Which platform is this?
Alternatively you can add something like following in intel_hdmi_detect
to make it ignore the live status checks.
@@ -1419,7 +1419,7 @@ intel_hdmi_detect(struct drm_connector *connector,
bool force)
intel_hdmi_unset_edid(connector);
-
+ live_status = live_status | force;
if (intel_hdmi_set_edid(connector, live_status)) {
struct intel_hdmi *intel_hdmi =
intel_attached_hdmi(connector);
Regards,
Sonika
>> Are you referring to the same issue as Oleksandr reported where a single
>> link dvi/hdmi cable didnât work and dual link worked?
> I'm not sure if this is the exact issue or not. I'll review the other
> thread and compare.
>
>> Regards,
>> Sonika
>>
>> -Original Message-
>> From: Joseph Salisbury [mailto:joseph.salisbury at canonical.com]
>> Sent: Thursday, February 25, 2016 3:09 AM
>> To: Jindal, Sonika
>> Cc: Sharma, Shashank ; Vivi, Rodrigo
>> ; Daniel Vetter ; Jani
>> Nikula ; David Airlie ;
>> intel-gfx ; dri-devel > lists.freedesktop.org>; LKML
>> Subject: [4.4-rc1][Regression] drm/i915: Check live status before reading
>> edid
>>
>> Hi Sonika,
>>
>> A kernel bug report was opened against Ubuntu [0]. After a kernel bisect,
>> it was found that reverting the following commit resolved this bug:
>>
>> commit 237ed86c693d8a8e4db476976aeb30df4deac74b
>> Author: Sonika Jindal
>> Date: Tue Sep 15 09:44:20 2015 +0530
>>
>> drm/i915: Check live status before reading edid
>>
>>
>>
>> The regression was introduced as of v4.4-rc1.
>>
>> I was hoping to get your feedback, since you are the patch author. Do think
>> increasing the number of tries in intel_hdmi_detect() is worth trying? Do
>> you think gathering any additional data will help diagnose this issue, or
>> would it be best to submit a revert request?
>>
>>
>> Thanks,
>>
>> Joe
>>
>> [0] http://pad.lv/lp1543683
>>
>
[4.4-rc1][Regression] drm/i915: Check live status before reading edid
Hi Joe, Yes, first thing to try is to increase the tries. Can you please point me to the bug and provide more details like platform, monitor, cable. Are you referring to the same issue as Oleksandr reported where a single link dvi/hdmi cable didnât work and dual link worked? Regards, Sonika -Original Message- From: Joseph Salisbury [mailto:joseph.salisb...@canonical.com] Sent: Thursday, February 25, 2016 3:09 AM To: Jindal, Sonika Cc: Sharma, Shashank ; Vivi, Rodrigo ; Daniel Vetter ; Jani Nikula ; David Airlie ; intel-gfx ; dri-devel ; LKML Subject: [4.4-rc1][Regression] drm/i915: Check live status before reading edid Hi Sonika, A kernel bug report was opened against Ubuntu [0]. After a kernel bisect, it was found that reverting the following commit resolved this bug: commit 237ed86c693d8a8e4db476976aeb30df4deac74b Author: Sonika Jindal Date: Tue Sep 15 09:44:20 2015 +0530 drm/i915: Check live status before reading edid The regression was introduced as of v4.4-rc1. I was hoping to get your feedback, since you are the patch author. Do think increasing the number of tries in intel_hdmi_detect() is worth trying? Do you think gathering any additional data will help diagnose this issue, or would it be best to submit a revert request? Thanks, Joe [0] http://pad.lv/lp1543683
[REGRESSION] i915: No HDMI output with 4.4
On 2/23/2016 8:38 PM, Ville Syrjälä wrote: > On Mon, Feb 22, 2016 at 02:32:32PM +0200, Oleksandr Natalenko wrote: >> Ville, Daniel, >> >> any additional info I could provide? I have to return dual-link DVI >> cable back, so let me know if I could reveal more details if necessary. > Unfortunately I'm out of ideas for now. Daniel is on vacation. > Anyone else? VPG folks should take the ball here since they broke it. > > In the meantime I think as a workaround I think you could use > something like video=HDMI-A-1:e on the kernel command line (not sure > I got the connector name right for your system). I think that should > result in the live status check to be skipped, at least when > populating the mode list. > > Might be a good idea to collect all the information here and put in a > bug report (https://bugs.freedesktop.org/ -> DRI -> DRM/Intel) so that > all the logs and such would be in one place. > >> Regards, >> Oleksandr >> >> 16.02.2016 14:54, Daniel Vetter напиÑав: >>> On Tue, Feb 16, 2016 at 12:58:56PM +0200, Oleksandr Natalenko wrote: Ville, Daniel, I've just got another monitor and another DVI-HDMI cable, and here what I've got. ===Single Link DVI-D cable with 3 different monitors=== Computer DVI âââ DVI-D (Single Link)/HDMI cable âââ HDMI LG 23MP65HQ-P === not working >>> I presume the above LG screen is what you've called previously "old >>> monitor"? >>> Computer DVI âââ DVI-D (Single Link)/HDMI cable âââ HDMI LG 23MP67HQ-P === not working Do you have logs for the failure with the single link hdmi cable and the register dump which you have given for the working case? If not, can you please capture the logs and register dump. Also which platform is this? If it is live status related issue, we need to see how long it is taking to set the live status, or is it not setting it at all with the single-link cable? Computer DVI âââ DVI-D (Single Link)/HDMI cable âââ HDMI LG 23MP55HQ-P === works! ===Dual Link DVI-D cable with monitor that doesn't work with Single Link cable=== Computer DVI âââ DVI-D (Dual Link)/HDMI cable âââ HDMI LG 23MP65HQ-P === works! >>> Funky. Can you pls grab the debug logs (with the special patches from >>> Ville) for this case? I wonder why suddenly different cable and it >>> works. >>> >>> Also: Is this one of these older-ish screens where you must have a >>> dual-link cable to drive it at full resolution rate? >>> -Daniel >>> >>> ===Laptop with HDMI output=== Laptop HDMI âââ HDMI/HDMI cable âââ HDMI LG 23MP65HQ-P === works! I'd say that single link DVI cables are broken with new kernel, but one of monitors could work with such a cable. So I have no idea :(. Regards, Oleksandr. 15.02.2016 17:42, Daniel Vetter wrote: > The other downside is that it'll make us non-compliant, which was the > point of this entire ordeal: HDMI spec forbids us from starting any i2c > transactions when the hpd isn't signalling a present screen. > > So maybe we need to buy one of these broken screens. > > Oleksandr, what exact model are you using? And any chance that you could > test this on some other machine with intel gfx and latest kernel, just to > make sure this really is some issue with the sink and not with the machine > itself? And I guess you've tested with some other hdmi sink, and that > works?
PROBLEM: Intel HDMI output busticated on 4.4 (regression)
On 1/21/2016 8:59 AM, Nick Bowler wrote: > On 1/20/16, Nick Bowler wrote: >> Hi, >> >> On 2016-01-20, Jindal, Sonika wrote: >>> Can you please check if you have following patch: >>> "commit 3d8acd1f667b45c531401c8f0c2033072e32a05d >>> Author: Gary Wang >>> Date: Wed Dec 23 16:11:35 2015 +0800 >>> >>> drm/i915: increase the tries for HDMI hotplug live status checking" >> Yes, that patch seems to be present in 4.4. >> >>> Does the same system works with any other monitor? >> I'll see if I can find another to try. > I tried another monitor, and the same problem occurs. Which make are these monitors? Do you have any other system other than G45? Shashank, Do you suggest anything else which Nick can try?
PROBLEM: Intel HDMI output busticated on 4.4 (regression)
Can you please check if you have following patch:
"commit 3d8acd1f667b45c531401c8f0c2033072e32a05d
Author: Gary Wang
Date: Wed Dec 23 16:11:35 2015 +0800
drm/i915: increase the tries for HDMI hotplug live status checking"
For some monitors, 30ms delay is not good enough to report the live
status correctly.
Does the same system works with any other monitor?
Daniel,
Can this be related to the live status bits on g45?
"commit 0ce99f749b3834edeb500e17d6ad17e86b60ff83
Author: Daniel Vetter
Date: Fri Jul 26 11:27:49 2013 +0200
drm/i915: fix gen4 digital port hotplug definitions"
Regards,
Sonika
On 1/20/2016 3:56 AM, Nick Bowler wrote:
> Hi,
>
> Upgrading from 4.3 to 4.4 breaks my HDMI output on my G45 machine.
>
> As soon as the intel driver is loaded, the monitor shuts off
> (standby mode). Inspecting /sys/class/drm/card0-HDMI-A-1/status
> reports "disconnected". When it is working, this attribute says
> "connected".
>
> There is nothing unusual printed to dmesg.
>
> Bisection pinpoints the following:
>
>237ed86c693d8a8e4db476976aeb30df4deac74b is the first bad commit
>commit 237ed86c693d8a8e4db476976aeb30df4deac74b
>Author: Sonika Jindal
>Date: Tue Sep 15 09:44:20 2015 +0530
>
>drm/i915: Check live status before reading edid
>[...]
>Signed-off-by: Shashank Sharma
>Signed-off-by: Sonika Jindal
>Reviewed-by: Rodrigo Vivi
>Signed-off-by: Daniel Vetter
>
> The commit does not revert cleanly, but this patch resolves the issue:
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index e6c035b0fc1c..8cefb9105f26 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1393,7 +1393,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool
> force)
>
> intel_hdmi_unset_edid(connector);
>
> - if (intel_hdmi_set_edid(connector, live_status)) {
> + if (intel_hdmi_set_edid(connector, true)) {
> struct intel_hdmi *intel_hdmi =
> intel_attached_hdmi(connector);
>
> hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
>
> Let me know if you need any more info.
>
> Thanks,
>Nick
[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
On 6/4/2015 7:12 PM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
> platform
> 2. Adds Gamma correction macros/defines
> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
> old blob for the property with the new one
> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>
> v2: Addressed all review comments from Sonika and Daniel Stone.
Instead you can mention the changes briefly.
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> drivers/gpu/drm/i915/intel_atomic.c| 6 ++
> drivers/gpu/drm/i915/intel_color_manager.c | 161
> +
> drivers/gpu/drm/i915/intel_color_manager.h | 62 +++
> 3 files changed, 229 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index b5c78d8..4726847 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -428,6 +428,12 @@ intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> struct drm_property *property,
> uint64_t val)
> {
> + struct drm_device *dev = crtc->dev;
> + struct drm_mode_config *config = >mode_config;
> +
> + if (property == config->gamma_property)
> + return intel_color_manager_set_gamma(dev, >base, val);
> +
> DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> return -EINVAL;
> }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index 8d4ee8f..421c267 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,167 @@
>
> #include "intel_color_manager.h"
>
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> + struct drm_crtc *crtc)
> +{
> + struct drm_gamma *gamma_data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_property_blob *blob;
> + struct drm_mode_config *config = >mode_config;
> + u32 cgm_control_reg = 0;
> + u32 cgm_gamma_reg = 0;
> + u32 reg, val;
> + enum pipe pipe;
> + u16 red, green, blue;
> + struct rgb_pixel correct_rgb;
> + u32 count = 0;
> + struct rgb_pixel *correction_values = NULL;
> + u32 num_samples;
> + u32 word;
> + u32 palette;
> + int ret = 0, length;
> +
> + blob = drm_property_lookup_blob(dev, blob_id);
> + if (!blob) {
> + DRM_ERROR("Invalid Blob ID\n");
> + return -EINVAL;
> + }
> +
> + gamma_data = (struct drm_gamma *)blob->data;
> + pipe = to_intel_crtc(crtc)->pipe;
> + num_samples = gamma_data->num_samples;
> + length = num_samples * sizeof(struct rgb_pixel);
> +
> + if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
Switch/case instead?
Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> +
> + /* Disable Gamma functionality on Pipe - CGM Block */
> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> + cgm_control_reg &= ~CGM_GAMMA_EN;
> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> + DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> + pipe_name(pipe));
> + ret = 0;
> + } else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> + if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> + DRM_ERROR("Incorrect number of samples received\n");
> + return -EINVAL;
> + }
> +
> + /* First, disable CGM Gamma, if already set */
> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> + cgm_control_reg &= ~CGM_GAMMA_EN;
> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> + /* Enable (Legacy) Gamma on Pipe */
> + palette = _PIPE_GAMMA_BASE(pipe);
> +
> + count = 0;
> + correction_values = (struct rgb_pixel *)_data->values;
> + while (count < num_samples) {
> + correct_rgb = correction_values[count];
> + blue = correction_values[count].blue;
> + green = correction_values[count].green;
> + red = correction_values[count].red;
> +
> + blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> + green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> + red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> + /* Red (23:16), Green (15:8), Blue (7:0) */
> + word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> + (green <<
> + CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> +
[PATCH v2 04/10] drm: Add Gamma correction structure
On 6/4/2015 7:12 PM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> This patch adds a new structure in DRM layer for Gamma color correction.
> This structure will be used by all user space agents to configure
> appropriate Gamma precision and Gamma level.
>
> struct drm_intel_gamma {
> __u32 gamma_level;
> (The gamma_level variable indicates if the Gamma correction is to be
Do you have any macro defines for these levels? Maybe an enum will be
better?
> applied on Pipe/plane)
> __u32 gamma_precision;
> (The Gamma precision indicates the Gamma mode to be applied)
>
> Supported precisions are -
> #define I915_GAMMA_PRECISION_UNKNOWN0
> #define I915_GAMMA_PRECISION_CURRENT0x
> #define I915_GAMMA_PRECISION_LEGACY (1 << 0)
> #define I915_GAMMA_PRECISION_10BIT (1 << 1)
> #define I915_GAMMA_PRECISION_12BIT (1 << 2)
> #define I915_GAMMA_PRECISION_14BIT (1 << 3)
> #define I915_GAMMA_PRECISION_16BIT (1 << 4)
>
> __u32 num_samples;
> (The num_samples indicates the number of Gamma correction
> coefficients)
> __u32 reserved;
You need this reserved?
> __u16 values[0];
> (An array of size 0, to accommodate the "num_samples" number of
> R16G16B16 pixels, dynamically)
> };
>
> v2: Addressing Daniel Stone's comment, added a variable sized array to
> carry Gamma correction values as blob property.
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> include/drm/drm_crtc.h | 3 +++
> include/uapi/drm/drm.h | 10 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2a75d7d..bc44f27 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -483,6 +483,9 @@ struct drm_crtc {
>* acquire context.
>*/
> struct drm_modeset_acquire_ctx *acquire_ctx;
> +
Just trying to understand, why do we need to store the blob id separately?
> + /* Color Management Blob IDs */
> + u32 gamma_blob_id;
> };
>
> /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..fc2661c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,16 @@ struct drm_event_vblank {
> __u32 reserved;
> };
>
> +/* Color Management structure for Gamma */
> +struct drm_gamma {
> + __u32 flags;
> + __u32 gamma_level;
> + __u32 gamma_precision;
> + __u32 num_samples;
> + __u32 reserved;
> + __u16 values[0];
> +};
> +
> /* typedef area */
> #ifndef __KERNEL__
> typedef struct drm_clip_rect drm_clip_rect_t;
>
[PATCH v2 00/10] Color Manager Implementation
HI Kausal, You don't need to send the entire series again . Just send the updated patch --in-reply-to to the last message. Otherwise the thread gets lost. You can send the entire series once the review is complete and you feel that the patches are too nested inside. Please keep sending the patches using --in-reply-to tag. Regards, Sonika -Original Message- From: Malladi, Kausal Sent: Thursday, June 4, 2015 7:13 PM To: Roper, Matthew D; Barnes, Jesse; Lespiau, Damien; Jindal, Sonika; R, Durgadoss; Purushothaman, Vijay A; intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org Cc: Vetter, Daniel; Sharma, Shashank; Kamath, Sunil; Mukherjee, Indranil; Matheson, Annie J; R, Dhanya p; Palleti, Avinash Reddy; Malladi, Kausal Subject: [PATCH v2 00/10] Color Manager Implementation From: Kausal Malladi <kausal.mall...@intel.com> This patch set adds color manager implementation in drm/i915 layer. Color Manager is an extension in i915 driver to support color correction/enhancement. Various Intel platforms support several color correction capabilities. Color Manager provides abstraction of these properties and allows a user space UI agent to correct/enhance the display. The first three patches add code for the framework, which will be common across all the Intel platforms. drm/i915: Initialize Color Manager drm/i915: Attach color properties to CRTC drm/i915: Add Set property interface for CRTC In the next patches, we are adding support for Gamma and CSC color properties. Please note that: 1. The current implementation is only limited for CHV/BSW platforms, and the support for other platforms will be following up soon. 2. The current patch set implements only the "set" part of the properties, "get" part will be following up soon. The design of color management on linux is done by: Jesse Barnes Sirisha Muppavarapu Shashank Sharma Susanta Bhattacharjee The complete design is explained in the design document, which is available at https://docs.google.com/document/d/1jyfNSAStKHEpmIUZd_1Gd68cPuyiyr8wmJXThSDb_2w/edit# v2: Addressed review comments from Sonika and Daniel Stone. Kausal Malladi (10): drm/i915: Initialize Color Manager drm/i915: Attach color properties to CRTC drm/i915: Add atomic set property interface for CRTC drm: Add Gamma correction structure drm: Add a new function for updating color blob drm: Avoid atomic commit path for CRTC property (Gamma) drm/i915: Add pipe level Gamma correction for CHV/BSW drm: Add CSC correction structure drm: Avoid atomic commit path for CSC property on CRTC drm/i915: Add CSC support for CHV/BSW drivers/gpu/drm/drm_atomic_helper.c| 18 +- drivers/gpu/drm/drm_crtc.c | 15 ++ drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/intel_atomic.c| 19 +- drivers/gpu/drm/i915/intel_color_manager.c | 348 + drivers/gpu/drm/i915/intel_color_manager.h | 122 ++ drivers/gpu/drm/i915/intel_display.c | 8 + drivers/gpu/drm/i915/intel_drv.h | 4 + include/drm/drm_crtc.h | 12 + include/uapi/drm/drm.h | 18 ++ 10 files changed, 563 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h -- 2.4.2
[PATCH 5/7] drm/i915: Add pipe level Gamma correction for CHV/BSW
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
> platform
> 2. Adds Gamma correction macros/defines
> 3. Adds drm_mode_crtc_update_color_property function, which replaces the
> old blob for the property with the new one
> 4. Adds a pointer to hold blob for Gamma property in drm_crtc
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> drivers/gpu/drm/drm_crtc.c | 14 ++
> drivers/gpu/drm/i915/intel_color_manager.c | 194
>
> drivers/gpu/drm/i915/intel_color_manager.h | 61 +
> drivers/gpu/drm/i915/intel_display.c |9 +-
> include/drm/drm_crtc.h |8 ++
> 5 files changed, 285 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 77f87b2..50b925b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4691,6 +4691,20 @@ int drm_mode_connector_set_tile_property(struct
> drm_connector *connector)
> }
> EXPORT_SYMBOL(drm_mode_connector_set_tile_property);
>
> +int drm_mode_crtc_update_color_property(struct drm_device *dev,
> + struct drm_property_blob **blob,
> + size_t length, const void *color_data,
> + struct drm_mode_object *obj_holds_id,
> + struct drm_property *prop_holds_id)
This can be simplified.. No need to pass so many params.
> +{
> + int ret;
> +
> + ret = drm_property_replace_global_blob(dev,
> + blob, length, color_data, obj_holds_id, prop_holds_id);
> +
> + return ret;
> +}
> +
Split the patch to add drm specific changes in a separate patch. Also
you need to export this function.
Will review the rest of the file in some time.
Regards,
Sonika
> /**
>* drm_mode_connector_update_edid_property - update the edid property of a
> connector
>* @connector: drm connector
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index b0eb679..f46857f 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,200 @@
>
> #include "intel_color_manager.h"
>
> +int chv_set_gamma(struct drm_device *dev, uint64_t blob_id,
> + struct drm_crtc *crtc)
> +{
> + struct drm_intel_gamma *gamma_data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_property_blob *blob;
> + struct drm_mode_config *config = >mode_config;
> +
> + u32 cgm_control_reg = 0;
> + u32 cgm_gamma_reg = 0;
> + u32 reg, val, pipe;
> + u16 red, green, blue;
> + struct rgb_pixel correct_rgb;
> + u32 count = 0;
> + struct rgb_pixel *correction_values = NULL;
> + u32 num_samples;
> + u32 word;
> + u32 palette;
> + int ret = 0, length;
> +
> + blob = drm_property_lookup_blob(dev, blob_id);
> + if (!blob) {
> + DRM_ERROR("Invalid Blob ID\n");
> + return -EINVAL;
> + }
> +
> + gamma_data = kzalloc(sizeof(struct drm_intel_gamma), GFP_KERNEL);
> + if (!gamma_data) {
> + DRM_ERROR("Memory unavailable\n");
> + return -ENOMEM;
> + }
> + gamma_data = (struct drm_intel_gamma *)blob->data;
> + pipe = to_intel_crtc(crtc)->pipe;
> + num_samples = gamma_data->num_samples;
> + length = num_samples * sizeof(struct rgb_pixel);
> +
> + if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> + /* Disable Gamma functionality on Pipe - CGM Block */
> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> + cgm_control_reg &= ~CGM_GAMMA_EN;
> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> + DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> + pipe_name(pipe));
> + ret = 0;
> + goto release_memory;
> + }
> +
> + if (pipe >= CHV_MAX_PIPES) {
> + DRM_ERROR("Incorrect Pipe ID\n");
> + ret = -EFAULT;
> + goto release_memory;
> + }
> +
> + correction_values = kzalloc(length, GFP_KERNEL);
> + if (!correction_values) {
> + DRM_ERROR("Out of Memory\n");
> + ret = -ENOMEM;
> + goto release_memory;
> + }
> +
> + ret = copy_from_user((void *)correction_values,
> + (const void __user *)gamma_data->gamma_ptr, length);
> + if (ret) {
> + DRM_ERROR("Error copying user data\n");
> + ret = -EFAULT;
> + goto release_memory;
> + }
> +
> + ret = drm_mode_crtc_update_color_property(dev,
> + >gamma_blob, length, (void *) correction_values,
> + >base, config->gamma_property);
> + if (ret) {
> + DRM_ERROR("Error updating
[PATCH 4/7] drm: Add Gamma correction structure
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> This patch adds a new structure in DRM layer for Gamma color correction.
> This structure will be used by all user space agents to configure
> appropriate Gamma precision and Gamma level.
>
> struct drm_intel_gamma {
> __u32 flags;
> (The flag variable will indicate if the property to be set/get
> is Gamma or DeGamma)
> __u32 gamma_level;
> (The gamma_level variable indicates if the Gamma correction is to be
> applied on Pipe/plane)
> __u32 gamma_precision;
> (The Gamma precision indicates the Gamma mode to be applied)
>
> Supported precisions are -
> #define I915_GAMMA_PRECISION_UNKNOWN0
> #define I915_GAMMA_PRECISION_CURRENT0x
> #define I915_GAMMA_PRECISION_LEGACY (1 << 0)
> #define I915_GAMMA_PRECISION_10BIT (1 << 1)
> #define I915_GAMMA_PRECISION_12BIT (1 << 2)
> #define I915_GAMMA_PRECISION_14BIT (1 << 3)
> #define I915_GAMMA_PRECISION_16BIT (1 << 4)
>
> __u32 num_samples;
> (The num_samples indicates the number of Gamma correction
> coefficients)
> __u32 reserved;
> __u64 gamma_ptr;
> (Points to the raw Gamma color correction values)
> };
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> include/uapi/drm/drm.h | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..fe27e5c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,17 @@ struct drm_event_vblank {
> __u32 reserved;
> };
>
> +/* Color Management structure for Gamma */
> +struct drm_intel_gamma {
I suppose, this can be used by other drivers as well? If yes, "intel"
can be removed.
> + __u32 obj_id;
> + __u32 flags;
> + __u32 gamma_level;
> + __u32 gamma_precision;
> + __u32 num_samples;
> + __u32 reserved;
> + __u64 gamma_ptr;
> +};
> +
> /* typedef area */
> #ifndef __KERNEL__
> typedef struct drm_clip_rect drm_clip_rect_t;
>
[PATCH 3/7] drm/i915: Add Set property interface for CRTC
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> This patch adds set property interface for Intel CRTC. This interface
> will be used to set color correction DRM properties.
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> drivers/gpu/drm/i915/intel_display.c |8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f817cea..21e67da 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12978,11 +12978,19 @@ out:
> return ret;
> }
>
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> + struct drm_property *property, uint64_t val)
> +{
> + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> + return -EINVAL;
> +}
> +
> static const struct drm_crtc_funcs intel_crtc_funcs = {
> .gamma_set = intel_crtc_gamma_set,
> .set_config = intel_crtc_set_config,
> .destroy = intel_crtc_destroy,
> .page_flip = intel_crtc_page_flip,
> + .set_property = intel_crtc_set_property,
I think it should be done similar to plane set property using atomic
helpers.
> .atomic_duplicate_state = intel_crtc_duplicate_state,
> .atomic_destroy_state = intel_crtc_destroy_state,
> };
>
[PATCH 1/7] drm/i915: Initialize Color Manager
On 6/2/2015 1:22 AM, Kausal Malladi wrote:
> From: Kausal Malladi
>
> Color Manager is an extension in i915 driver to handle color correction
> and enhancements across various Intel platforms.
>
> This patch initializes color manager framework by :
> 1. Adding two new files, intel_color_manager(.c/.h)
> 2. Introducing new pointers in DRM mode_config structure to
> carry CSC and Gamma color correction properties.
> 3. Creating these DRM properties in Color Manager initialization
> sequence.
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Kausal Malladi
> ---
> drivers/gpu/drm/i915/Makefile |3 ++
> drivers/gpu/drm/i915/intel_color_manager.c | 49
>
> drivers/gpu/drm/i915/intel_color_manager.h | 32 ++
> drivers/gpu/drm/i915/intel_display.c |5 +++
> include/drm/drm_crtc.h |4 +++
> 5 files changed, 93 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
> create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b7ddf48..c62d048 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
> # legacy horrors
> i915-y += i915_dma.o
>
> +# Color Management
> +i915-y += intel_color_manager.o
> +
> obj-$(CONFIG_DRM_I915) += i915.o
>
> CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
> b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 000..c83a212
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Shashank Sharma
> + * Kausal Malladi
> + */
> +
> +#include "intel_color_manager.h"
> +
> +int intel_color_manager_init(struct drm_device *dev)
> +{
> + struct drm_mode_config *config = >mode_config;
> +
> + /* Create Gamma and CSC properties */
> + config->gamma_property = drm_property_create(dev,
> + DRM_MODE_PROP_BLOB, "gamma_property", 0);
> + if (!config->gamma_property)
> + DRM_ERROR("Gamma property creation failed\n");
> +
> + DRM_DEBUG_DRIVER("Created Gamma property\n");
> +
> + config->csc_property = drm_property_create(dev,
> + DRM_MODE_PROP_BLOB, "csc_property", 0);
> + if (!config->csc_property)
> + DRM_ERROR("CSC property creation failed\n");
> +
> + DRM_DEBUG_DRIVER("Created CSC property\n");
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h
> b/drivers/gpu/drm/i915/intel_color_manager.h
> new file mode 100644
> index 000..3cff09d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
[Intel-gfx] [PATCH] Documentation/drm: Update rotation property with 90/270 and description
On 5/13/2015 9:57 AM, Jindal, Sonika wrote:
>
>
> On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
>> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>>> Cc: dri-devel at lists.freedesktop.org
>>> Signed-off-by: Sonika Jindal
>>> ---
>>> Documentation/DocBook/drm.tmpl |7 +--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/drm.tmpl
>>> b/Documentation/DocBook/drm.tmpl
>>> index f4976cd..266d50a 100644
>>> --- a/Documentation/DocBook/drm.tmpl
>>> +++ b/Documentation/DocBook/drm.tmpl
>>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>> Plane
>>> ârotationâ
>>> BITMASK
>>> -{ 0, "rotate-0" }, { 2, "rotate-180" }
>>> +{ 0, "rotate-0" }, { 1, "rotate-90" },
>>> +{ 2, "rotate-180" }, { 3, "rotate-270" }
>>> Plane
>>> -TBD
>>> +To set plane HW rotation. This rotation
>>> property does
>>> +the plane rotation in counter clockwise direction which is
>>> +inline with the way XRandr works.
>>
>> I would suggest moving the thing to the generci props section since we
>> have omap and i915 both supporting it.
> You mean in DRM properties section?
> Right now, OMAP section also has rotation property. I will remove it
> from OMAP section as well if you think drm is the better place.
>
>>
>> As for the description, we should also document the reflect flags.
>>
i915 doesn't support reflect flags. It only create rotation property
with rotation flags.
For "generic" section, you mean moving to generic group of properties in
i915 only right?
>> I might write it as something like this:
>> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
>> specified amount in degrees in a counter clockwise direction.
>> reflect-x,reflect-y reflect the image along the specified axis,
>> prior to rotation."
>>
>>>
>>>
>>> SDVO-TV
>>> --
>>> 1.7.10.4
>>>
>>> ___
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
[Intel-gfx] [PATCH] Documentation/drm: Update rotation property with 90/270 and description
On 5/13/2015 9:57 AM, Jindal, Sonika wrote:
>
>
> On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
>> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>>> Cc: dri-devel at lists.freedesktop.org
>>> Signed-off-by: Sonika Jindal
>>> ---
>>> Documentation/DocBook/drm.tmpl |7 +--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/drm.tmpl
>>> b/Documentation/DocBook/drm.tmpl
>>> index f4976cd..266d50a 100644
>>> --- a/Documentation/DocBook/drm.tmpl
>>> +++ b/Documentation/DocBook/drm.tmpl
>>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>> Plane
>>> ârotationâ
>>> BITMASK
>>> -{ 0, "rotate-0" }, { 2, "rotate-180" }
>>> +{ 0, "rotate-0" }, { 1, "rotate-90" },
>>> +{ 2, "rotate-180" }, { 3, "rotate-270" }
>>> Plane
>>> -TBD
>>> +To set plane HW rotation. This rotation
>>> property does
>>> +the plane rotation in counter clockwise direction which is
>>> +inline with the way XRandr works.
>>
>> I would suggest moving the thing to the generci props section since we
>> have omap and i915 both supporting it.
> You mean in DRM properties section?
> Right now, OMAP section also has rotation property. I will remove it
> from OMAP section as well if you think drm is the better place.
>
>>
>> As for the description, we should also document the reflect flags.
>>
Also, i915 doesn't support reflect flags. Only rotation is supported there.
For the "generic" part, you meant just moving to the generic group in
i915 property section?
>> I might write it as something like this:
>> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
>> specified amount in degrees in a counter clockwise direction.
>> reflect-x,reflect-y reflect the image along the specified axis,
>> prior to rotation."
>>
>>>
>>>
>>> SDVO-TV
>>> --
>>> 1.7.10.4
>>>
>>> ___
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
[Intel-gfx] [PATCH] Documentation/drm: Update rotation property with 90/270 and description
On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>> Cc: dri-devel at lists.freedesktop.org
>> Signed-off-by: Sonika Jindal
>> ---
>> Documentation/DocBook/drm.tmpl |7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> index f4976cd..266d50a 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>> Plane
>> ârotationâ
>> BITMASK
>> -{ 0, "rotate-0" }, { 2, "rotate-180" }
>> +{ 0, "rotate-0" }, { 1, "rotate-90" },
>> +{ 2, "rotate-180" }, { 3, "rotate-270" }
>> Plane
>> -TBD
>> +To set plane HW rotation. This rotation property does
>> +the plane rotation in counter clockwise direction which is
>> +inline with the way XRandr works.
>
> I would suggest moving the thing to the generci props section since we
> have omap and i915 both supporting it.
You mean in DRM properties section?
Right now, OMAP section also has rotation property. I will remove it
from OMAP section as well if you think drm is the better place.
>
> As for the description, we should also document the reflect flags.
>
> I might write it as something like this:
> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
> specified amount in degrees in a counter clockwise direction.
> reflect-x,reflect-y reflect the image along the specified axis,
> prior to rotation."
>
>>
>>
>> SDVO-TV
>> --
>> 1.7.10.4
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
[PATCH v2] drm/dp: add DPCD definitions from eDP 1.4
Reviewed-by: Sonika Jindal On 2/27/2015 4:41 PM, Jani Nikula wrote: > Add a number of DPCD definitions from eDP 1.4. > > v2: s/DP_ALPM_LOCK_TIMEOUT_ERROR_STATUS/DP_ALPM_LOCK_TIMEOUT_ERROR/ > (Sonika) > > Signed-off-by: Jani Nikula > --- > include/drm/drm_dp_helper.h | 37 + > 1 file changed, 37 insertions(+) > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 319d5edfb3b5..c5fdc2d3ca97 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -168,10 +168,18 @@ > #define DP_AUD_DEL_INS2 0x02d > /* End of AV_SYNC_DATA_BLOCK */ > > +#define DP_RECEIVER_ALPM_CAP 0x02e /* eDP 1.4 */ > +# define DP_ALPM_CAP (1 << 0) > + > +#define DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP 0x02f /* eDP 1.4 */ > +# define DP_AUX_FRAME_SYNC_CAP (1 << 0) > + > #define DP_GUID 0x030 /* 1.2 */ > > #define DP_PSR_SUPPORT 0x070 /* XXX 1.2? */ > # define DP_PSR_IS_SUPPORTED1 > +# define DP_PSR2_IS_SUPPORTED2 /* eDP 1.4 */ > + > #define DP_PSR_CAPS 0x071 /* XXX 1.2? */ > # define DP_PSR_NO_TRAIN_ON_EXIT1 > # define DP_PSR_SETUP_TIME_330 (0 << 1) > @@ -211,6 +219,7 @@ > > /* link configuration */ > #define DP_LINK_BW_SET 0x100 > +# define DP_LINK_RATE_TABLE 0x00/* eDP 1.4 */ > # define DP_LINK_BW_1_620x06 > # define DP_LINK_BW_2_7 0x0a > # define DP_LINK_BW_5_4 0x14/* 1.2 */ > @@ -307,15 +316,30 @@ > #define DP_AUDIO_DELAY2 0x114 > > #define DP_LINK_RATE_SET0x115 /* eDP 1.4 */ > +# define DP_LINK_RATE_SET_SHIFT 0 > +# define DP_LINK_RATE_SET_MASK (7 << 0) > + > +#define DP_RECEIVER_ALPM_CONFIG 0x116 /* eDP 1.4 */ > +# define DP_ALPM_ENABLE (1 << 0) > +# define DP_ALPM_LOCK_ERROR_IRQ_HPD_ENABLE (1 << 1) > + > +#define DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF 0x117 /* eDP 1.4 */ > +# define DP_AUX_FRAME_SYNC_ENABLE(1 << 0) > +# define DP_IRQ_HPD_ENABLE (1 << 1) > > #define DP_UPSTREAM_DEVICE_DP_PWR_NEED 0x118 /* 1.2 */ > # define DP_PWR_NOT_NEEDED (1 << 0) > > +#define DP_AUX_FRAME_SYNC_VALUE 0x15c /* eDP 1.4 */ > +# define DP_AUX_FRAME_SYNC_VALID (1 << 0) > + > #define DP_PSR_EN_CFG 0x170 /* XXX 1.2? */ > # define DP_PSR_ENABLE (1 << 0) > # define DP_PSR_MAIN_LINK_ACTIVE(1 << 1) > # define DP_PSR_CRC_VERIFICATION(1 << 2) > # define DP_PSR_FRAME_CAPTURE (1 << 3) > +# define DP_PSR_SELECTIVE_UPDATE (1 << 4) > +# define DP_PSR_IRQ_HPD_WITH_CRC_ERRORS (1 << 5) > > #define DP_ADAPTER_CTRL 0x1a0 > # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE (1 << 0) > @@ -423,6 +447,10 @@ > # define DP_SET_POWER_MASK 0x3 > > #define DP_EDP_DPCD_REV 0x700/* eDP 1.2 */ > +# define DP_EDP_11 0x00 > +# define DP_EDP_12 0x01 > +# define DP_EDP_13 0x02 > +# define DP_EDP_14 0x03 > > #define DP_EDP_GENERAL_CAP_10x701 > > @@ -430,6 +458,8 @@ > > #define DP_EDP_GENERAL_CAP_20x703 > > +#define DP_EDP_GENERAL_CAP_3 0x704/* eDP 1.4 */ > + > #define DP_EDP_DISPLAY_CONTROL_REGISTER 0x720 > > #define DP_EDP_BACKLIGHT_MODE_SET_REGISTER 0x721 > @@ -456,6 +486,9 @@ > #define DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET 0x732 > #define DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET 0x733 > > +#define DP_EDP_REGIONAL_BACKLIGHT_BASE 0x740/* eDP 1.4 */ > +#define DP_EDP_REGIONAL_BACKLIGHT_0 0x741/* eDP 1.4 */ > + > #define DP_SIDEBAND_MSG_DOWN_REQ_BASE 0x1000 /* 1.2 MST */ > #define DP_SIDEBAND_MSG_UP_REP_BASE 0x1200 /* 1.2 MST */ > #define DP_SIDEBAND_MSG_DOWN_REP_BASE 0x1400 /* 1.2 MST */ > @@ -474,6 +507,7 @@ > #define DP_PSR_ERROR_STATUS 0x2006 /* XXX 1.2? */ > # define DP_PSR_LINK_CRC_ERROR (1 << 0) > # define DP_PSR_RFB_STORAGE_ERROR (1 << 1) > +# define DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR (1 << 2) /* eDP 1.4 */ > > #define DP_PSR_ESI 0x2007 /* XXX 1.2? */ > # define DP_PSR_CAPS_CHANGE (1 << 0) > @@ -487,6 +521,9 @@ > # define DP_PSR_SINK_INTERNAL_ERROR 7 > # define DP_PSR_SINK_STATE_MASK 0x07 > > +#define DP_RECEIVER_ALPM_STATUS 0x200b /* eDP 1.4 */ > +# define DP_ALPM_LOCK_TIMEOUT_ERROR (1 << 0) > + > /* DP 1.2
[Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there. That's how it gets right coordinates and w,h for sprite for 90 rotation. I thought drm_rect_rotate and other functions are there to handle such scenarios. -Original Message- From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com] Sent: Wednesday, January 14, 2015 11:38 PM To: Jindal, Sonika Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update On Wed, Jan 14, 2015 at 01:54:06PM +, Jindal, Sonika wrote: > I think I am confused here.. > Since sprite plane handles this scaling and rotation in kernel, I thought it > should be taken care in driver for primary as well. Well, userspace still has to know how the src and dst coordinates relate to each other. Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc. Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out. Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC. It should look something like this: rotation=0 src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2) dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2) FB:CRTC: 0,0 0,0 |abcd| | abcd | |efgh| | efgh | 4,2 || || 8,4 rotation=90 src=0,0 -> 4,2 (w=4, h=2) dst=2,0 -> 4,4 (w=2, h=4) FB:CRTC: 0,0 0,0 |abcd| | dh| |efgh| | cg| 4,2 | bf| | ae| 8,4 > >From the test, I don't really change the src sizes for primary as well as > >sprite to take care of 90/270 rotation. Sounds a bit buggy then. > > -Original Message- > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com] > Sent: Wednesday, January 14, 2015 5:14 PM > To: Jindal, Sonika > Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 09:49:36AM +, Jindal, Sonika wrote: > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser > > than the min_hscale (which is no_scaling by default), so it returns -ERANGE. > > If you want no scaling then with 90/270 rotation then your src->w should be > equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then > your test is passing in bad coordinates. > > > So, I think we _relaxed is the function which should be called to update > > the destination size appropriately. > > Please correct me if I am wrong. > > > > -Original Message- > > From: Jindal, Sonika > > Sent: Wednesday, January 14, 2015 3:06 PM > > To: 'Ville Syrjälä' > > Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > We do exactly like this for sprite plane (ie, rotate the rect, then check > > scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) > > That's why I saw the need of this for primary plane as well. > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments > > and the rotated sizes are fine. But since we don't do any of those stuff > > for primary the destination size doesn't come right, and I get a little > > corrupted output after rotation. > > With this change, the rotated plane is properly adjusted in the viewport. > > So, I don't think it is a bug in test. > > > > -Original Message- > > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com] > > Sent: Wednesday, January 14, 2015 2:58 PM > > To: Jindal, Sonika > > Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > > >> Taking rota
[Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
I think I am confused here..
Since sprite plane handles this scaling and rotation in kernel, I thought it
should be taken care in driver for primary as well.
>From the test, I don't really change the src sizes for primary as well as
>sprite to take care of 90/270 rotation.
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
Sent: Wednesday, January 14, 2015 5:14 PM
To: Jindal, Sonika
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
drm_plane_helper_check_update
On Wed, Jan 14, 2015 at 09:49:36AM +, Jindal, Sonika wrote:
> Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than
> the min_hscale (which is no_scaling by default), so it returns -ERANGE.
If you want no scaling then with 90/270 rotation then your src->w should be
equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your
test is passing in bad coordinates.
> So, I think we _relaxed is the function which should be called to update the
> destination size appropriately.
> Please correct me if I am wrong.
>
> -Original Message-
> From: Jindal, Sonika
> Sent: Wednesday, January 14, 2015 3:06 PM
> To: 'Ville Syrjälä'
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
> drm_plane_helper_check_update
>
> We do exactly like this for sprite plane (ie, rotate the rect, then check
> scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed)
> That's why I saw the need of this for primary plane as well.
> For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments
> and the rotated sizes are fine. But since we don't do any of those stuff for
> primary the destination size doesn't come right, and I get a little corrupted
> output after rotation.
> With this change, the rotated plane is properly adjusted in the viewport.
> So, I don't think it is a bug in test.
>
> -Original Message-
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Wednesday, January 14, 2015 2:58 PM
> To: Jindal, Sonika
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
> drm_plane_helper_check_update
>
> On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> >
> > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > >> Taking rotation into account while checking the plane and
> > >> adjusting the sizes accordingly.
> > >>
> > >> Signed-off-by: Sonika Jindal
> > >> ---
> > >> drivers/gpu/drm/drm_plane_helper.c | 79
> > >> ++--
> > >> include/drm/drm_plane_helper.h |3 +-
> > >> 2 files changed, 77 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > >> b/drivers/gpu/drm/drm_plane_helper.c
> > >> index f24c4cf..4badd69 100644
> > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane
> > >> *plane,
> > >> int max_scale,
> > >> bool can_position,
> > >> bool can_update_disabled,
> > >> -bool *visible)
> > >> +bool *visible,
> > >> +unsigned int rotation)
> > >> {
> > >> int hscale, vscale;
> > >> +int crtc_x, crtc_y;
> > >> +unsigned int crtc_w, crtc_h;
> > >> +uint32_t src_x, src_y, src_w, src_h;
> > >>
> > >> if (!fb) {
> > >> *visible = false;
> > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane
> > >> *plane,
> > >> return -EINVAL;
> > >> }
> > >>
> > >> +if (fb)
> > >> +drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > >> +rotation);
> > >> +
> > >&g
[Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than
the min_hscale (which is no_scaling by default), so it returns -ERANGE.
So, I think we _relaxed is the function which should be called to update the
destination size appropriately.
Please correct me if I am wrong.
-Original Message-
From: Jindal, Sonika
Sent: Wednesday, January 14, 2015 3:06 PM
To: 'Ville Syrjälä'
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
drm_plane_helper_check_update
We do exactly like this for sprite plane (ie, rotate the rect, then check
scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed)
That's why I saw the need of this for primary plane as well.
For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and
the rotated sizes are fine. But since we don't do any of those stuff for
primary the destination size doesn't come right, and I get a little corrupted
output after rotation.
With this change, the rotated plane is properly adjusted in the viewport.
So, I don't think it is a bug in test.
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
Sent: Wednesday, January 14, 2015 2:58 PM
To: Jindal, Sonika
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
drm_plane_helper_check_update
On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
>
> On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> >> Taking rotation into account while checking the plane and adjusting
> >> the sizes accordingly.
> >>
> >> Signed-off-by: Sonika Jindal
> >> ---
> >> drivers/gpu/drm/drm_plane_helper.c | 79
> >> ++--
> >> include/drm/drm_plane_helper.h |3 +-
> >> 2 files changed, 77 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index f24c4cf..4badd69 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane
> >> *plane,
> >>int max_scale,
> >>bool can_position,
> >>bool can_update_disabled,
> >> - bool *visible)
> >> + bool *visible,
> >> + unsigned int rotation)
> >> {
> >>int hscale, vscale;
> >> + int crtc_x, crtc_y;
> >> + unsigned int crtc_w, crtc_h;
> >> + uint32_t src_x, src_y, src_w, src_h;
> >>
> >>if (!fb) {
> >>*visible = false;
> >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane
> >> *plane,
> >>return -EINVAL;
> >>}
> >>
> >> + if (fb)
> >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> >> + rotation);
> >> +
> >>/* Check scaling */
> >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale,
> >> +max_scale);
> > This is an unrelated change. Relaxed scaling allows the the src/dest
> > rectangles to be reduced in size in order to keep the scaling ration
> > within the min/max range. I suppose we should switch to using it to
> > make the behaviour uniform across drivers, but definitely should be
> > done with a separate patch.
> Since, I added drm_rect_rotate before this, it changes the src sizes
> and it was giving me Invalid scaling if we don't let the sizes to be
> changed using _relaxed functions. I am trying this for 90/270
> rotation.
That would indicate a bug somewhere. Pontetially the bug could be in whatever
test you're using as well.
> I can move it to a separate patch if required.
We nee to figure out why you got the error first.
--
Ville Syrjälä
Intel OTC
[Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
We do exactly like this for sprite plane (ie, rotate the rect, then check
scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed)
That's why I saw the need of this for primary plane as well.
For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and
the rotated sizes are fine. But since we don't do any of those stuff
for primary the destination size doesn't come right, and I get a little
corrupted output after rotation.
With this change, the rotated plane is properly adjusted in the viewport.
So, I don't think it is a bug in test.
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
Sent: Wednesday, January 14, 2015 2:58 PM
To: Jindal, Sonika
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to
drm_plane_helper_check_update
On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
>
> On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> >> Taking rotation into account while checking the plane and adjusting
> >> the sizes accordingly.
> >>
> >> Signed-off-by: Sonika Jindal
> >> ---
> >> drivers/gpu/drm/drm_plane_helper.c | 79
> >> ++--
> >> include/drm/drm_plane_helper.h |3 +-
> >> 2 files changed, 77 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index f24c4cf..4badd69 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane
> >> *plane,
> >>int max_scale,
> >>bool can_position,
> >>bool can_update_disabled,
> >> - bool *visible)
> >> + bool *visible,
> >> + unsigned int rotation)
> >> {
> >>int hscale, vscale;
> >> + int crtc_x, crtc_y;
> >> + unsigned int crtc_w, crtc_h;
> >> + uint32_t src_x, src_y, src_w, src_h;
> >>
> >>if (!fb) {
> >>*visible = false;
> >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane
> >> *plane,
> >>return -EINVAL;
> >>}
> >>
> >> + if (fb)
> >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> >> + rotation);
> >> +
> >>/* Check scaling */
> >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale,
> >> +max_scale);
> > This is an unrelated change. Relaxed scaling allows the the src/dest
> > rectangles to be reduced in size in order to keep the scaling ration
> > within the min/max range. I suppose we should switch to using it to
> > make the behaviour uniform across drivers, but definitely should be
> > done with a separate patch.
> Since, I added drm_rect_rotate before this, it changes the src sizes
> and it was giving me Invalid scaling if we don't let the sizes to be
> changed using _relaxed functions. I am trying this for 90/270
> rotation.
That would indicate a bug somewhere. Pontetially the bug could be in whatever
test you're using as well.
> I can move it to a separate patch if required.
We nee to figure out why you got the error first.
--
Ville Syrjälä
Intel OTC
[PATCH 3/7] drm/exynos: Renaming DP training vswing pre emph defines
On 8/28/2014 11:36 AM, Jingoo Han wrote: > On Thursday, August 28, 2014 1:33 PM, Sonika Sonika wrote: >> On 8/28/2014 6:25 AM, Jingoo Han wrote: >>> On Friday, August 08, 2014 7:54 PM, Sonika Jindal wrote: From: Sonika Jindal Rename the defines to have levels instead of values for vswing and pre-emph levels as the values may differ in other scenarios like low vswing of eDP1.4 where the values are different. Done using following cocci patch for each define: @@ @@ # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) ... Signed-off-by: Sonika Jindal --- drivers/gpu/drm/exynos/exynos_dp_core.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c index 4f3c7eb..02602a8 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.c +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c @@ -329,8 +329,8 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp) return retval; for (lane = 0; lane < lane_count; lane++) - buf[lane] = DP_TRAIN_PRE_EMPHASIS_0 | - DP_TRAIN_VOLTAGE_SWING_400; + buf[lane] = DP_TRAIN_PRE_EMPH_LEVEL_0 | + DP_TRAIN_VOLTAGE_SWING_LEVEL_0; >>> >>> NAK! >>> >>> It makes build error. Please build your patch, before sending the patch. >>> It is a rule when submitting patches. >>> >>> Please, fix it as follows. >>> >>> + buf[lane] = DP_TRAIN_PRE_EMPHASIS_LEVEL_0| >>> + DP_TRAIN_VOLTAGE_SWING_LEVEL_0; >>> >> I think the first patch which you have taken (which adds new defines) is >> the one from the previous series for the same change. In the second >> version, I have named them as DP_TRAIN_PRE_EMPH_LEVEL_* which was done >> using cocci. Following is from that patch: > > Oh, I see. Sorry for annoying you. > However, how about tagging V2, V3.. into patches? For instance, > '[PATCH V2 3/7] drm/exynos: Renaming DP training vswing pre emph defines' > It will be helpful, in order to prevent the same mistakes happening again. > Actually I had bumped the version in the cover letter. Because last time I had changed the version of all the patches (for some other feature), somebody asked me to just change the version at the top when it is a series. Also, this was a different patch altogether done using cocci, so thought it should be fine. Will take care next time :) Thanks, Sonika > Also, the patch looks good. > Acked-by: Jingoo Han > > Best regards, > Jingoo Han > >># define DP_TRAIN_PRE_EMPHASIS_0 (0 << 3) >> +# define DP_TRAIN_PRE_EMPH_LEVEL_0 (0 << 3) >># define DP_TRAIN_PRE_EMPHASIS_3_5(1 << 3) >> +# define DP_TRAIN_PRE_EMPH_LEVEL_1 (1 << 3) >># define DP_TRAIN_PRE_EMPHASIS_6 (2 << 3) >> +# define DP_TRAIN_PRE_EMPH_LEVEL_2 (2 << 3) >># define DP_TRAIN_PRE_EMPHASIS_9_5(3 << 3) >> +# define DP_TRAIN_PRE_EMPH_LEVEL_3 (3 << 3) >>> Best regards, >>> Jingoo Han >>> retval = exynos_dp_write_bytes_to_dpcd(dp, DP_TRAINING_LANE0_SET, lane_count, buf); -- 1.7.10.4 >>> >
[PATCH 3/7] drm/exynos: Renaming DP training vswing pre emph defines
On 8/28/2014 6:25 AM, Jingoo Han wrote: > On Friday, August 08, 2014 7:54 PM, Sonika Jindal wrote: >> >> From: Sonika Jindal >> >> Rename the defines to have levels instead of values for vswing and >> pre-emph levels as the values may differ in other scenarios like low vswing >> of >> eDP1.4 where the values are different. >> >> Done using following cocci patch for each define: >> @@ >> @@ >> >> # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) >> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) >> >> ... >> >> Signed-off-by: Sonika Jindal >> --- >> drivers/gpu/drm/exynos/exynos_dp_core.c |4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c >> b/drivers/gpu/drm/exynos/exynos_dp_core.c >> index 4f3c7eb..02602a8 100644 >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c >> @@ -329,8 +329,8 @@ static int exynos_dp_link_start(struct exynos_dp_device >> *dp) >> return retval; >> >> for (lane = 0; lane < lane_count; lane++) >> -buf[lane] = DP_TRAIN_PRE_EMPHASIS_0 | >> -DP_TRAIN_VOLTAGE_SWING_400; >> +buf[lane] = DP_TRAIN_PRE_EMPH_LEVEL_0 | >> +DP_TRAIN_VOLTAGE_SWING_LEVEL_0; > > NAK! > > It makes build error. Please build your patch, before sending the patch. > It is a rule when submitting patches. > > Please, fix it as follows. > > + buf[lane] = DP_TRAIN_PRE_EMPHASIS_LEVEL_0| > + DP_TRAIN_VOLTAGE_SWING_LEVEL_0; > I think the first patch which you have taken (which adds new defines) is the one from the previous series for the same change. In the second version, I have named them as DP_TRAIN_PRE_EMPH_LEVEL_* which was done using cocci. Following is from that patch: # define DP_TRAIN_PRE_EMPHASIS_0 (0 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_0 (0 << 3) # define DP_TRAIN_PRE_EMPHASIS_3_5(1 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_1 (1 << 3) # define DP_TRAIN_PRE_EMPHASIS_6 (2 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_2 (2 << 3) # define DP_TRAIN_PRE_EMPHASIS_9_5(3 << 3) +# define DP_TRAIN_PRE_EMPH_LEVEL_3 (3 << 3) > Best regards, > Jingoo Han > >> >> retval = exynos_dp_write_bytes_to_dpcd(dp, DP_TRAINING_LANE0_SET, >> lane_count, buf); >> -- >> 1.7.10.4 >
[PATCH 1/7] drm: Renaming DP training vswing pre emph defines
On 8/26/2014 4:58 PM, Thierry Reding wrote: > On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal at intel.com wrote: >> From: Sonika Jindal >> >> Adding new defines, older one will be removed in the last patch in the >> series. >> This is to rename the defines to have levels instead of values for vswing and >> pre-emph levels as the values may differ in other scenarios like low vswing >> of >> eDP1.4 where the values are different. >> >> Done using following cocci patch for each define: >> @@ >> @@ >> >> # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) >> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) > > Could this perhaps be simply: > > #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0) > > As it is, there's no information about the value within the symbolic > name anyway, so _LEVEL_* really isn't that useful and keeping several > macros for each value seems isn't either. > I feel _LEVEL_* makes it more readable and since there are only 4 values possible, it is ok to have 4 different macros for readability purpose. What do you think? > An alternative would be to provide a second set of defines for eDP 1.4 > where the name implies the meaning and then use them as appropriate. > > Thierry >
[Intel-gfx] [PATCH v2 0/7] Rename DP training vswing/pre-emph defines
Hi, Did anybody get a chance to review the patches? Adding respective owners for different drivers.. Thanks, Sonika -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Jindal, Sonika Sent: Tuesday, August 19, 2014 1:42 PM To: intel-gfx at lists.freedesktop.org Cc: dri-devel at lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 0/7] Rename DP training vswing/pre-emph defines Hi, Did anybody get a chance to review other patches in this series? I got r-b for 2 patches (patches with changes in drm and i915) from Damien. Thanks, Sonika -Original Message- From: Jindal, Sonika Sent: Friday, August 8, 2014 4:24 PM To: intel-gfx at lists.freedesktop.org Cc: dri-devel at lists.freedesktop.org; Jindal, Sonika Subject: [PATCH v2 0/7] Rename DP training vswing/pre-emph defines From: Sonika Jindal <sonika.jin...@intel.com> Rename the defines to have levels instead of values for vswing and pre-emph levels as the values may differ in other scenarios like low vswing of eDP 1.4 where the values are different. Updated in all the drivers as well v2: Keeping the old defines in first patch and removing them in last patch. Used cocci semantic patch to replace the defines. Sonika Jindal (7): drm: Renaming DP training vswing pre emph defines drm/i915: Renaming DP training vswing pre emph defines drm/exynos: Renaming DP training vswing pre emph defines drm/gma500: Renaming DP training vswing pre emph defines drm/radeon: Renaming DP training vswing pre emph defines drm/tegra: Renaming DP training vswing pre emph defines drm: Remove old defines for vswing and pre-emph values drivers/gpu/drm/exynos/exynos_dp_core.c |4 +- drivers/gpu/drm/gma500/cdv_intel_dp.c |4 +- drivers/gpu/drm/gma500/intel_bios.c | 16 +-- drivers/gpu/drm/i915/intel_bios.c | 16 +-- drivers/gpu/drm/i915/intel_dp.c | 194 +++ drivers/gpu/drm/radeon/atombios_dp.c|4 +- drivers/gpu/drm/tegra/dpaux.c |4 +- include/drm/drm_dp_helper.h | 16 +-- 8 files changed, 129 insertions(+), 129 deletions(-) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx at lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[PATCH v2 0/7] Rename DP training vswing/pre-emph defines
Hi, Did anybody get a chance to review other patches in this series? I got r-b for 2 patches (patches with changes in drm and i915) from Damien. Thanks, Sonika -Original Message- From: Jindal, Sonika Sent: Friday, August 8, 2014 4:24 PM To: intel-gfx at lists.freedesktop.org Cc: dri-devel at lists.freedesktop.org; Jindal, Sonika Subject: [PATCH v2 0/7] Rename DP training vswing/pre-emph defines From: Sonika Jindal <sonika.jin...@intel.com> Rename the defines to have levels instead of values for vswing and pre-emph levels as the values may differ in other scenarios like low vswing of eDP 1.4 where the values are different. Updated in all the drivers as well v2: Keeping the old defines in first patch and removing them in last patch. Used cocci semantic patch to replace the defines. Sonika Jindal (7): drm: Renaming DP training vswing pre emph defines drm/i915: Renaming DP training vswing pre emph defines drm/exynos: Renaming DP training vswing pre emph defines drm/gma500: Renaming DP training vswing pre emph defines drm/radeon: Renaming DP training vswing pre emph defines drm/tegra: Renaming DP training vswing pre emph defines drm: Remove old defines for vswing and pre-emph values drivers/gpu/drm/exynos/exynos_dp_core.c |4 +- drivers/gpu/drm/gma500/cdv_intel_dp.c |4 +- drivers/gpu/drm/gma500/intel_bios.c | 16 +-- drivers/gpu/drm/i915/intel_bios.c | 16 +-- drivers/gpu/drm/i915/intel_dp.c | 194 +++ drivers/gpu/drm/radeon/atombios_dp.c|4 +- drivers/gpu/drm/tegra/dpaux.c |4 +- include/drm/drm_dp_helper.h | 16 +-- 8 files changed, 129 insertions(+), 129 deletions(-) -- 1.7.10.4
[Intel-gfx] [PATCH 1/6] drm: Renaming DP training vswing/pre-emph defines
On 8/5/2014 6:00 PM, Daniel Vetter wrote: > On Tue, Aug 5, 2014 at 1:33 PM, Jindal, Sonika > wrote: >> >> >> On 8/5/2014 4:45 PM, Daniel Vetter wrote: >>> >>> On Tue, Aug 05, 2014 at 04:38:17PM +0530, sonika.jindal at intel.com wrote: >>>> >>>> From: Sonika Jindal >>>> >>>> Renaming defines to have levels instead of nominal values. >>>> >>>> Signed-off-by: Sonika Jindal >>> >>> >>> You can't split up patches like this since this will break compilation. >>> For larger stuff (and imo this is right above the cutoff) you first need >>> to add the new functions/defines, then convert everyone over. And only >>> when all the drivers are converted can we apply the patch to remove the >>> old functions/defines. >>> -Daniel >>> >> Got your concern. So, I will repost the first patch keeping both the defines >> and an additional last patch for removing the extra defines. > > Yeah. Btw for the actual replacement I highly recommend to do it with > a semantic patch and cocci. There's unfortunately not a hole lot of > good documentation around, but we recently started and it helps a lot > with regenerating patches (e.g. for newly merged drm drivers) and > avoiding mistakes for manual conversions. If we do a cocci patch we > put it into the commit message, grep for @@. > -Daniel > I need to find out about cocci. Can we go ahead with this patch series only? -Sonika
[Intel-gfx] [PATCH 1/6] drm: Renaming DP training vswing/pre-emph defines
On 8/5/2014 4:45 PM, Daniel Vetter wrote: > On Tue, Aug 05, 2014 at 04:38:17PM +0530, sonika.jindal at intel.com wrote: >> From: Sonika Jindal >> >> Renaming defines to have levels instead of nominal values. >> >> Signed-off-by: Sonika Jindal > > You can't split up patches like this since this will break compilation. > For larger stuff (and imo this is right above the cutoff) you first need > to add the new functions/defines, then convert everyone over. And only > when all the drivers are converted can we apply the patch to remove the > old functions/defines. > -Daniel > Got your concern. So, I will repost the first patch keeping both the defines and an additional last patch for removing the extra defines. >> --- >> include/drm/drm_dp_helper.h | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index a21568b..70f362b 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -190,16 +190,16 @@ >> # define DP_TRAIN_VOLTAGE_SWING_MASK 0x3 >> # define DP_TRAIN_VOLTAGE_SWING_SHIFT 0 >> # define DP_TRAIN_MAX_SWING_REACHED(1 << 2) >> -# define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0) >> -# define DP_TRAIN_VOLTAGE_SWING_600 (1 << 0) >> -# define DP_TRAIN_VOLTAGE_SWING_800 (2 << 0) >> -# define DP_TRAIN_VOLTAGE_SWING_1200(3 << 0) >> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0) >> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_1 (1 << 0) >> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_2 (2 << 0) >> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_3 (3 << 0) >> >> # define DP_TRAIN_PRE_EMPHASIS_MASK(3 << 3) >> -# define DP_TRAIN_PRE_EMPHASIS_0(0 << 3) >> -# define DP_TRAIN_PRE_EMPHASIS_3_5 (1 << 3) >> -# define DP_TRAIN_PRE_EMPHASIS_6(2 << 3) >> -# define DP_TRAIN_PRE_EMPHASIS_9_5 (3 << 3) >> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_0 (0 << 3) >> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_1 (1 << 3) >> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_2 (2 << 3) >> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_3 (3 << 3) >> >> # define DP_TRAIN_PRE_EMPHASIS_SHIFT 3 >> # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED (1 << 5) >> -- >> 1.7.10.4 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
[Intel-gfx] [v3 09/13] drm/i915: Add rotation property for sprites
On 7/12/2014 3:29 AM, Daniel Vetter wrote:
> On Tue, Jul 08, 2014 at 10:31:59AM +0530, sonika.jindal at intel.com wrote:
>> From: Ville Syrj?l?
>>
>> Sprite planes support 180 degree rotation. The lower layers are now in
>> place, so hook in the standard rotation property to expose the feature
>> to the users.
>>
>> v2: Moving rotation_property to drm_plane
>
> This change is wrong and I haven't figured out who suggested it. The
> actual property is global (i.e. we should only have 1), but then it can be
> attached to different pieces.
>
> So here's what we need to do instead:
> - Add the rotation property to dev->mode_config.
> - Convert omapdrm to also store it there by moving it out of the omap
>driver private structure.
> - Probably better to just unconditionally set it up, but not sure.
> - Adjust the fb helper code accordingly.
> - Rip the omap restore rotation code in lastclose since the fb helper will
>now take care of this.
>
Ok, I will make this property a part of mode_config and will attach that
to drm_plane, and will post the remaining patches.
-Sonika
> I've merged the drm core parts for now but dropped the i915 implementation
> and the fb helper patch again.
>
> Thanks, Daniel
>
>>
>> Cc: dri-devel at lists.freedesktop.org
>> Signed-off-by: Ville Syrj?l?
>> Signed-off-by: Sonika Jindal
>> Reviewed-by: Imre Deak
>> ---
>> drivers/gpu/drm/i915/intel_sprite.c | 40
>> ++-
>> include/drm/drm_crtc.h |1 +
>> 2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index cbad738..aa63027 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -1202,6 +1202,29 @@ out_unlock:
>> return ret;
>> }
>>
>> +static int intel_plane_set_property(struct drm_plane *plane,
>> +struct drm_property *prop,
>> +uint64_t val)
>> +{
>> +struct intel_plane *intel_plane = to_intel_plane(plane);
>> +uint64_t old_val;
>> +int ret = -ENOENT;
>> +
>> +if (prop == plane->rotation_property) {
>> +/* exactly one rotation angle please */
>> +if (hweight32(val & 0xf) != 1)
>> +return -EINVAL;
>> +
>> +old_val = intel_plane->rotation;
>> +intel_plane->rotation = val;
>> +ret = intel_plane_restore(plane);
>> +if (ret)
>> +intel_plane->rotation = old_val;
>> +}
>> +
>> +return ret;
>> +}
>> +
>> int intel_plane_restore(struct drm_plane *plane)
>> {
>> struct intel_plane *intel_plane = to_intel_plane(plane);
>> @@ -1228,6 +1251,7 @@ static const struct drm_plane_funcs intel_plane_funcs
>> = {
>> .update_plane = intel_update_plane,
>> .disable_plane = intel_disable_plane,
>> .destroy = intel_destroy_plane,
>> +.set_property = intel_plane_set_property,
>> };
>>
>> static uint32_t ilk_plane_formats[] = {
>> @@ -1338,8 +1362,22 @@ intel_plane_init(struct drm_device *dev, enum pipe
>> pipe, int plane)
>> _plane_funcs,
>> plane_formats, num_plane_formats,
>> false);
>> -if (ret)
>> +if (ret) {
>> kfree(intel_plane);
>> +goto out;
>> +}
>> +
>> +if (!intel_plane->base.rotation_property)
>> +intel_plane->base.rotation_property =
>> +drm_mode_create_rotation_property(dev,
>> + BIT(DRM_ROTATE_0) |
>> + BIT(DRM_ROTATE_180));
>> +
>> +if (intel_plane->base.rotation_property)
>> +drm_object_attach_property(_plane->base.base,
>> + intel_plane->base.rotation_property,
>> + intel_plane->rotation);
>>
>> + out:
>> return ret;
>> }
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 08ed55e..6006c70 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -615,6 +615,7 @@ struct drm_plane {
>>
>> const struct drm_plane_funcs *funcs;
>>
>> +struct drm_property *rotation_property;
>> struct drm_object_properties properties;
>>
>> enum drm_plane_type type;
>> --
>> 1.7.10.4
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
[Intel-gfx] [v2 00/11] Support for 180 degree HW rotation
On 7/4/2014 8:36 PM, Damien Lespiau wrote: > On Fri, Jul 04, 2014 at 03:13:52PM +0530, sonika.jindal at intel.com wrote: >> From: Sonika Jindal >> >> Enables 180 degree rotation for sprite and primary planes. >> Updated the primary plane rotation support as per the new universal plane >> design. >> >> Most of these patches were already reviewed in intel-gfx in February 2014 >> thats >> why there is version history in few of them. >> >> v2: Moved rotation_property to drm_plane. Added updation of FBC when >> rotation is >> again set to 0. >> >> Testcase: kms_rotation_crc >> This igt can be extended for clipped rotation cases. Right it only tests 180 >> degree rotation for sprite and primary plane with crc check. >> >> >> Sonika Jindal (2): >>drm/i915: Add 180 degree primary plane rotation support >>drm: Resetting rotation property >> >> Ville Syrj?l? (9): >>drm: Move DRM_ROTATE bits out of omapdrm into drm_crtc.h >>drm: Add support_bits parameter to drm_property_create_bitmask() >>drm: Add drm_mode_create_rotation_property() >>drm/omap: Switch omapdrm over to drm_mode_create_rotation_property() >>drm: Add drm_rect rotation functions >>drm: Add drm_rotation_simplify() >>drm/i915: Add 180 degree sprite rotation support >>drm/i915: Make intel_plane_restore() return an error >>drm/i915: Add rotation property for sprites > > Missing from this series, your two documentation patches (we need to > bundle things up as a entity that makes sense for one of the maintainers > to pick it up (either Dave or Daniel)). > I was not aware that documentation patches should also be part of this. Because I was asked to send the igt testcase separately I thought documentation will also go separately. Documentation patches have also got reviewed-by tags. What do you suggest? Should I send the patchset again with documentation? Will Igt still be a separate post?
[Intel-gfx] [v2 10/11] drm/i915: Add 180 degree primary plane rotation support
On 7/4/2014 8:28 PM, Damien Lespiau wrote:
> On Fri, Jul 04, 2014 at 03:14:02PM +0530, sonika.jindal at intel.com wrote:
>> +static int intel_primary_plane_set_property(struct drm_plane *plane,
>> +struct drm_property *prop,
>> +uint64_t val)
>> +{
>> +struct drm_device *dev = plane->dev;
>> +struct drm_i915_private *dev_priv = dev->dev_private;
>> +struct intel_plane *intel_plane = to_intel_plane(plane);
>> +struct intel_crtc *intel_crtc = to_intel_crtc(plane->crtc);
>> +struct drm_crtc *crtc = _crtc->base;
>> +uint64_t old_val;
>> +
>> +if (prop == plane->rotation_property) {
>> +/* exactly one rotation angle please */
>> +if (hweight32(val & 0xf) != 1)
>> +return -EINVAL;
>> +
>> +old_val = intel_plane->rotation;
>> +intel_plane->rotation = val;
>> +
>> +if (intel_crtc->active && intel_crtc->primary_enabled) {
>> +intel_crtc_wait_for_pending_flips(crtc);
>> +
>> +/* FBC does not work on some platforms for rotated planes */
>> +if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
>> +if (dev_priv->fbc.plane == intel_crtc->plane &&
>> +intel_plane->rotation !=
>> BIT(DRM_ROTATE_0))
>> +intel_disable_fbc(dev);
>> +/* If rotation was set earlier and new rotation
>> is 0, we might
>> + * have disabled fbc earlier. So update it now
>> */
>> +else if (intel_plane->rotation ==
>> BIT(DRM_ROTATE_0) &&
>> +old_val != BIT(DRM_ROTATE_0))
>> +intel_update_fbc(dev);
>
> I see a intel_update_fbc() called with the struct_mutext lock elsewhere,
> don't we need it here as well?
>
> mutex_lock(>struct_mutex);
> intel_update_fbc(dev);
> mutex_unlock(>struct_mutex);
>
Sure, I'l add that and post the patch.
[PATCH 01/12] drm: Move DRM_ROTATE bits out of omapdrm into drm_crtc.h
Hi Thierry, This patch was sent out by mistake. I am sorry for the confusion. I am working on further patchset based on these patches. Regards, Sonika On 5/13/2014 1:16 PM, Thierry Reding wrote: > On Fri, May 09, 2014 at 01:31:22PM +0530, sonika.jindal at intel.com wrote: >> From: Ville Syrj?l? >> >> The rotation property stuff should be standardized among all drivers. >> Move the bits to drm_crtc.h from omap_drv.h. >> >> Cc: David Airlie >> Cc: Tomi Valkeinen >> Cc: Dave Airlie >> Cc: Rob Clark >> Cc: Daniel Vetter >> Cc: Archit Taneja >> Cc: dri-devel at lists.freedesktop.org >> Cc: linux-kernel at vger.kernel.org >> Signed-off-by: Ville Syrj?l? >> Reviewed-by: Sagar Kamble >> Tested-by: Sagar Kamble > > This is missing your Signed-off-by line. Also the patch subject says > 01/12, where are the other eleven patches? > > Thierry >
