Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c97a8778-2f02-d74e-d38a-d11829fc15d9@wolfvision.net>
Date:   Thu, 19 Jan 2023 13:56:32 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/5] drm/rockchip: rgb: embed drm_encoder into
 rockchip_encoder
Content-Language: en-US
To:     Sascha Hauer <sha@pengutronix.de>
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Sandy Huang <hjc@rock-chips.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
References: <20221130140217.3196414-1-michael.riesch@wolfvision.net>
 <20221130140217.3196414-2-michael.riesch@wolfvision.net>
 <20221207064507.GV29728@pengutronix.de>
From:   Michael Riesch <michael.riesch@wolfvision.net>
In-Reply-To: <20221207064507.GV29728@pengutronix.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MEFQQjlFcmNRZFlRSU0wUWlhOGE4MEpaQ0Z6TUNnQ1VxcE9pUmtRaDNqS2dQ?=
 =?utf-8?B?RU9mVGxHV3FwRlhobTF4QzQ1RUtEOGVaaUJWcDFCUVdZWDFBaUhUWUFBRVJB?=
 =?utf-8?B?RXJUaTlXQXYxRXU2dWFRaktHOUZkQjFDZEdZblpNYlBWRi9BSlNZY3g2QzhP?=
 =?utf-8?B?WERydDRvdndUUHdGczVtNVlyR0pRRWdOSkZnSjdseGR1VFUyeDN2bGdXcThP?=
 =?utf-8?B?M3FRTDE5SlFtNzQrVWFVRnRvRk43YStDZlpsd3NKTnhZR0pVZTFtRHBpbnZi?=
 =?utf-8?B?M1FwRTVxSGk5QVVDK2tuVXN1NTFud29taWdRaU41dlRIQVNESWxrTmMzZjVt?=
 =?utf-8?B?Q2d6ajVNMHZyOGFKUU1PNmU2SEM1NDlzOGZHS3dxUm1wQktiNU82V3FFZGpO?=
 =?utf-8?B?Um5FL0tMUTRYbUw2WUFmaEdKYzZBeDI2dHYweWtJWTk2UXVmM2R3enFCT1k4?=
 =?utf-8?B?N3FQYytwS09NbURiV0JjMEVlb2xzMHZXUlBwWmtFVmZqZWxHa3BYdHl6N01o?=
 =?utf-8?B?dEw1aWR0Y0JReFZleG84Y2w3cDErNkNFbzl1bXZXdlZjd2VuZFQydldKMy9a?=
 =?utf-8?B?WWlhVVJQSThsZGx4K0NMMXlQdElhRmNjQ1VZaHZXaVNyZ3J2K3JmekhUYlYx?=
 =?utf-8?B?Y0VLeFB1Y0l6cllaUVJpeE44Tjdxd2gxSWNnTlE4M0lZNUNUdndaSTBDNzNL?=
 =?utf-8?B?YythalpnRGp1VmFLcVhycnQwbUkyVE9RL1k0M2FwSmovbW9GTmFWZkZaS0U3?=
 =?utf-8?B?bFRoUmxIaTk3ejViZ3BQOXovRXhiM1h1ZjMzRVJnM2dPMEZnNDVoMVlVM3RV?=
 =?utf-8?B?cW9zWUw3K3M2bzk5UTVWcVlMK0dSNGhWbHVGZWtMbVhoMWJZcVE3SGwvUFBh?=
 =?utf-8?B?Q1lzamNWa0NabHF0T0ZuVjZKa1ZSZlBzSThoNk9XWnVOUmRnaXRJdnNqWm81?=
 =?utf-8?B?cDR5OEM0UnZrWmcxaGlHbWM0OExIckN2WGdlazdXbEJjdzFNTlU4RmNYLzZE?=
 =?utf-8?B?Q0FZSDlvVkVWWS94YWIvUFZJeVUrUnQ3WVg3a2FpMlB0ZjlLRTBNb0Z6TVdp?=
 =?utf-8?B?U3pIT0Rvc2JDZThwS1Q1bVRYZGpCOUhoNUg4Yk02TFBBU3VsRFdYbjFPMFJj?=
 =?utf-8?B?T05pOHBVZUNDZFFaL3hISnI0bHltQjhpTmVXaEV6UDNaaGZqQmc4cnNNd0Q1?=
 =?utf-8?B?ZWlpQzM3SThFMW5OUENyRG9BOVBZRzBxdThUdk45N041Q21hd25BNS9TSnNo?=
 =?utf-8?B?R1ExVEtZU1A4d1RUdncwL0c1VWJVN1JzY25NNVAxMm90eEk4amJWWS9BTmdB?=
 =?utf-8?B?RVg3eHhuQ3dvS2Qxd3VMK2d5cGJlNkt3bFZXeTkvQzN0WGJySEJXRGNKOXRX?=
 =?utf-8?B?d3RjWG9CTGtaRHFKRDFrUjAxWmg0RlI0RVI3TXNRSEIrYnNHaWN5ZjNJbnE1?=
 =?utf-8?B?aXJlbVU3SGFEcTRrcDBjM2NneDIrRkRPa0tYWkprcTVxSEJuUmRKOXJYTEF2?=
 =?utf-8?B?dTdkTUE4QWpuQnVTR0I3cyttWkJodEhybFlweW9OSTBDNFZDLzRCRjRHQjBh?=
 =?utf-8?B?ZWUwNDFRUmFaZnN3c2N1eC9qajlhWktrdjJtZUpGT3Qzb3dQUnA5UmQyZkta?=
 =?utf-8?B?SjJoZEhJUGQ0WGVQQTNaQjZpcWVWRFVNdXg4aURnM1JMM3ZsY0ZNckprZEpX?=
 =?utf-8?B?aU16ZVZDN3djZU04YjZvTjU4SWd0c01UczVEb1dmSmZibi9Hc0NXcnVkalN1?=
 =?utf-8?B?UjBueHl2NTNPb2JlaS9iUHRTTmxHSUVRREVmUzVSbTBlOWJKcy9Xd1lPYXZm?=
 =?utf-8?B?bTlzWVIrYUpMelQvZTBMZmFMTFhUZWVRc3lIVk1yRVNFMDh6SUR1MXF6K0tS?=
 =?utf-8?B?d25lSEN3TFlRTnFoYnpWTlJaR2orR1pMWll1eHJpNEJKS3NuVXhRNXJjQkQ5?=
 =?utf-8?B?aFgxc0o3aGN4STVxa2dEa1VsbysrcnlkOEZRSHpSMnFZZDR0YXErNzFiaUJO?=
 =?utf-8?B?RkN1RVgxdCt4WThoYnN0OXNKL3UzNEk2YUphWWcrSVo5bGRqblgvVERWR3Vx?=
 =?utf-8?B?Y280THgweFVTcXd1T0ZCWXl6Ri9tOGU3QkszZFlEYTFUck1wVk1OZVJYR2lW?=
 =?utf-8?B?S0FzdkpUN0ZUekdxVis3bDQvSklsU1hUVlVDYllrelR1ejVUbHlvUnRKQWFC?=
 =?utf-8?B?QktZWmhKVzFHN2gyUDV1M3ZLNURuYythbFBPUGs4ZEtadHlZTEI4Z2ZsTEhR?=
 =?utf-8?Q?fZHGHaWHqgB6tyO22Ue/ClmtFpK8KxiFasEFM59lTE=3D?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 38a62e34-87c3-4fc0-f60e-08dafa1c989e
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9155.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 12:56:35.3867
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: e94ec9da-9183-471e-83b3-51baa8eb804f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iJ6sHmJpWMuM5j56FcEvCh0rWOF17crUZ6oEJmSD8DzJuyVVDuOv7DOpETs++cL9DV56g5CSE5AVoG83ZCXkNcvQjtXDOdEEMjmWJOUPwRw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS2PR08MB9245
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Sascha,

