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;
Date:   Wed, 29 Mar 2023 22:17:26 -0600
From:   Lucas De Marchi <lucas.demarchi@intel.com>
To:     Thomas Zimmermann <tzimmermann@suse.de>
CC:     Sui Jingfeng <15330273260@189.cn>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        suijingfeng <suijingfeng@loongson.cn>, liyi <liyi@loongson.cn>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/fbdev-generic: optimize out a redundant assignment
 clause
Message-ID: <20230330041726.w7boceq7ljymvfq2@ldmartin-desk2>
References: <20230325074636.136833-1-15330273260@189.cn>
 <a3370ae7-8c78-8170-f9c3-7f616a1fa382@suse.de>
Content-Type: text/plain; charset="iso-8859-1"; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a3370ae7-8c78-8170-f9c3-7f616a1fa382@suse.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?IxOtDxyAKCNy/+DKpZj1ZLiCmdrpePhbFV7DFvzQ7JHe6HkMhLOLUgugaV?=
 =?iso-8859-1?Q?KfoGxhJWPygtDDeMz7SwabPLXhW2UjIGpd+mzWaHFN+q51lE6276OLPuWR?=
 =?iso-8859-1?Q?W011Q3WZDRLlibpow5bnRYYF8F25lF+64QGHoFj0bq82MA2iCjwYiGhfhi?=
 =?iso-8859-1?Q?59kWaxCl+OvX60n000CTuzRlFh1h2hucDTkzv4Lftf4lfGYErY9wwxUTUc?=
 =?iso-8859-1?Q?kPoNNHbjpXRoFV2lGtuClO930xP3JIbkOiNH9v2nccQoV7XP4bzxBWZpjS?=
 =?iso-8859-1?Q?qE87zwPEUjSEFOjATUzhHREqRrKrI0wIkG8t7TX1lJU1QoR7Anqw7R9tcn?=
 =?iso-8859-1?Q?T2VlKm68ziFrBvkQCsKhi3iyHwMY04HAmFPxPzNTvOn92tmztH4wg4/iDO?=
 =?iso-8859-1?Q?aQrB64VRkFT1bIVEBRhqEjyVgF5ELDgFBne0A/zbVqZiSd+uAt6+jgN4fj?=
 =?iso-8859-1?Q?8PKGyP0oMlGh3LOEeGCridJ5UuO2HJmsJ1ggR7wdbtR/we6cuSLYaBvy5D?=
 =?iso-8859-1?Q?fOzeR8zagvlOxMjKCKFJShbRlC1aTXGoOpPiJ/v66Vcc2+0jXqEVlC9xta?=
 =?iso-8859-1?Q?MJ6lozcelSkFDvw+znN7DVi9JNKToxZLJP7rwv/ZNQTfHcW+iKQeczH2uR?=
 =?iso-8859-1?Q?2KEHjx9Ncjg9Y7Lxz4K0FraDt54HZu9HbOsKOQUBSrQRpi8X/Ys5aL1cgk?=
 =?iso-8859-1?Q?RWqQW947EkXMva5fyYYUst8fROK4umDvLfGV85sf8rPoIxIYylgrb+Vo+1?=
 =?iso-8859-1?Q?j0dLzIfeQguXqGHr4ODSE+Q8IFslNiDArvaUm+mmXfgqp0Ay+yqQV5wV4m?=
 =?iso-8859-1?Q?lfhqIisi7Cu5PBvyH/BCX+NAnk0r2+8OUU4FNcYnHfpucMP6dcZOzmk2tb?=
 =?iso-8859-1?Q?4FUxyOfF8BsnunXB8PGUNpyiAwYlr0mm65iRjSaklOvwz+Yw4Iw08cTLAB?=
 =?iso-8859-1?Q?iP5+kMmiUf1jTRuzmG92b/40923KoM6tiaPdrnjLYUYdQ5SLjjrnAOHcH7?=
 =?iso-8859-1?Q?vjtQ7h2tJ6PTjHZQmL/P+I9V5kaopeRZpw9JpZcFXavuhyAHRwS7+4fnkO?=
 =?iso-8859-1?Q?+9UpL6Yr7hyuQGyv1tIqiXdxECryHxkEKul6tew7/Iks40sXFpVeWGOBkX?=
 =?iso-8859-1?Q?asWqRvLO6J0xxRXxGnZM/t6sbRNkKXFVQa7TkH3GjH69wVWzyDLBC+1mA7?=
 =?iso-8859-1?Q?z6U7ffny85nSNxcTxlrlxn5s6hotkCLd/AXAtRLYDKmSU5P67Db4JpNX0e?=
 =?iso-8859-1?Q?vLVL1YcJH1sQDFCqUa0kX17P5sisOzieLVzz/oDULf7DaNnDZc1ddsKETA?=
 =?iso-8859-1?Q?BEcZU8nvbTP2AfhTQ8xwR4U1plqC7B0Rje1THbybQpxlyJg6tg+nlGI/kh?=
 =?iso-8859-1?Q?yeWIvm3AnjfY7HyJhrMPynblx6LQE4yzE/ixvwAupN9WL/FgUrNp7576UM?=
 =?iso-8859-1?Q?j8dRUev0ZFy/0UPHHQJjw1f9guL2Szh/QRnLnTzwAc2G3znNjDmXBZClS+?=
 =?iso-8859-1?Q?K539vic66ENQXTJH6fx9TycbGbvzo670ZtGHYVAGmCjmGsdnal8lSZi2NB?=
 =?iso-8859-1?Q?2zZTkMNiD+luHKVOAeIKGvP7AWgnc62PozGZ0C96+1LaMSlNDJUBdLgBKm?=
 =?iso-8859-1?Q?5oDVSNVk9NBvERc7C1W9NDvpFBi0LSpBzgeOeNqaN3R+dfGNIiltPedQ?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d7bc165-4171-4b2d-3a1b-08db30d5bfc2
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 04:18:00.9490
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pqNMLOHaXSImecHRjsReln2/pInzeWUrBUaNeZ3H8huMvqkYcgJ2L8g5IMowjO9nIrU6x0kP3uf9sbWkIiUzCZu4ALLf03dZ0E+JxXThYyQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5106
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_NONE autolearn=unavailable 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

