Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more functionality to the API #65

Merged
merged 10 commits into from
Aug 14, 2021
Merged

Add more functionality to the API #65

merged 10 commits into from
Aug 14, 2021

Conversation

Rain92
Copy link
Contributor

@Rain92 Rain92 commented Aug 12, 2021

These are my proposed changes for the FBInk API.

Some new members to the FBInkState struct:
uint8_t current_rota_native; // native screen rotaiton; vInfo.rotate (current rotation, c.f., <linux/fb.h>)
uint8_t current_rota_canonical; // canonical screen rotation

uint32_t screen_stride; // screen line length in bytes;

A new method to access the underlying framebuffer:
// Grants direct access to the frame buffer pointer as well as the size of the frame buffer allocation.
// If the framebuffer is not yet allocated it will do so and return the result of the operation.
FBINK_API int
fbink_get_fb_pointer(int fbfd, FBPtrInfo *fbInfo);

A new method to set the framebuffer rotation and bpp.
The code is mostly copied from fbdepth and probably needs some tinkering:
// Sets the framebuffer bits per pixel and rotation and invoke a reinit afterwards
// rota will be the rotation in natve format; use fbink_rota_canonical_to_native to convert it to canonical
// bpp will remain unchanged if the value is < 8
// req_gray will remain unchanged if the value is < 0
// on sunxi devices it will invoke fbink_sunxi_ntx_enforce_rota
FBINK_API int
fbink_set_fb_info(int fbFd, int32_t bpp, int8_t rota, int32_t req_gray,
const FBInkConfig* restrict fbink_cfg);


This change is Reviewable

@NiLuJe
Copy link
Owner

NiLuJe commented Aug 12, 2021

I'll have more time this week-end, so, very, very quick comments:

  • Not a fan of the split current_rota_native/current_rota_canonical, as there's nothing private about one of these: it's just the other run through a public API call ;).

  • I'd probably mark the exported pointer const to document the fact that the pointer itself should be kept around untouched (or even export a pointer to the internal fbPtr to be able to track the mapping state; with the same const markings).

  • Will review the fbdepth stuff, that's definitely long overdue ;). (utils/dump.c has similar code that was a bit clunkier because not relying on private APIs ;p).

  • The get_fb_data stuff probably ought to export const pointers to const data to vInfo & fInfo, because PocketBook suffers from the same inconsistencies we discussed ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Aug 12, 2021

* The get_fb_data stuff probably ought to export const pointers to const data to vInfo & fInfo, because PocketBook suffers from the same inconsistencies we discussed ;).

Then again, the rota/bitdepth stuff isn't really supported on PB (as it's even quirkier than on Kobo), and the only things one could really have missed were indeed line_length & smem_len ;).

Plus, I really like the get_fb_ptr name, and that wouldn't fit as well if it did other stuff ;p.

@Rain92
Copy link
Contributor Author

Rain92 commented Aug 13, 2021

I agree it's not really nesassary to expose vInfo & fInfo right now (except for the color format in vinfo but let's just ignore that).
I can rename it to get_fb_ptr.
Do you propose to put the allocation size into the FBInkState struct instead?

@Rain92
Copy link
Contributor Author

Rain92 commented Aug 13, 2021

I am not sure how to mark the exported pointer as const tho as it has to be written somewhere if it is passed as an argument.
And if it is passed as the return value it would be a bit wanky because the method has to return an exit code.

@NiLuJe
Copy link
Owner

NiLuJe commented Aug 13, 2021

I was vaguely thinking of something along the lines of unsigned char* fbink_get_fb_pointer(int fbfd, size_t* alloc_size);, i.e., the alloc size as an outptr, and returning NULL on error ;).

EDIT: Can't const rvalues anyway, brain is fried ^^.

The only way I can think of to return a const something would be with an unwieldier double pointer with something like an unsigned char * restrict const * fb_ptr; member in the struct set to &fbPtr. But that's a bit icky ;p.

TL;DR: I'll sleep on it some more. Not super fond of the brand new struct for this, but I don't hate it either, hence the proposed signature above. And I do like keeping this separate and those two together (so, no moving alloc_size to FBInkState).

@NiLuJe
Copy link
Owner

NiLuJe commented Aug 13, 2021

As far as set_fbinfo is concerned (don't mind the names I'm using here, writing this quickly without actual reference, and nothing's set in stone ;p), I'd move it to fbink_rota_quirks.c.

As for its actual code, I've got a few quick notes jotted down, but I'll probably simply handle some minor cleanup post-merge myself, as the API looks fine ;).

I do like the sunxi shortcut for rota, I hadn't quite thought it through yet, and my original plan was simply to throw an ENOTSUP on sunxi, but this is much better, and actually makes sense ;p.

EDIT: I would possibly just move rota before bpp, as leaving bpp & gray alone is going to be far more common than tweaking rota, and bpp & gray kinda go hand in hand anyway, so I like 'em close together ;).

(FWIW, part of the stuff I jotted down involves making the magic "don't change me" values actual defines to make that clearer in the API docs).

@NiLuJe NiLuJe merged commit fd1a518 into NiLuJe:master Aug 14, 2021
@NiLuJe
Copy link
Owner

NiLuJe commented Aug 14, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants