On 7/27/22 10:24, Thomas Zimmermann wrote:
> Hi
>
> Am 25.07.22 um 18:23 schrieb Javier Martinez Canillas:
>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>> Move some of simpledrm's functionality into a helper library. Other
>>> drivers for firmware-provided framebuffers will also need functions
>>> to handle fixed modes and color formats, or update the back buffer.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> ---
>>
>> Nice patch!
>
> TBH it took me 3 tries to get something done for this library and I'm
> still not happy with the result. I want to share code between simpledrm
> and ofdrm, but that turns out to be harder then expected. A good part of
> this code appears to belong into other libraries (you also mentioned
> this below).
>
> I don't want to duplicated code between simpledrm and ofdrm without
> reason, but I expect that this library will somewhen be refactored and
> dissolved into existing libraries.
>
Yes, I think is a step in the right direction and guess it would be even
more useful once/if a 3rd firmware-provided framebuffer driver is added.
>
>>
>> [...]
>>
>>> +
>>> +/**
>>> + * DOC: overview
>>> + *
>>> + * The Firmware Framebuffer library FWFB provides helpers for devices with
>>> + * fixed-mode backing storage. It helps drivers to export a display mode of
>>> + * te correct size and copy updates to the backing storage.
>>
>> the
>>
>> it is "backing storage" or "backing store" ? I always thought that storage was
>> used for non-volatile media while "store" could be volatile and non-volatile.
>
> Why store? Isn't that a little shop for fashion or groceries? I'm no
> native speaker; I can't tell if either implies that we're sending
> pictures to a warehouse or bakery. :)
>
LOL.
> Would 'back buffer' (in contrast to 'shadow buffer') be clear?
>
Back buffer is more clear indeed.
[...]
>> It seems a little bit arbitrary to me that format is the only field that's
>> a pointer and the other ones are embedded into the struct drm_fwfb. Any
>> reason for that or is just a consequence of how types were used by the
>> simpledrm_device_create() function before that code moved into helpers ?
>
> Format is constant and comes from statically initialized memory in
> drm_fourcc.c. I'd expect to be able to compare formats by comparing the
> pointers. Copying the format here would break the assumption.
>
I see. Makes sense.
>>
>> [...]
>>
>>> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
>>> +{
>>> + const uint32_t *fourccs_end = fourccs + nfourccs;
>>> +
>>> + while (fourccs < fourccs_end) {
>>> + if (*fourccs == fourcc)
>>> + return true;
>>> + ++fourccs;
>>> + }
>>> + return false;
>>> +}
>>
>> This seems a helper that could be useful besides the drm_fwfb_helper.c file.
>>
>> I believe patches 1-6 shouldn't wait for the others in this series and could
>> just be merged when ready. Patches 7-10 can follow later.
>
> Yeah, I'd like to move patches 1 to 5 into a new series for merging.
> Patch 6 is only useful for ofdrm and as I said, maybe there's a better
> solution then this library. I'd rather keep it here for now.
>
OK.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
