Re: [drm_hwcomposer PATCH] Take Connection state into account. (v2)

[Bish, Jim]

On Wed, 2018-01-03 at 13:40 +0100, Mauro Rossi wrote:


2018-01-03 12:16 GMT+01:00 Robert Foss 
>:
Hey Mauro,

Thanks for the patch! It builds and looks good to me, but I have some
suggestions however.


On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote:
> These changes avoid following logcat error on integrated and
> dedicated GPUs:
>
> ... 2245  2245 E hwc-drm-resources: Could not find a suitable
> encoder/crtc for display 2
> ... 2245  2245 E hwc-drm-resources: Failed CreateDisplayPipe 56 with
> -19
> ... 2245  2245 E hwcomposer-drm: Can't initialize Drm object -19

It isn't quite clear clear which errors belong to which changes,
to make this patch a bit more bisectable it would be nice to see
independent commits created for each error.

Hi Robert,

In this case I honestly do not think that splitting would add too much value,
original commit (v1) is  well documented in [1] and tackles with bug in 
drmresources.cpp
which currently fails to find workable crtc -> encoder -> display output 
combination
for integrated and dedicated GPUs. Besides that it was also adding DisplayPort 
drm mode connector type

So changes in drmresources.cpp belong to solving "Could not find a suitable 
encoder/crtc for display X" problem,
which is a show stopper for enabling mainline graphics in android-x86

Other developers observed independently that the current implementation is 
"embedded oriented" and only checks the first display output,
isn't it?

The only change I did in drmconnector.cpp is to account for the additional 
external drm mode connectors types
which were missing (DVID, DVII, VGA) . One question on my side: Do we need more?

Besides these minor types lists discussions, I would propose you to check with 
Jim Bish if he should have the credit for the patch
and review the coding of his changes.
it would be proper to keep the original author signoff but ok to add additional 
signoff/testing, etc.  I am not so worried about credit for this just patch but 
good to following proper procedures.


>
> (v1) There are various places where we should be really taking
> connection
> state into account before querying the properties or assuming it
> as primary. This patch fixes them.
>
> BUG=None.
> TEST=System boots up and shows UI.
>
> (v1) Signed-off-by: Jim Bish >
>
> (v2) porting of original commit 76fb87e675 of android-ia master
> with additional external connector types (DisplayPort, DVID, DVII,
> VGA)
> Tested with i965 on Sandybridge and nouveau on GT120, GT610

The commit message isn't really following the usual format. It doesn't
need to include a changelog (that is typically placed between the "---"
markers in the email instead),
and it is missing a signoff from the submitter (you).

Sorry I don't have a signature, as usually my patches need sign-off from 
professionals,
I'm a (crash test dummy) hobbyist..really :-)

Maybe Rob Herring, Qiang Yu  or Chih-Wei could review and sign-off the proposed 
patch


The BUG and TEST fields are not strictly required either, but aren't a
problem.

That part is the original Jim Bish commit message [1], my changes version is 
(v2)
Sorry if I was not clear enought


> ---
>  drmconnector.cpp | 4 +++-
>  drmresources.cpp | 9 +++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drmconnector.cpp b/drmconnector.cpp
> index 247f56b..145518f 100644
> --- a/drmconnector.cpp
> +++ b/drmconnector.cpp
> @@ -73,7 +73,9 @@ bool DrmConnector::internal() const {
>  }
>
>  bool DrmConnector::external() const {
> -  return type_ == DRM_MODE_CONNECTOR_HDMIA;
> +  return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ ==
> DRM_MODE_CONNECTOR_DisplayPort ||
> + type_ == DRM_MODE_CONNECTOR_DVID || type_ ==
> DRM_MODE_CONNECTOR_DVII ||
> + type_ == DRM_MODE_CONNECTOR_VGA;
>  }

The changes to external() should probably be broken out into it's own
commit, since external() is called elsewhere changes to it _could_
introduce issues.

Will you check the code, involving Jim Bish if necessary, about these potential 
issues?
I am available to perform changes/tests, but the original maker will be better.
Cheers

Mauro