Thanks for your comments!

On 12/7/22 07:45, Sascha Hauer wrote:
> On Wed, Nov 30, 2022 at 03:02:13PM +0100, Michael Riesch wrote:
>> Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
>> rockchip_decoder") provides the means to pass the endpoint ID to the
>> VOP2 driver, which sets the interface MUX accordingly. However, this
>> step has not yet been carried out for the RGB output block. Embed the
>> drm_encoder structure into the rockchip_encoder structure and set the
>> endpoint ID correctly.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> index 75eb7cca3d82..16201a5cf1e8 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> @@ -18,17 +18,17 @@
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_simple_kms_helper.h>
>>  
>> +#include <dt-bindings/soc/rockchip,vop2.h>
>> +
>>  #include "rockchip_drm_drv.h"
>>  #include "rockchip_drm_vop.h"
>>  #include "rockchip_rgb.h"
>>  
>> -#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
>> -
>>  struct rockchip_rgb {
>>  	struct device *dev;
>>  	struct drm_device *drm_dev;
>>  	struct drm_bridge *bridge;
>> -	struct drm_encoder encoder;
>> +	struct rockchip_encoder encoder;
>>  	struct drm_connector connector;
>>  	int output_mode;
>>  };
>> @@ -125,7 +125,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>>  		return ERR_PTR(ret);
>>  	}
>>  
>> -	encoder = &rgb->encoder;
>> +	encoder = &rgb->encoder.encoder;
>>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>>  
>>  	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
>> @@ -161,6 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>>  		goto err_free_encoder;
>>  	}
>>  
>> +	rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;
> 
> This is vop2 specific. This code is used with the vop1 as well, so it
> doesn't look like it belongs here, at least not hidden in a patch titled
> "embed drm_encoder into rockchip_encoder".

OK, the very least I can do is to create an extra patch that sets the
crtc_endpoint_id.

> Normally the crtc_endpoint_id is set by the encoder, coming from the
> encoder node, asking the question "To which port on the VOP am I
> connected to?"
> 
> Here the situation is different. We are called from the VOP and the
> question instead is: "Is there something connected to VPx endpoint id
> ROCKCHIP_VOP2_EP_RGB0?"
> 
> You might need a vop2 specific entry to this code.

I just realized that rockchip_rgb_init parses the endpoint ID. If I am
not mistaken, we can directly store it in the crtc_endpoint_id member.

Seeing that this would be pretty generic, can it be included in one
patch (or should I still split this into a separate patch)?

Best regards,
Michael