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 support for freescale i.MX6 #5202

Merged
merged 2 commits into from Sep 2, 2014
Merged

ADD support for freescale i.MX6 #5202

merged 2 commits into from Sep 2, 2014

Conversation

@wolfgar
Copy link
Contributor

wolfgar commented Aug 12, 2014

Hi

Here is a PR (split as 2 commits to ease reviewing) which adds support for the Freescale i.MX6 SystemOnChip.
I initiated this work a little more than one year ago in this thread. Then, to ease collaboration, this porting effort moved to github repo when Chris contributed the rendering through vivante extensions.

This result is a collaborative work and I would like to thank especially @koying, @smallint and @warped-rudi who helped a lot with this development. Also maintainers of Geexbox, archlinux arm and openelec helped a lot.

This code is known to work on all i.MX6 variants and has been run successfully on a large number of boards/devices.

Sorry if everything is not ready for inclusion : I am fully available to take into account all your remarks or to provide explanations to enable this merge.

Stéphan

@koying
Copy link
Contributor

koying commented Aug 12, 2014

/cc @davilla @sraue @elupus

The codec is enabled with "imxvpu" in --enable-codec. The code is inactive otherwise.
Linux only for now.

@sraue
Copy link
Member

sraue commented Aug 12, 2014

+1 for including helix (and improve step by step over the time, like its done with RPi)
will make a build later today for testing (the version from some days ago is working so far)

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Aug 13, 2014
@avjui
Copy link

avjui commented Aug 18, 2014

+1 for including

@@ -2607,6 +2840,15 @@ bool CLinuxRendererGLES::Supports(EINTERLACEMETHOD method)
if(method == VS_INTERLACEMETHOD_AUTO)
return true;

if(m_renderMethod & RENDER_IMXMAP)
{
if(method == VS_INTERLACEMETHOD_DEINTERLACE

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

you should not abuse those existing methods. VS_INTERLACEMETHOD_DEINTERLACE activates yadif.

This comment has been minimized.

Copy link
@smallint

smallint Aug 24, 2014

Member

What is your recommendation? Should we add new IMX6 specific enumerations? We actually need four of them (Low Motion, High Motion, Low Motion Double Rate, High Motion Double Rate). If adding new ones is advisable what is the correct way to also add corresponding string representations?

@piotrasd
Copy link

piotrasd commented Aug 18, 2014

+1 for including

@@ -103,6 +114,7 @@ CLinuxRendererGLES::YUVBUFFER::YUVBUFFER()
#if defined(TARGET_ANDROID)
mediacodec = NULL;
#endif
codecinfo = NULL;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

should be split into its own commit if not directly related to MX6

}

#endif
}

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

please add an new line between those blocks

@@ -2739,5 +2984,16 @@ void CLinuxRendererGLES::AddProcessor(CDVDMediaCodecInfo *mediacodec, int index)
}
#endif

void CLinuxRendererGLES::AddProcessor(CDVDVideoCodecBuffer *codecinfo, int index)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

no entry in ReleaseBuffer required ?

@MartijnKaijser
Copy link
Member

MartijnKaijser commented Aug 18, 2014

Guys. It's asked to some specific devs. Stop doing +1 or it will be rejected

@@ -25,6 +25,7 @@
#include <vector>
#include <string>
#include "cores/VideoRenderers/RenderFormats.h"
#include "DVDVideoCodecInfo.h"

This comment has been minimized.

Copy link
@FernetMenta

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

btw: codecinfo is IMO not a good name for this class.

*
*/

#ifndef DVDVIDEOCODECINFO_H

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

we use pragma once

(nativeGuess = CreateEGLNativeType<CEGLNativeTypeRaspberryPI>(implementation)))
(nativeGuess = CreateEGLNativeType<CEGLNativeTypeRaspberryPI>(implementation))
#ifdef HAS_IMXFB
|| (nativeGuess = CreateEGLNativeType<CEGLNativeTypeIMX>(implementation))

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 18, 2014

Contributor

why do you change the coding style?

@wolfgar
Copy link
Contributor Author

wolfgar commented Aug 18, 2014

@FernetMenta
Thanks a lot for reviewing : I am taking into account your remarks and will push correction on the branch quickly...

edit 20/08 : I have locally taken into account all your remarks : I still have to run non regression testing. As soon as I have run them, I update the PR...

@wolfgar
Copy link
Contributor Author

wolfgar commented Aug 21, 2014

