[Mesa-dev] [PATCH 2/2] egl_dri2: Adding swrast and refactoring

[Feng, Haitao]

Hi Kristian,

Thanks for your comments. I will follow the mesa development process to add swrast into egl_dri2.

I have used the OO approach to refactor your egl_dri2 driver. Now drm and x11_dri2 shared the dri2_dri2 part, and x11_dri2 and x11_swrast shared the X11 related part. So I have made x11_dri2 inherit both dri2_dri2 and X11. There is a diamond after refactoring and 9 files in the dri2 folder.

If we do not want to split this file to that much, and only need 4 files:
  egl_dri2.h - header file for everything
  egl_dri2.c - shared code
  platform_x11.c code related to the x11 platform
  platform_drm.c code related to the drm platform

It is doable. I will move all the dri2 related to egl_dri2.c including dri2_dri2 and dri2_swrast. I will do this when I am back to office after the Chinese New Year Holiday vacation :)

Thanks
-Haitao

-----Original Message-----
From: hoegsberg at gmail.com [mailto:hoegsberg at gmail.com] On Behalf Of Kristian Høgsberg
Sent: 2011年2月4日 0:55
To: Feng, Haitao
Cc: mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 2/2] egl_dri2: Adding swrast and refactoring

On Sun, Jan 30, 2011 at 3:05 AM, Feng, Haitao <haitao.feng at intel.com> wrote:
> From f723112e2d10429e9691a547baeab588d45511d6 Mon Sep 17 00:00:00 2001
> From: Haitao Feng <haitao.feng at intel.com>
> Date: Sat, 29 Jan 2011 23:50:01 -0800
> Subject: [PATCH 2/2] egl_dri2: Adding swrast and refactoring
>
> This enables the egl_dri2 driver to load swrast driver
> for software rendering. It could be used when hardware
> dri2 driver is not available, such as in VM. After the
> swrast enabling, I refactor the code as egl image functions
> are not supported in swrast and egl surface functions
> are not supported in drm. After refactoring, egl_dri2.c
> contains functions shared between drm, x11_dri2 and
> x11_swrast. dri2_shared.c contains functions shared between
> drm and x11_dri2. x11_shared.c contains functions shared
> between x11_dri2 and x11_swrast.
>
> Note: this is a candidate for the 7.9 branch.

Hi Haitao,

Thanks for adding swrast support to egl_dri2.c.  The overall approach
looks good, though I'm not sure we need to split it up quite that
much.  However for something like this we really need to break it down
into a series of patches.  The patch here splits out the different
platforms and then adds the swrast functionality, but it would be
better if it was a patch to split out shared code, a patch split out
the x11 platform, and patch to split out the drm platform, and then
the patch to add swrast.  It makes it easer to review the patches
since all the code motion is in separate patches and the meat of the
series, the swrast addition, will have each own patch.  It also makes
bisecting possible and you're less likely to hit the mailing size
limit.

Also, the patch looks like it's against mesa 7.9.  I know that's what
we ship in meego and where we need the swrast functionality, but for
submitting upstream, we need a patch against mesa master.  We can ship
the patch in the meego rpm, but we can't add a feature like this in a
stable mesa branch.  The patch doesn't apply against mesa master, and
since a lot changed in egl_dri2.c since 7.9, I think the best approach
is to just redo the split and then add the swrast functionality.  I've
split up egl_dri2.c in mesa master like this:

  egl_dri2.h - header file for everything
  egl_dri2.c - shared code
  platform_x11.c code related to the x11 platform
  platform_drm.c code related to the drm platform

and I imagine we can just add swrast in platform_x11.c.  Is that
doable you think?

Kristian