[MPlayer-dev-eng] [PATCH] SUN XVR-100 VO driver 4. try

[Balatoni Denes]

I have attached a patch which implements most of Diego's and Albeu's and 
Reimar's suggestions.

Thursday 26 July 2007 17:22-kor Reimar Döffinger ezt írta:
> Hello,
>
> You use a pfb_ prefix everywhere IMO that is really pointless and just
> bloats the text, and might even be really confusing since name of the vo
> is xvr100, not pfb.

pfb is the solaris driver's name, xvr-100 is the cards name. There is a prefix 
intentionally, because these should be in the VOs private context, if one 
existed. But if one day VOs have private contexts, it will (would) 
be easier to convert to priv->stuff . It is bad design, that VOs are not 
reentrant and are using static variables.
So this should stay, this is my opinion.

> On Mon, Jul 23, 2007 at 08:27:22PM +0200, Balatoni Denes wrote:
> > @@ -144,6 +147,9 @@
> >
> >  vo_functions_t* video_out_drivers[] =
> >  {
> > +#ifdef HAVE_XVR100
> > +        &video_out_xvr100,
> > +#endif
> >  #ifdef HAVE_TDFX_VID
> >          &video_out_tdfx_vid,
> >  #endif
>
> I doubt it is justified putting this before all other vos, IMO at least xv
> if available should be tried first.

1) It is faster than XV.
2) xv is not even available in sparc solaris as of now (remember, this is for 
SPARC Solaris).



> > +#define PFB_OV0_V_INC 0x109
> > +#define PFB_OV0_P1_V_ACCUM_INIT 0x10A
> > +#define PFB_OV0_P23_V_ACCUM_INIT 0x10B
> > +#define PFB_OV0_P1_BLANK_LINES_AT_TOP 0x10C
> > +#define PFB_OV0_P23_BLANK_LINES_AT_TOP 0x10D
>
> Where do these come from? They aren't in some system or vidix header
> already somewhere?

Well, obviously this driver does the same thing as the vidix radeon driver 
(only worse). But it was much easier to port this driver from XINE (the 
hardware driver parts are from XINE, if you have not realised from the 
copyright notice), the way it is than to correctly port vidix to sparc 
solaris (I am actually thinking of the later, but I don't know enough of 
these proprietary solaris framebuffer drivers to do it). 
So yes, probably they are available, but I won't find as it could be redundant 
later, and it would be a half solution.

> > +static int pfb_buffer1, pfb_buffer2, pfb_buffer3;
> > +static int pfb_stride1, pfb_stride2, pfb_stride3;
>
> Why not use an array?

Done.

> > +static short int pfb_wx0, pfb_wy0, pfb_wx1, pfb_wy1;
> > +static int pfb_xres,pfb_yres;
> > +static int pfb_free_top;
> > +static int pfb_fs;
> > +static int pfb_usecolorkey=0;
> > +static uint32_t pfb_colorkey;
>
> Why not reuse vo_fs, vo_dx, vo_dy, vo_screenwidth, vo_screenheight and
> vo_colorkey?
> vo_colorkey contains the information of both pfb_colorkey and
> pfb_usecolorkey, if (vo_colorkey & 0xff000000) != 0 then colorkey is
> disabled.

Well, the colorkey is passed to the driver by the xover 
interface. Otherwise I don't know how they should be used, but feel free to 
change it.

> > +void pfb_overlay_on() {
>
> All functions (unless part of the vo API like reinit, config etc.) should
> have doxygen-compatible comments.

They should, but almost nothing else have doxygen comments in mplayer. I 
understand you think it would be cool, so feel free to add doxygen comments 
to mplayer, instead of directing me to do more extra work.

> > +/* This actually doesn't change a thing, width greater than 1536 still
> > won't work... */ +#if 0
> > +    /* if the source width was larger than what would fit in overlay
> > scaler increase step_by values */
>
> Well, I'd be surprised if the scaler didn't always read a full line at
> once into its buffer, so this just can't work. IMO remove it.

Yes, as Albeu suggested, this is removed, and a warning is issued if width is 
greater than 1536.

> > +    pfb_vregs[PFB_OV0_REG_LOAD_CNTL] = PFB_OV0_REG_LOAD_LOCK;
> > +    while (!(pfb_vregs[PFB_OV0_REG_LOAD_CNTL] &
> > PFB_OV0_REG_LOAD_LOCK_READBACK)) {}
>
> This is not time critical I think, so you should sleep here instead of
> doing such a tight busy loop.

Done.

> > +    pfb_vregs[PFB_OV0_P1_H_ACCUM_INIT] = (((0x00005000 + h_inc) << 7) &
> > 0x000f8000) | (((0x00005000 + h_inc) << 15) & 0xf0000000); +   
> > pfb_vregs[PFB_OV0_P23_H_ACCUM_INIT] = (((0x0000A000 + h_inc) << 6) &
> > 0x000f8000) | (((0x0000A000 + h_inc) << 14) & 0x70000000);
>
> I am quite sure this can be simplified, and esp. in a way that makes it
> clearer how to extend it to non-YV12.
> I mean e.g.
> (0x0000A000 + h_inc) << 6 == (0x00005000 + h_inc/2) << 7.
> And it can be & 0xf0000000 in both cases, does not make any difference.
> I do not know how you came to that code, so maybe it makes sense how you
> did it (then you should add comments to explain it), otherwise this is
> simpler (making heavy use of h_inc <= 0x1fff):
> // handle high-part of h_inc
> h_inc >>= 8;
> pfb_vregs[PFB_OV0_P1_H_ACCUM_INIT] = ((h_inc ^ 0x10) << 15) | ((h_inc &
> 0x10) << 24) | (1 << 29); h_inc >>= 1;
> pfb_vregs[PFB_OV0_P23_H_ACCUM_INIT] = ((h_inc ^ 0x10) << 15) | ((h_inc &
> 0x10) << 24) | (1 << 29);

As I said, this is from xine. In xine non YV12 is handled the same way, so I 
don't think it's a problem. If you want to rewrite it to something that looks 
better to you, feel free (it's GPL after all)

> > +    if (pfb_usecolorkey) {
> > +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_LOW] = pfb_colorkey;
> > +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_HIGH] = pfb_colorkey |
> > 0xff000000; +        pfb_vregs[PFB_OV0_KEY_CNTL] = PFB_OV0_KEY_EN;
> > +    } else {
> > +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_LOW] = 0;
> > +        pfb_vregs[PFB_OV0_GRPH_KEY_CLR_HIGH] = 0 | 0xff000000;
> > +        pfb_vregs[PFB_OV0_KEY_CNTL] = 0x010;
> > +    }
>
> Why is PFB_OV0_KEY_EN a define but 0x010 not? And do you really need to
> set the colour key when you disable it?