On Wed, Mar 29, 2023 at 11:04:17AM +0200, Thomas Zimmermann wrote:
>(cc'ing Lucas)
>
>Hi
>
>Am 25.03.23 um 08:46 schrieb Sui Jingfeng:
>>  The assignment already done in drm_client_buffer_vmap(),
>>  just trival clean, no functional change.
>>
>>Signed-off-by: Sui Jingfeng <15330273260@189.cn>
>>---
>>  drivers/gpu/drm/drm_fbdev_generic.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>>index 4d6325e91565..1da48e71c7f1 100644
>>--- a/drivers/gpu/drm/drm_fbdev_generic.c
>>+++ b/drivers/gpu/drm/drm_fbdev_generic.c
>>@@ -282,7 +282,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
>>  				 struct drm_clip_rect *clip)
>>  {
>>  	struct drm_client_buffer *buffer = fb_helper->buffer;
>>-	struct iosys_map map, dst;
>>+	struct iosys_map map;
>>  	int ret;
>>  	/*
>>@@ -302,8 +302,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
>>  	if (ret)
>>  		goto out;
>>-	dst = map;
>>-	drm_fbdev_damage_blit_real(fb_helper, clip, &dst);
>>+	drm_fbdev_damage_blit_real(fb_helper, clip, &map);
>
>I see what you're doing and it's probably correct in this case.
>
>But there's a larger issue with this iosys interfaces. Sometimes the 
>address has to be modified (see calls of iosys_map_incr()). That can 
>prevent incorrect uses of the mapping in other places, especially in 
>unmap code.

using a initializer for the cases it's needed IMO would make these kind
of problems go away, because then the intent is explicit

>
>I think it would make sense to consider a separate structure for the 
>I/O location. The buffer as a whole would still be represented by 
>struct iosys_map.  And that new structure, let's call it struct 
>iosys_ptr, would point to an actual location within the buffer's

sounds fine to me, but I'd have to take a deeper look later (or when
someone writes the patch).  It seems we'd replicate almost the entire
API to just accomodate the 2 structs.  And the different types will lead
to confusion when one or the other should be used

thanks
Lucas De Marchi

>memory range. A few locations and helpers would need changes, but 
>there are not so many callers that it's an issue.  This would also 
>allow for a few debugging tests that ensure that iosys_ptr always 
>operates within the bounds of an iosys_map.
>
>I've long considered this idea, but there was no pressure to work on 
>it. Maybe now.
>
>Best regards
>Thomas
>
>>  	drm_client_buffer_vunmap(buffer);
>
>-- 
>Thomas Zimmermann
>Graphics Driver Developer
>SUSE Software Solutions Germany GmbH
>Maxfeldstr. 5, 90409 N?rnberg, Germany
>(HRB 36809, AG N?rnberg)
>Gesch?ftsf?hrer: Ivo Totev