boundarydevices / linux

Kernel tree for Boundary Devices platforms
194 stars 290 forks source link

tc358743: Fix RGB888 1920x1080@60 #68

Closed ThbaMG closed 2 years ago

ThbaMG commented 2 years ago

Hello there,

since this is my first pull request please be gentle in pointing me into the right direction :)

During a project with a custom board i have attached a tc358743 to an imx8mm board via mipi csi. It took me quit some time to get the driver working for numerours lane and resolution configurations from 640x480 to 1920x1080.

I encountered some issues in the driver which i am using from your 5.10 repo which in my opinion should be corrected for this driver to work with RGB888 and higher lane numbers.

I had to fix the lane calculation since the prior one ignores hsync and vsync, however these bits have to be transmitted as well. This breaks the correct lane calculation if the frequency comes close to the maximum lane frequency. This leads to corrupted data which will be with correct crc.

The incoming FIFO size has been set to 16. In the header file it is stated that toshiba recommends using 300 as default. Without increasing this value the stream may skip some bytes from time to time if the input signal is not stable enough and has some jitter on it. This may even be caused by the tx35873 itself not being able to detect the pixel clk properly which seems to happen from time to time.

To be able to drive 1920x1080@60 in RGB888 with 3 bytes per pixel i had to increase the link-frequency since the prior one seems to work for YUV422 which transmits two pixels every four bytes. Therefore i integrated the patch from https://www.spinics.net/lists/linux-media/msg122228.html to support the higher frequencies.

I hope this patch is useful and may spare some time for the next poor soul who has to figure that out.

Best Regards,

Thomas

gibsson commented 2 years ago

Hi, Thank you for your contribution, it is highly appreciated! Now, since it's your first contribution, here are some guidelines you need to comply to: 1- always sign-off your commits: https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff 2- have a proper commit title + log, your current title is too long and doesn't even include the name of the driver you are fixing. https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes 3- make sure to also use the proper indentation for your commit: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#style-check-your-changes 4- when you use the patch from someone else well use that patch, don't copy it, just cherry-pick it instead so that it keeps the ownership of its author So please cherry-pick the original patch you rely on, then have a follow-up patch with your own changes, explaining why (your text should be in the commit log, not in the pull request text). Regards, Gary

ThbaMG commented 2 years ago

Hi Gary,

thank you for the hints and guildeline.

Since my patch does not fully work without the base work of another contributor i will have to contact the author and figure out how to integrate that since it was a patch from a mailing list. Unfortunately i have not found the patch in a repo so it is not clear to me how to properly integrate that in my working repo without losing the ownership of the author.

I will issue a new pull request as soon as i have figured out how to properly integrate the base patch from another author or do you have a suggestion on how to handle this?

Regards,

Thomas

gibsson commented 2 years ago

Hi,

Since it has been sent on the mailing list you can always retrieve the original patch from patchwork (see mbox button): https://patchwork.linuxtv.org/project/linux-media/patch/6de475044437c01c14429ac20292e5c29fdd39f9.1505826082.git.dave.stevenson@raspberrypi.org/ https://patchwork.linuxtv.org/project/linux-media/patch/3e638375aff788b24f988e452214649d6100a596.1505826082.git.dave.stevenson@raspberrypi.org/

Regards, Gary

ThbaMG commented 2 years ago

Hi,

I reworked everything you suggested. However i need to rework the integrated patch since your kernel version differs from the one the patch was intended to be applied to. Is there a rule on how to do that properly since a direct git am import does not work due to failed hunks. Since i would change the patch to make it fit how does that affect the author tracking.

Regards,

Thomas

gibsson commented 2 years ago

Hi, You need to fix the merge issues (with mergetool or others) and add your Signed-off-by to the original commit to show you reworked it. Then apply your other changes as a follow-up patch. Regards,

ThbaMG commented 2 years ago

Hi,

i tried my very best in applying everything. I hope this set makes more sense and meets the requirements. Hopefully i did everything right this time.

Regards,

Thomas

gibsson commented 2 years ago

Hi,

Thanks, you're almost there. Now please rebase your work on top of the branch and remove the first commits that you reverted, I only want 3 commits in the PR: 1- Increase FIFO 2- Add 972 link freq 3- fix for RGB888 for 1920x1080 Also for that 3rd patch, please rework the log as it includes explanation covered by the first 2 that are unnecessary, please make it only about the computation of nb of lanes.

Thanks, Gary

ThbaMG commented 2 years ago

Hi,

there you go, i was a bit afraid if i would break the pr complete by the force push but it seemed to work out.

Regards,

Thomas

gibsson commented 2 years ago

Hi Thomas,

Thank you very much for this, I will integrate and check the changes don't affect the YUV cases.

Regards, Gary