I have just pushed changes that should take remarks into account
As stated in the comment, I will submit the imx deinterlacing stuff with proper dedicated methods in a PR following this one
Do not hesitate to tell if additional changes are required or if I have misunderstood something...
Also maybe you would prefer that I squash the last commit in the previous ones ?

@AdamWill
Copy link

AdamWill commented Aug 21, 2014

I'm curious as to why the ProbeResolutions function in the egl native type throws out all resolutions that don't start with "S":

if(!StringUtils::StartsWith(probe_str[i], "S:"))
    continue;
if(ModeToResolution(probe_str[i], &res))
    resolutions.push_back(res);

I can't find the meaning of the possible letters there (my Cubox has modes prefixed with "D", "S", "U" and "V") documented anywhere, so I'm not sure where to ask other than here why the ones other than "S" are getting thrown out. This prevents XBMC using the correct resolution on my Cubox when using an HDMI->DVI cable (as both 1920x1080 and 1280x720 modes show up as "D", not "S").

@FernetMenta
Copy link
Contributor

FernetMenta commented Aug 21, 2014

@wolfgar yes, please squash the last commit. I cannot say much about the codec implementation itself but the interface to the application looks ok now.

@wolfgar wolfgar force-pushed the xbmc-imx6:pr-imx branch from 7ed99d4 to 752bc2d Aug 22, 2014
@wolfgar wolfgar force-pushed the xbmc-imx6:pr-imx branch from 752bc2d to fba4372 Aug 22, 2014
@wolfgar
Copy link
Contributor Author

wolfgar commented Aug 22, 2014

@FernetMenta : Thanks again, I have just updated the branch with the updated commits.

As a reminder here are the changes I pushed following reviewing :

  • Remove imx deinterlacing support : It will be reworked and submitted later as a new PR with dedicated methods for imx deinterlacing
  • class CDVDVideoCodecBuffer removed and specific CDVDVideoCodecIMXBuffer used
  • Renaming of codecinfo to IMXBuffer
  • Useless includes removed
  • Remove the IMXFB define as linux/mxcfb.h is no longer used
  • A few cosmetics changes (remove trailing blanks, insert missing newlines...)
@wolfgar
Copy link
Contributor Author

wolfgar commented Aug 22, 2014

@AdamWill :
To answer your question regarding letter prefixes for modes, the answer is in the kernel file drivers/video/fbsysfs.c

static int mode_string(char *buf, unsigned int offset,
               const struct fb_videomode *mode)
{
    char m = 'U';
    char v = 'p';

    if (mode->flag & FB_MODE_IS_DETAILED)
        m = 'D';
    if (mode->flag & FB_MODE_IS_VESA)
        m = 'V';
    if (mode->flag & FB_MODE_IS_STANDARD)
        m = 'S';

So now the remaining question is when is the flag FB_MODE_IS_DETAILED applied ?
Answer is : when it is a CEA detailed timing out of edid...
So you are right these values should not be evinced I guess
I file an issue to track it...

@topfs2
Copy link
Member

topfs2 commented Aug 25, 2014

CreateIMXPTexture isn't define guarded. Is this intentional?
Otherwise it seems very safe to pull

@topfs2
Copy link
Member

topfs2 commented Aug 25, 2014

It looks like it's implemented as the other ones so should be OK. So I will pull in next window unless someone voice against

@topfs2
Copy link
Member

topfs2 commented Aug 25, 2014

Jenkins build this please

@topfs2 topfs2 self-assigned this Aug 25, 2014
@topfs2 topfs2 added the Helix label Aug 25, 2014
@Memphiz
Copy link
Member

Memphiz commented Aug 25, 2014

sorry @topfs2 its still case sensitive - i can't change it :) jenkins build this please

@topfs2
Copy link
Member

topfs2 commented Aug 25, 2014

@Memphiz I can't make heads or tails of the log? Seems to be something jenkins related no?

@MartijnKaijser
Copy link
Member

MartijnKaijser commented Aug 25, 2014

jenkins build this please

@wolfgar
Copy link
Contributor Author

wolfgar commented Aug 25, 2014

@topfs2 Regarding CreateIMXMAPTexture : it should be buildable without additional guard...
I have just seen the Rpi build has failed but I don't think it is related.
If any additional change is required, do not hesitate to ask for them...

@topfs2 topfs2 added the Approved label Aug 28, 2014
@topfs2
Copy link
Member

topfs2 commented Sep 1, 2014

jenkins build this please

topfs2 added a commit that referenced this pull request Sep 2, 2014
ADD support for freescale i.MX6
@topfs2 topfs2 merged commit 8b1ac02 into xbmc:master Sep 2, 2014
1 check passed
1 check passed
default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.