0x10 is from trial and error. I have removed setting the colorkey when it's 
not used (it still works).

> > +    pfb_buffer1 = pfb_free_top -
> > (pfb_stride1*pfb_srcheight+pfb_stride2*((pfb_srcheight+1) & ~1));
>
> You should check that this does not underflow.

Done (the contents of the framebuffer could still be messed up, but that's 
life).

> > +    pfb_free_top = attr.fbtype.fb_size - 0x2000;
>
> Hm... this seems really weird to me, please comment the idea behind
> this.

This is, again, from xine. As I don't have any good documentation on the sparc 
solaris framebuffer drivers (and there might be none), let's 
assume they knew what they were doing.

> > +    page_size = sysconf(_SC_PAGE_SIZE);
> > +
> > +    if ((pfb_vbase = mmap(NULL, (((PFB_VRAM_MMAPLEN + page_size - 1) /
> > page_size) * page_size), PROT_READ | PROT_WRITE, MAP_SHARED, pfb_devfd,
> > PFB_VRAM_MMAPBASE)) == MAP_FAILED) {
>
> According to the man page there is no reason to page-align the length
> argument of mmap. And for VRAM it is probably a given anyway.

Ok, removed.

> > +static uint32_t get_image(mp_image_t *mpi){
> > +    return VO_FALSE;
> > +#if 0 /* doesn't work for some reason */
>
> If it doesn't work you should find out why, it might hide some bug. If
> you need help, explain more precisely why it doesn't work.
> And anyway, this code does not have to work 100% flawless, it can be
> disable at runtime via -nodr (which btw. is default) anyway.

Albeu has answered this, and now it is enabled, because it works with TEMP 
buffer.

> > +static int draw_slice(uint8_t *src[], int stride[], int w,int h,int
> > x,int y) +{
> > +    mem2agpcpy_pic(pfb_vbase + pfb_buffer1 + pfb_stride1 * y + x,
> > src[0], w, h, pfb_stride1, stride[0]); +    mem2agpcpy_pic(pfb_vbase +
> > pfb_buffer2 + pfb_stride2 * (y>>1) + (x>>1), src[1], (w>>1), (h>>1),
> > pfb_stride2, stride[1]); +    mem2agpcpy_pic(pfb_vbase + pfb_buffer3 +
> > pfb_stride2 * (y>>1) + (x>>1), src[2], (w>>1), (h>>1), pfb_stride2,
> > stride[2]);
>
> Just do
> x >>= 1; y >>= 1;
> w >>= 1; h >>= 1;
> after the part handling the Y case IMO.
> Makes it easier to extend to non-YV12 as well.

Ok, done.

> Greetings,
> Reimar Döffinger

Dear MPlayer developers, my free time and motivation beutifying the code to 
your taste lasted for so long, now you are on your own.
i have already spent alltogether like 3 hours correcting the code according to 
your (Diego, Reimar and You) suggestions. This is quite a lot considering 
that the driver has always been working reasonable well, and the coding rules 
were obeyed from the beginning (except for a few slipped in trailing 
whitspaces and a broken configure part). I won't spend more time on it 
anymore. Maybe in the future I will fix  outstanding issues like the lack of 
UYVY support  (this is in fact a problem with MJPEGs) though. 

Meanwhile I think you should commit the driver, as it - to emphasize once 
more - works reasonably well.

best regards
Denes

-------------- next part --------------
A non-text attachment was scrubbed...
Name: xvr100_try4.diff
Type: text/x-diff
Size: 17466 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070726/fc0ea761/attachment.diff>