[PATCH v8 1/6] drm/panic: Move drawing functions to drm_draw

[Thomas Zimmermann]

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 at redhat.com> wrote:
>>> On 03/12/2024 12:06, Jani Nikula wrote:
>>>> On Fri, 15 Nov 2024, Jocelyn Falempe <jfalempe at redhat.com> 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.

Best regards
Thomas

>
> Best regards,
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)