From: Jani Nikula <jani.nikula@intel.com> To: "Thomas Zimmermann" <tzimmermann@suse.de>, "David Gow" <davidgow@google.com>, "José Expósito" <jose.exposito89@gmail.com> Cc: magalilemes00@gmail.com, "David Airlie" <airlied@linux.ie>, "Maíra Canal" <maira.canal@usp.br>, "Daniel Latypov" <dlatypov@google.com>, "Javier Martinez Canillas" <javierm@redhat.com>, dri-devel@lists.freedesktop.org, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, tales.aparecida@gmail.com, "Isabella Basso" <isabbasso@riseup.net>, "KUnit Development" <kunit-dev@googlegroups.com> Subject: Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro Date: Tue, 21 Jun 2022 13:13:18 +0300 [thread overview] Message-ID: <87fsjys4sh.fsf@intel.com> (raw) In-Reply-To: <045f480b-9f47-6f10-9e5d-4436335b272e@suse.de> On Tue, 21 Jun 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 21.06.22 um 11:38 schrieb David Gow: >> On Tue, Jun 21, 2022 at 12:06 AM José Expósito >> <jose.exposito89@gmail.com> wrote: >>> >>> Add a helper macro to initialize a rectangle from x, y, width and >>> height information. >>> >>> Reviewed-by: Jani Nikula <jani.nikula@intel.com> >>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: José Expósito <jose.exposito89@gmail.com> >>> --- >> >> This looks good to me, though I have one minor concern about the macro >> name. (But if it's okay with the DRM folks, which it seems to be, I >> won't object.) >> >> Either way, >> Reviewed-by: David Gow <davidgow@google.com> >> >>> include/drm/drm_rect.h | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>> index 6f6e19bd4dac..e8d94fca2703 100644 >>> --- a/include/drm/drm_rect.h >>> +++ b/include/drm/drm_rect.h >>> @@ -47,6 +47,22 @@ struct drm_rect { >>> int x1, y1, x2, y2; >>> }; >>> >>> +/** >>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h >>> + * @x: x coordinate >>> + * @y: y coordinate >>> + * @w: width >>> + * @h: height >>> + * >>> + * RETURNS: >>> + * A new rectangle of the specified size. >>> + */ >>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \ >>> + .x1 = (x), \ >>> + .y1 = (y), \ >>> + .x2 = (x) + (w), \ >>> + .y2 = (y) + (h) }) >>> + >> >> My only slight concern here is that it might be a little bit confusing >> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the >> actual struct drm_rect is x1/y1/x2/y2. If the macro were called >> something like DRM_RECT_INIT_FROM_XYWH() or similar. > > The existing drm_rect_init() function uses xywh arguments. So the > current name is consistent with existing practice. I don't think we > refer to x2,y2 much, if ever. Agreed, and if we initialized with x1,y1,x2,y2 we wouldn't need the function/macro in the first place. BR, Jani. > > Best regards > Thomas > >> >> >>> /** >>> * DRM_RECT_FMT - printf string for &struct drm_rect >>> */ >>> -- >>> 2.25.1 >>> -- Jani Nikula, Intel Open Source Graphics Center
WARNING: multiple messages have this Message-ID (diff)
From: Jani Nikula <jani.nikula@intel.com> To: "Thomas Zimmermann" <tzimmermann@suse.de>, "David Gow" <davidgow@google.com>, "José Expósito" <jose.exposito89@gmail.com> Cc: dri-devel@lists.freedesktop.org, magalilemes00@gmail.com, "David Airlie" <airlied@linux.ie>, "Maíra Canal" <maira.canal@usp.br>, "Daniel Latypov" <dlatypov@google.com>, "Javier Martinez Canillas" <javierm@redhat.com>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, tales.aparecida@gmail.com, "Isabella Basso" <isabbasso@riseup.net>, "KUnit Development" <kunit-dev@googlegroups.com> Subject: Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro Date: Tue, 21 Jun 2022 13:13:18 +0300 [thread overview] Message-ID: <87fsjys4sh.fsf@intel.com> (raw) In-Reply-To: <045f480b-9f47-6f10-9e5d-4436335b272e@suse.de> On Tue, 21 Jun 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 21.06.22 um 11:38 schrieb David Gow: >> On Tue, Jun 21, 2022 at 12:06 AM José Expósito >> <jose.exposito89@gmail.com> wrote: >>> >>> Add a helper macro to initialize a rectangle from x, y, width and >>> height information. >>> >>> Reviewed-by: Jani Nikula <jani.nikula@intel.com> >>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: José Expósito <jose.exposito89@gmail.com> >>> --- >> >> This looks good to me, though I have one minor concern about the macro >> name. (But if it's okay with the DRM folks, which it seems to be, I >> won't object.) >> >> Either way, >> Reviewed-by: David Gow <davidgow@google.com> >> >>> include/drm/drm_rect.h | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>> index 6f6e19bd4dac..e8d94fca2703 100644 >>> --- a/include/drm/drm_rect.h >>> +++ b/include/drm/drm_rect.h >>> @@ -47,6 +47,22 @@ struct drm_rect { >>> int x1, y1, x2, y2; >>> }; >>> >>> +/** >>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h >>> + * @x: x coordinate >>> + * @y: y coordinate >>> + * @w: width >>> + * @h: height >>> + * >>> + * RETURNS: >>> + * A new rectangle of the specified size. >>> + */ >>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \ >>> + .x1 = (x), \ >>> + .y1 = (y), \ >>> + .x2 = (x) + (w), \ >>> + .y2 = (y) + (h) }) >>> + >> >> My only slight concern here is that it might be a little bit confusing >> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the >> actual struct drm_rect is x1/y1/x2/y2. If the macro were called >> something like DRM_RECT_INIT_FROM_XYWH() or similar. > > The existing drm_rect_init() function uses xywh arguments. So the > current name is consistent with existing practice. I don't think we > refer to x2,y2 much, if ever. Agreed, and if we initialized with x1,y1,x2,y2 we wouldn't need the function/macro in the first place. BR, Jani. > > Best regards > Thomas > >> >> >>> /** >>> * DRM_RECT_FMT - printf string for &struct drm_rect >>> */ >>> -- >>> 2.25.1 >>> -- Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2022-06-21 10:13 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-20 16:06 [PATCH v4 0/3] KUnit tests for drm_format_helper José Expósito 2022-06-20 16:06 ` José Expósito 2022-06-20 16:06 ` [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro José Expósito 2022-06-20 16:06 ` José Expósito 2022-06-21 9:38 ` David Gow 2022-06-21 9:38 ` David Gow 2022-06-21 10:02 ` Thomas Zimmermann 2022-06-21 10:02 ` Thomas Zimmermann 2022-06-21 10:13 ` Jani Nikula [this message] 2022-06-21 10:13 ` Jani Nikula 2022-06-22 7:12 ` David Gow 2022-06-22 7:12 ` David Gow 2022-06-20 16:06 ` [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332() José Expósito 2022-06-20 16:06 ` José Expósito 2022-06-21 9:38 ` David Gow 2022-06-21 9:38 ` David Gow 2022-06-21 17:37 ` José Expósito 2022-06-21 17:37 ` José Expósito 2022-06-20 16:06 ` [PATCH v4 3/3] drm/doc: Add KUnit documentation José Expósito 2022-06-20 16:06 ` José Expósito 2022-06-21 9:38 ` David Gow 2022-06-21 9:38 ` David Gow 2022-06-21 18:15 ` José Expósito 2022-06-21 18:15 ` José Expósito
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87fsjys4sh.fsf@intel.com \ --to=jani.nikula@intel.com \ --cc=airlied@linux.ie \ --cc=davidgow@google.com \ --cc=dlatypov@google.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=isabbasso@riseup.net \ --cc=javierm@redhat.com \ --cc=jose.exposito89@gmail.com \ --cc=kunit-dev@googlegroups.com \ --cc=linux-kernel@vger.kernel.org \ --cc=magalilemes00@gmail.com \ --cc=maira.canal@usp.br \ --cc=tales.aparecida@gmail.com \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.