Re: [PATCH] drm/fsl-dcu: set DPMS off before initializing connector

[Stefan Agner <stefan@agner.ch>]

Hi Laurent,

On 2017-11-09 11:45, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thursday, 9 November 2017 11:14:36 EET Stefan Agner wrote:
> 
> I notice you have changed the subject line. I'm not sure if the new wording is 
> better, as this patch doesn't set DPMS to off, it instead skips setting it 
> manually. How about "drm/fsl-dcu: Don't set connector DPMS property manually" 
> or "drm/fsl-dcu: Don't set DPMS property before initializing connector" ?

In a first version I also added:

connector->dpms = DRM_MODE_DPMS_OFF;

before calling drm_connector_init, but I think this is not really
required so removed it again, but forgot to adjust the subject.

Will reset to "drm/fsl-dcu: Don't set connector DPMS property"

> 
> Additionally, would you mind retaining the authorship of the patch I have 
> originally submitted ?

Yeah, sorry, will send v2 with proper authorship and subject.

--
Stefan


>> Since commit 4a97a3da420b ("drm: Don't update property values for atomic
>> drivers") atomic drivers must not update property values as properties
>> are read from the state instead. To catch remaining users, the
>> drm_object_property_set_value() function now throws a warning when
>> called by atomic drivers on non-immutable properties, and we hit that
>> warning when creating connectors.
>>
>> The easy fix is to just remove the drm_object_property_set_value() as it
>> is used here to set the initial value of the connector's DPMS property
>> to OFF. The DPMS property applies on top of the connector's state crtc
>> pointer (initialized to NULL) that is the main connector on/off control,
>> and should thus default to ON.
>>
>> Fixes: 4a97a3da420b ("drm: Don't update property values for atomic drivers")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index
>> edd7d8127d19..c54806d08dd7 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
>> @@ -102,7 +102,6 @@ static int fsl_dcu_attach_panel(struct
>> fsl_dcu_drm_device *fsl_dev, {
>>  	struct drm_encoder *encoder = &fsl_dev->encoder;
>>  	struct drm_connector *connector = &fsl_dev->connector.base;
>> -	struct drm_mode_config *mode_config = &fsl_dev->drm->mode_config;
> 
> Oops, I had missed that, sorry.
> 
>>  	int ret;
>>
>>  	fsl_dev->connector.encoder = encoder;
>> @@ -122,10 +121,6 @@ static int fsl_dcu_attach_panel(struct
>> fsl_dcu_drm_device *fsl_dev, if (ret < 0)
>>  		goto err_sysfs;
>>
>> -	drm_object_property_set_value(&connector->base,
>> -				      mode_config->dpms_property,
>> -				      DRM_MODE_DPMS_OFF);
>> -
>>  	ret = drm_panel_attach(panel, connector);
>>  	if (ret) {
>>  		dev_err(fsl_dev->dev, "failed to attach panel\n");