>
>  bool DrmConnector::valid_type() const {
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 32dd376..d582cfe 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -159,7 +159,7 @@ int DrmResources::Init() {
>
>// First look for primary amongst internal connectors
>for (auto  : connectors_) {
> -if (conn->internal() && !found_primary) {
> +if (conn->state() == DRM_MODE_CONNECTED && conn->internal() &&
> !found_primary) {
>conn->set_display(0);
>found_primary = true;
>  } else {
> @@ -170,7 +170,7 @@ int DrmResources::Init() {
>
>// Then look for primary amongst external connectors
>for (auto  : connectors_) {
> -if (conn->external() && !found_primary) {
> +if (conn->state() == 

[PATCH v7 11/25] drm/i915: Register color correction capabilities

[Bish, Jim]

On Tue, 2015-10-20 at 18:04 +0530, Shashank Sharma wrote:
> From DRM color management:
> 
> DRM color manager supports these color properties:
> 1. "ctm": Color transformation matrix property, where a
>color transformation matrix of 9 correction values gets
>applied as correction.
> 2. "palette_before_ctm": for corrections which get applied
>beore color transformation matrix correction.
> 3. "palette_after_ctm": for corrections which get applied
>after color transformation matrix correction.
> 
> These color correction capabilities may differ per platform, 
> supporting
> various different no. of correction coefficients. So DRM color 
> manager
> support few properties using which a user space can query the 
> platform's
> capability, and prepare color correction accordingly.
> These query properties are:
> 1. cm_coeff_after_ctm_property
> 2. cm_coeff_before_ctm_property
>   (CTM is fix to 9 coefficients across industry)
> 
> Now, Intel color manager registers:
> ==
> 1. Gamma correction property as "palette_after_ctm" property
> 2. Degamma correction capability as "palette_bafore_ctm" property
>capability as "palette_after_ctm" DRM color property hook.
> 3. CSC as "ctm" property.
> 
> So finally, This patch does the following:
> 1. Add a function which loads the platform's color correction
>capabilities in the cm_crtc_palette_capabilities_property 
> structure.
> 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
>getting initiaized.
> 3. Adds two new parameters "num_samples_after_ctm" and
>"num_samples_before_ctm" in intel_device_info as gamma and
>degamma coefficients vary per platform basis.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/i915_drv.h|  2 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 31 
> ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h
> index 8afda45..613bee2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -789,6 +789,8 @@ struct intel_device_info {
>   u8 num_sprites[I915_MAX_PIPES];
>   u8 gen;
>   u8 ring_mask; /* Rings supported by the HW */
> + u16 num_samples_after_ctm;
> + u16 num_samples_before_ctm;
thought we agreed last week that num_samples was going to be 
removed.  May be ok to handle in a later patch unless
someone has strong objection.

Jim
>   DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
>   /* Register offsets for the various display pipes and transcoders 
> */
>   int pipe_offsets[I915_MAX_TRANSCODERS];
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index b03ee94..334bfff 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -30,4 +30,35 @@
>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>   struct drm_crtc *crtc)
>  {
> + struct drm_mode_config *config = >mode_config;
> + struct drm_mode_object *mode_obj = >base;
> +
> + /*
> + * Register:
> + * =
> + * Gamma correction as palette_after_ctm property
> + * Degamma correction as palette_before_ctm property
> + *
> + * Load:
> + * =
> + * no. of coefficients supported on this platform for gamma
> + * and degamma with the query properties. A user
> + * space agent should read these query property, and prepare
> + * the color correction values accordingly. Its expected from the
> + * driver to load the right number of coefficients during the init
> + * phase.
> + */
> + if (config->cm_coeff_after_ctm_property) {
> + drm_object_attach_property(mode_obj,
> + config->cm_coeff_after_ctm_property,
> + INTEL_INFO(dev)->num_samples_after_ctm);
> + DRM_DEBUG_DRIVER("Gamma query property initialized\n");
> + }
> +
> + if (config->cm_coeff_before_ctm_property) {
> + drm_object_attach_property(mode_obj,
> + config->cm_coeff_before_ctm_property,
> + INTEL_INFO(dev)->num_samples_before_ctm);
> + DRM_DEBUG_DRIVER("Degamma query property initialized\n");
> + }
>  }

[Intel-gfx] [PATCH v6 14/23] drm/i915: CHV: Pipe level degamma correction

[Bish, Jim]

On 10/19/2015 11:54 AM, Daniel Vetter wrote:
> On Mon, Oct 19, 2015 at 06:08:52PM +, Smith, Gary K wrote:
>> FYI - this shouldn't block the commits, but should be optimized later 
>> (fairly soon). 
>>
>> I believe the current implementation ends up executing 
>>  while (count < CHV_DEGAMMA_MAX_VALS) {
>>  // Do lots of caclulation
>>  I915_WRITE(cgm_degamma_reg, word);
>> Every frame after you set the property, whether you change the contents of 
>> the blob or not. Clearly this is really inefficient when there are 100s of 
>> gamma values to write. 
>> Same with the Gamma and CTM. CTM is less of an issue with only 9 entries.
>>
>> My suggestion here is to set a boolean when the property has been set as
>> part of a flip and only apply it on the atomic commit after the update
>> was received.
> 
> Yeah the usual design is to add a foo_changed boolean (or bitmask, e.g.
> for changed planes tracked in the crtc_state) in the state where we need
> to commit the update. That /should/ be there really, didn't ever realize
> it's not done. Note that that for legacy cursors we update them without
> waiting for vblanks and legacy userspace does that a _lot_ (*cough* X
> server *cough*), if the overhead is severe this might be a problem and
> needs to be fixed before merging.
> 
> -Daniel
Severe enough to block? I wanted to get this closed out this week but...
I see your point Gary but need a reading on Daniel's last sentence.

Jim
> 
>>
>> Thanks
>> Gary
>>
>> -Original Message-
>> From: Sharma, Shashank 
>> Sent: Friday, October 16, 2015 3:29 PM
>> To: dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; 
>> emil.l.velikov at gmail.com; Roper, Matthew D; Bradford, Robert; Bish, Jim
>> Cc: Matheson, Annie J; kausalmalladi at gmail.com; Kumar, Kiran S; Smith, 
>> Gary K; Vetter, Daniel; Mukherjee, Indranil; Palleti, Avinash Reddy; Sharma, 
>> Shashank
>> Subject: [PATCH v6 14/23] drm/i915: CHV: Pipe level degamma correction
>>
>> CHV/BSW supports Degamma color correction, which linearizes all the 
>> non-linear color values. This will be applied before Color Transformation.
>>
>> This patch does the following:
>> 1. Attach deGamma property to CRTC
>> 2. Add the core function to program DeGamma correction values for
>>CHV/BSW platform
>> 2. Add DeGamma correction macros/defines
>>
>> Signed-off-by: Shashank Sharma 
>> Signed-off-by: Kausal Malladi 
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h|  6 ++
>>  drivers/gpu/drm/i915/intel_color_manager.c | 92 
>> ++  drivers/gpu/drm/i915/intel_color_manager.h | 
>>  5 ++
>>  3 files changed, 103 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h index 45ddd84..1e46562 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8160,4 +8160,10 @@ enum skl_disp_power_wells {  #define 
>> _PIPE_GAMMA_BASE(pipe) \
>>  (_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
>>  
>> +#define PIPEA_CGM_DEGAMMA  (VLV_DISPLAY_BASE + 0x66000)
>> +#define PIPEB_CGM_DEGAMMA  (VLV_DISPLAY_BASE + 0x68000)
>> +#define PIPEC_CGM_DEGAMMA  (VLV_DISPLAY_BASE + 0x6A000)
>> +#define _PIPE_DEGAMMA_BASE(pipe) \
>> +(_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, 
>> +PIPEC_CGM_DEGAMMA))
>> +
>>  #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
>> b/drivers/gpu/drm/i915/intel_color_manager.c
>> index acb9647..1bbad79 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,92 @@
>>  
>>  #include "intel_color_manager.h"
>>  
>> +static int chv_set_degamma(struct drm_device *dev,
>> +struct drm_property_blob *blob, struct drm_crtc *crtc) {
>> +u16 red_fract, green_fract, blue_fract;
>> +u32 red, green, blue;
>> +u32 num_samples;
>> +u32 word = 0;
>> +u32 count, cgm_control_reg, cgm_degamma_reg;
>> +enum pipe pipe;
>> +struct drm_palette *degamma_data;
>> +struct drm_i915_private *dev_priv = dev->dev_private;
>> +struct drm_r32g32b32 *correction_values = NULL;
>> +struct drm_crtc_state *state = crtc->state;
>> +
>> +if (WARN_ON(!blob))
>> +return -EINVAL;
>> +
>>