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:   Mon, 05 Sep 2022 14:50:38 +0200
Message-ID: <87leqyj881.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Takashi Iwai <tiwai@suse.de>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/12] drm/udl: Drop unneeded alignment
In-Reply-To: <05f4a358-70df-18f4-1672-f02c9eebe98f@suse.de>
References: <20220816153655.27526-1-tiwai@suse.de>
        <20220816153655.27526-9-tiwai@suse.de>
        <05f4a358-70df-18f4-1672-f02c9eebe98f@suse.de>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 05 Sep 2022 10:40:58 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > The alignment of damaged area was needed for the original udlfb driver
> > that tried to trim the superfluous copies between front and backend
> > buffers and handle data in long int.  It's not the case for udl DRM
> > driver, hence we can omit the whole unneeded alignment, as well as the
> > dead code.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_modeset.c  | 34 ++++++-------------------
> >   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
> >   2 files changed, 8 insertions(+), 66 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..bca31c890108 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
> >   	return __ffs(cpp);
> >   }
> >   -static int udl_aligned_damage_clip(struct drm_rect *clip, int x,
> > int y,
> > -				   int width, int height)
> > -{
> > -	int x1, x2;
> > -
> > -	if (WARN_ON_ONCE(x < 0) ||
> > -	    WARN_ON_ONCE(y < 0) ||
> > -	    WARN_ON_ONCE(width < 0) ||
> > -	    WARN_ON_ONCE(height < 0))
> > -		return -EINVAL;
> > -
> > -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> > -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> > -
> > -	clip->x1 = x1;
> > -	clip->y1 = y;
> > -	clip->x2 = x2;
> > -	clip->y2 = y + height;
> > -
> > -	return 0;
> > -}
> > -
> >   static int udl_handle_damage(struct drm_framebuffer *fb,
> >   			     const struct iosys_map *map,
> >   			     int x, int y, int width, int height)
> > @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> >   	struct drm_rect clip;
> >   	int log_bpp;
> >   +	if (width <= 0 || height <= 0)
> > +		return 0;
> > +
> 
> That shouldn't happen.
> 
> >   	ret = udl_log_cpp(fb->format->cpp[0]);
> >   	if (ret < 0)
> >   		return ret;
> >   	log_bpp = ret;
> >   -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> > -	if (ret)
> > -		return ret;
> > -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > +	clip.x1 = x;
> > +	clip.y1 = y;
> > +	clip.x2 = x + width;
> > +	clip.y2 = y + height;
> 
> drm_rect_init() please.
> 
> > +	if (clip.x2 > fb->width || clip.y2 > fb->height)
> 
> That's another thing that should not happen. The damage clips in the
> plane state is what you what to copy. The DRM helpers ensure that
> these various plane, fb and clip coordinates add up.

OK, then we can drop those clip size checks completely.
Will do that in v2 patch.


thanks,

Takashi