On Tue, 03 Dec 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > Hi > > > Am 03.12.24 um 15:19 schrieb Jocelyn Falempe: >> On 03/12/2024 15:08, Jani Nikula wrote: >>> On Tue, 03 Dec 2024, Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote: >>>> On 03/12/2024 12:06, Jani Nikula wrote: >>>>> On Fri, 15 Nov 2024, Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote: >>>>>> Move the color conversions, blit and fill functions to drm_draw.c, >>>>>> so that they can be re-used by drm_log. >>>>>> drm_draw is internal to the drm subsystem, and shouldn't be used by >>>>>> gpu drivers. >>>>> >>>>> I started looking at this in patch 2: >>>>> >>>>>> +#include "../drm_draw.h" >>>>> >>>>> I think we should avoid #includes with ../ like this. >>>> >>>> Sure, I've added it in v8, after the clients moved to drm/clients/, but >>>> I didn't think much about it. >>>> >>>>> >>>>> Either drm_draw.h belongs in include/drm, or maybe clients/Makefile >>>>> needs to add subdir-ccflags-y += -I$(src)/.. or something like that? >>>>> >>>>> If it's supposed to be internal, I guess the latter, but then the >>>>> current convention is to have _internal.h suffix. All drm headers >>>>> under >>>>> drivers/ have that. >>>> >>>> ok, I can rename drm_draw.h to drm_draw_internal.h, and add the include >>>> in the Makefile. >>>>> >>>>> Is this the first drm subsystem internal thing that's a separate >>>>> module? >>>>> Should we use EXPORT_SYMBOL_NS() and MODULE_IMPORT_NS() to enforce it >>>>> being internal? >>>> >>>> It's not really a separate module, as it's built in the core drm >>>> module. >>>> (the reason is that it's used by drm_panic too, which must be in the >>>> core drm module). >>> >>> Right, sorry, I got confused and looked at this the other way round. >>> >>> drm_draw is part of drm.ko, drm log is part of drm_client_lib.ko, and >>> uses drm_draw, right? >> Yes, that's it. >>> >>> So is drm_draw really internal if drm client uses it? >> >> For me "internal" includes drm clients and kms helpers, but yes, it's >> not really clear. >> >>> >>> I don't really deeply care either way, but it bugs me when it's >>> somewhere in between. :) >>> >>> Either include/drm/drm_draw.h or drivers/gpu/drm/drm_draw_internal.h, >>> the latter *possibly* with symbol namespace, but not a big deal. >> >> The other reason for "internal" is that we should find a way to merge >> drm_draw, drm_format_helper, and some vkms conversion helper, that >> does similar things. So the less users, the easier it will be. > > Exactly. 'internal' definitely means not-drivers. And the last thing we > want is DRM drivers that draw framebuffers by themselves. When we unify > and harmonize the various blit and draw helpers, we can still loosen the > requirements if necessary. Then maybe we should have a "drm internal" symbol namespace for internal stuff that's spread across modules? I don't think it's a blocker for merging this series, but it would certainly raise some flags before someone accidentally starts using the stuff, them being exported and all. BR, Jani. -- Jani Nikula, Intel
