Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/2013 11:55 PM, Rob Clark wrote:

On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com  wrote:

- TDA998x irq handling - ignored
- TDA998x sync fix - ignored


At least the sync fix, looks like I missed it (it probably is a good
idea to CC me if you want me to look at it).  Looks like there was
some follow-up discussion on both patches, unless I missed seeing a
newer version of those patches.


Yeah, I will update the patch for current mainline linux as it was
based on some of Russell's RFC patches.


Sometimes if you think a patch has been ignored/forgotten, it doesn't
hurt to ping on mailing list or #dri-devel..  a lot of us are working
not just on kernel (the relatively small part in the whole linux
graphics stack), but also mesa and/or x11.  Some times things end up
several pages down in the mail folder.  It's not because we are all
sitting on a beach drinking margaritas, or because we don't like you.
It is just because we are busy and missed it.

Last few months I've been pretty buried in r/e + gallium driver for
new gpu, so I wasn't always checking dri-devel list every day.  At
least now I am in drm-driver mode again ;-)


It is ok, I don't blame anyone here. Darren wants to test it on tilcdc.
I also found a datasheet for TDA9983b which is kind of compatible but
has more information about register layout in it. IIRC digikey also has
it.


- Fix drm I2C slave encoder probing


If you have a better idea about how to make the slave encoder probing
better (and/or more generic to support stuff other than i2c), please
send RFC patch.  (And if you already did this, please send updated
version, see previous point about sometimes missing patches.)


Also, will send an updated fix.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Tue, Jul 02, 2013 at 09:01:55PM +0300, Ville Syrjälä wrote:
 On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
  On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
  li...@arm.linux.org.uk wrote:
   | Default Colorimetry
   |
   | ...
   |
   | 480p, 480i, 576p, 576i, 240p and 288p
   |
   | The default colorimetry for all 480-line, 576-line, 240-line, and 
   288-line
   | video formats described in CEA-861-D is based on SMPTE 170M.
   |
   | 1080i, 1080p and 720p
   |
   | The default colorimetry for high-definition video formats (1080i, 1080p 
   and
   | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
 
 I think this was pretty much copy pasted from CEA-861-D which is very
 vague.
 
 CEA-861-E is a bit better, and more clearly states that if the sink can
 receive YCbCr, then the source should use it by default for CE formats,
 and the default colorimetry depends on whether it's SD or HD. It also
 states that when transmitting IT or CE formats as RGB, the color space
 is the one defined in the EDID. CEA-861-D only made that statement for
 IT formats, and left the RGB CE format case out in the cold.

Actually, what I'm doing there is probably wrong when you consider
what is going on:

 Overlay (YUV) - YUV-RGB colorspace conversion
  |
  v
 Graphic (RGB) ---(colorkey) HDMI

These bits control the YUV-RGB colorspace conversion.  The is it 601
or 709 colorspace question applies more to the colorspace of the
overlay image.  As far as I can tell, that is unspecified within our
normal video playback programs - there's provision to communicate that
information (they certainly don't seem to look for any kind of Xv
attribute).

The is it computer or studio RGB question (I think - I can't say
because the documentation is hellishly poor, and you now have as much
information on this as I do) refers to the colorspace of the RGB side.

So, maybe I should move the YUV colorspace setting to be a drm_plane
property?  But then how do we know what format it is supposed to be?
Do we just pick one and hope it's right?  Do we try to autodetect it
from the size of the drm_plane framebuffer?  What if something
downscales a HD YUV framebuffer to something smaller because the
display is smaller?

What I can say is that I've watched many hours of content with my driver
and at 720p output resolution, I prefer it converting the YUV between
709 to studio RGB - otherwise the blacks are too black and I find that
I have to adjust the brightness/contrast to bring the black levels up
compared to a standard TV broadcast.

 Oh and the other thing someone should do is fix the intel Xv code to
 use BT.709 CSC matrix for HD content. I believe that code is hardcoded
 for BT.601 currently, which may explain the last weirdness reported in
 that CEA bug or ours.

How do you propose to switch between the two?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Ville Syrjälä]

On Tue, Jul 02, 2013 at 07:23:27PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 02, 2013 at 09:01:55PM +0300, Ville Syrj?l? wrote:
> > On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
> > > On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
> > >  wrote:
> > > > | Default Colorimetry
> > > > |
> > > > | ...
> > > > |
> > > > | 480p, 480i, 576p, 576i, 240p and 288p
> > > > |
> > > > | The default colorimetry for all 480-line, 576-line, 240-line, and 
> > > > 288-line
> > > > | video formats described in CEA-861-D is based on SMPTE 170M.
> > > > |
> > > > | 1080i, 1080p and 720p
> > > > |
> > > > | The default colorimetry for high-definition video formats (1080i, 
> > > > 1080p and
> > > > | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
> > 
> > I think this was pretty much copy pasted from CEA-861-D which is very
> > vague.
> > 
> > CEA-861-E is a bit better, and more clearly states that if the sink can
> > receive YCbCr, then the source should use it by default for CE formats,
> > and the default colorimetry depends on whether it's SD or HD. It also
> > states that when transmitting IT or CE formats as RGB, the color space
> > is the one defined in the EDID. CEA-861-D only made that statement for
> > IT formats, and left the RGB CE format case out in the cold.
> 
> Actually, what I'm doing there is probably wrong when you consider
> what is going on:
> 
>  Overlay (YUV) -> YUV->RGB colorspace conversion
>   |
>   v
>  Graphic (RGB) ---(colorkey)> HDMI
> 
> These bits control the YUV->RGB colorspace conversion.  The "is it 601
> or 709 colorspace" question applies more to the colorspace of the
> overlay image.  As far as I can tell, that is unspecified within our
> normal video playback programs - there's provision to communicate that
> information (they certainly don't seem to look for any kind of Xv
> attribute).
> 
> The "is it computer or studio RGB" question (I think - I can't say
> because the documentation is hellishly poor, and you now have as much
> information on this as I do) refers to the colorspace of the RGB side.
> 
> So, maybe I should move the YUV colorspace setting to be a drm_plane
> property?  But then how do we know what format it is supposed to be?
> Do we just pick one and hope it's right?  Do we try to autodetect it
> from the size of the drm_plane framebuffer?  What if something
> downscales a HD YUV framebuffer to something smaller because the
> display is smaller?

Yes a plane property would be the right thing to use here. I'm not sure
we need any automagic default in the kernel for this stuff. I'm thinking
we just default to BT.601 (or something else if the hardware is really
weird and doesn't do BT.601) and let userspace override if it wants.

My plan for such a property has thus far been an enum called "csc matrix"
(or something vaguely similar) and the values could just be something
like "BT.601", "BT.709" etc.

Calling the property "csc matrix" has one downside though; What if the
hardware CSC is fully programmable and we want to push an actual matrix
from userspace. That property might also like to be called "csc matrix",
so maybe we want try to come up with a better name for this first guy.
Or maybe it should be "csc matrix" = "custom" + "csc matrix custom" =
. There's also the question of the format of the
coefficients in the custom matrix. We may need some HW specifics
there...

> What I can say is that I've watched many hours of content with my driver
> and at 720p output resolution, I prefer it converting the YUV between
> 709 to studio RGB - otherwise the blacks are too black and I find that
> I have to adjust the brightness/contrast to bring the black levels up
> compared to a standard TV broadcast.
> 
> > Oh and the other thing someone should do is fix the intel Xv code to
> > use BT.709 CSC matrix for HD content. I believe that code is hardcoded
> > for BT.601 currently, which may explain the last weirdness reported in
> > that CEA bug or ours.
> 
> How do you propose to switch between the two?

An Xv port attribute should do. Google found me XV_COLORSPACE, which
seems to be the name the radeon folks picked for it.

-- 
Ville Syrj?l?
Intel OTC

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Ville Syrjälä]

On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
> On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
>  wrote:
> > On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
> >> On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> >> > +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
> >> > +{
> >> > +   struct drm_display_mode *adj = >crtc.mode;
> >> > +   uint32_t val = 0;
> >> > +
> >> > +   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
> >> > +   val |= CFG_CSC_YUV_CCIR709;
> >> > +   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
> >> > +   val |= CFG_CSC_RGB_STUDIO;
> >> > +
> >> > +   /*
> >> > +* In auto mode, set the colorimetry, based upon the HDMI spec.
> >> > +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
> >> > +* ITU601.  It may be more appropriate to set this depending on
> >> > +* the source - but what if the graphic frame is YUV and the
> >> > +* video frame is RGB?
> >> > +*/
> >> > +   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
> >> > +!(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
> >> > +   (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
> >> > +   if (dcrtc->csc_yuv_mode == CSC_AUTO)
> >> > +   val |= CFG_CSC_YUV_CCIR709;
> >> > +   }
> >> > +
> >> > +   /*
> >> > +* We assume we're connected to a TV-like device, so the YUV->RGB
> >> > +* conversion should produce a limited range.  We should set this
> >> > +* depending on the connectors attached to this CRTC, and what
> >> > +* kind of device they report being connected.
> >> > +*/
> >> > +   if (dcrtc->csc_rgb_mode == CSC_AUTO)
> >> > +   val |= CFG_CSC_RGB_STUDIO;
> >>
> >> In the intel driver we check whether it's an cea mode with
> >> drm_match_cea_mode, e.g. in intel_hdmi.c:
> >>
> >>   if (intel_hdmi->color_range_auto) {
> >>   /* See CEA-861-E - 5.1 Default Encoding Parameters */
> >>   if (intel_hdmi->has_hdmi_sink &&
> >>   drm_match_cea_mode(adjusted_mode) > 1)
> >>   intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
> >>   else
> >>   intel_hdmi->color_range = 0;
> >>   }
> >>
> >> (The color_range gets then propageted to the right place since different
> >> platforms have different ways to do that in intel hw).
> >
> > Unfortunately, this disagrees with the HDMI v1.3a specification:
> >
> > | Default Colorimetry
> > |
> > | ...
> > |
> > | 480p, 480i, 576p, 576i, 240p and 288p
> > |
> > | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
> > | video formats described in CEA-861-D is based on SMPTE 170M.
> > |
> > | 1080i, 1080p and 720p
> > |
> > | The default colorimetry for high-definition video formats (1080i, 1080p 
> > and
> > | 720p) described in CEA-861-D is based on ITU-R BT.709-5.

I think this was pretty much copy pasted from CEA-861-D which is very
vague.

CEA-861-E is a bit better, and more clearly states that if the sink can
receive YCbCr, then the source should use it by default for CE formats,
and the default colorimetry depends on whether it's SD or HD. It also
states that when transmitting IT or CE formats as RGB, the color space
is the one defined in the EDID. CEA-861-D only made that statement for
IT formats, and left the RGB CE format case out in the cold.

> > As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
> > CEA when dealing with HDMI specifics.
> >
> > So, according to the HDMI specification, the default is that it's only
> > 1080i, 1080p and 720p which are 709, the others are 601.  This does not
> > correspond with "drm_match_cea_mode(adjusted_mode) > 1".

We're mixing our apples and oranges here. The logic in i915 has to do
with the default RGB quantization range only.

> Hm, sounds like we need to improve stuff a bit, maybe add a common cea
> helper to the drm core with a bool is_hdmi to decide whether it's
> palin CEA or HDMI-CEA. Ville (who's written the current code and the
> match cea mode helper) can you please take a look?

Currently i915 only outputs RGB, so based on CEA-861-E setting C=00 is
the right thing to do. So I think the only thing we could improve is to
use YCbCr for CE formats by default, but first we need to implement
YCbCr output support :P

Oh and the other thing someone should do is fix the intel Xv code to
use BT.709 CSC matrix for HD content. I believe that code is hardcoded
for BT.601 currently, which may explain the last weirdness reported in
that CEA bug or ours.

> > Unfortunately, given DRM's structure, there's no way for the CRTC layer
> > to really know what its driving, and this setting is at the CRTC layer,
> > not the output layer.  It may be that you have the CRTC duplicated
> > between two different display devices with different characteristics,
> > which means you have to "choose" one - or just not bother.  I've chosen
> > 

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Tue, Jul 02, 2013 at 09:01:55PM +0300, Ville Syrj?l? wrote:
> On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
> > On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
> >  wrote:
> > > | Default Colorimetry
> > > |
> > > | ...
> > > |
> > > | 480p, 480i, 576p, 576i, 240p and 288p
> > > |
> > > | The default colorimetry for all 480-line, 576-line, 240-line, and 
> > > 288-line
> > > | video formats described in CEA-861-D is based on SMPTE 170M.
> > > |
> > > | 1080i, 1080p and 720p
> > > |
> > > | The default colorimetry for high-definition video formats (1080i, 1080p 
> > > and
> > > | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
> 
> I think this was pretty much copy pasted from CEA-861-D which is very
> vague.
> 
> CEA-861-E is a bit better, and more clearly states that if the sink can
> receive YCbCr, then the source should use it by default for CE formats,
> and the default colorimetry depends on whether it's SD or HD. It also
> states that when transmitting IT or CE formats as RGB, the color space
> is the one defined in the EDID. CEA-861-D only made that statement for
> IT formats, and left the RGB CE format case out in the cold.

Actually, what I'm doing there is probably wrong when you consider
what is going on:

 Overlay (YUV) -> YUV->RGB colorspace conversion
  |
  v
 Graphic (RGB) ---(colorkey)> HDMI

These bits control the YUV->RGB colorspace conversion.  The "is it 601
or 709 colorspace" question applies more to the colorspace of the
overlay image.  As far as I can tell, that is unspecified within our
normal video playback programs - there's provision to communicate that
information (they certainly don't seem to look for any kind of Xv
attribute).

The "is it computer or studio RGB" question (I think - I can't say
because the documentation is hellishly poor, and you now have as much
information on this as I do) refers to the colorspace of the RGB side.

So, maybe I should move the YUV colorspace setting to be a drm_plane
property?  But then how do we know what format it is supposed to be?
Do we just pick one and hope it's right?  Do we try to autodetect it
from the size of the drm_plane framebuffer?  What if something
downscales a HD YUV framebuffer to something smaller because the
display is smaller?

What I can say is that I've watched many hours of content with my driver
and at 720p output resolution, I prefer it converting the YUV between
709 to studio RGB - otherwise the blacks are too black and I find that
I have to adjust the brightness/contrast to bring the black levels up
compared to a standard TV broadcast.

> Oh and the other thing someone should do is fix the intel Xv code to
> use BT.709 CSC matrix for HD content. I believe that code is hardcoded
> for BT.601 currently, which may explain the last weirdness reported in
> that CEA bug or ours.

How do you propose to switch between the two?

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

> Of course, I share the idea of true, full-blown of_drm_something
> helpers. But the DT patch, is not an improvement but a real fix as in
> "make DRM not break on some platforms". From that on, I can start
> digging into DRM API and improve DT support here and there.
>
> So for the three patches I mentioned above, they are all minor
> improvements of the API. Minor, because I cannot ever sent _the_ single
> perfect patch just because I don't know the API well enough, yet.
>
> Just based on my personal experience: TDA998x driver got merged the way it
> is _although_ I addressed some concerns - the fixes are not merged
> _although_ I provided experimental results. This is of course
> disappointing for me, but I am sure it will work out soon and I will
> get back to happily send improvements for the platforms I can test on.

To be honest I've no idea what those tda998x patches could fix or
break, so they go on my no ideas list and I hope if they get reposted
someone will tell me what they do.

I could probably be more motivated towards poking other people to
review stuff, but I mostly hope in vain that the ARM people will cross
review things for other ARM chips, I had a bit of that happening for a
while at the start, but it seems to have died off. Now I'm mostly
relying on Rob to have some clue what I'm merging is sane.

My current priority for merging stuff is:
core patches that affect all platforms,
core patches that affect x86
drivers that I maintain by default
core patches that affect ARM
misc ARM drivers that fall through cracks.

Maybe I can persuade Rob to become a sub maintainer for all of the SoC
drivers but I suspect he'd try and hurt me in real life.

I'll take another look at the tda patches but I may still have no idea
what they are doing.

Dave.

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/2013 11:55 PM, Rob Clark wrote:
> On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
>   wrote:
>> - TDA998x irq handling - ignored
>> - TDA998x sync fix - ignored
>
> At least the sync fix, looks like I missed it (it probably is a good
> idea to CC me if you want me to look at it).  Looks like there was
> some follow-up discussion on both patches, unless I missed seeing a
> newer version of those patches.

Yeah, I will update the patch for current mainline linux as it was
based on some of Russell's RFC patches.

> Sometimes if you think a patch has been ignored/forgotten, it doesn't
> hurt to ping on mailing list or #dri-devel..  a lot of us are working
> not just on kernel (the relatively small part in the whole linux
> graphics stack), but also mesa and/or x11.  Some times things end up
> several pages down in the mail folder.  It's not because we are all
> sitting on a beach drinking margaritas, or because we don't like you.
> It is just because we are busy and missed it.
>
> Last few months I've been pretty buried in r/e + gallium driver for
> new gpu, so I wasn't always checking dri-devel list every day.  At
> least now I am in drm-driver mode again ;-)

It is ok, I don't blame anyone here. Darren wants to test it on tilcdc.
I also found a datasheet for TDA9983b which is kind of compatible but
has more information about register layout in it. IIRC digikey also has
it.

>> - Fix drm I2C slave encoder probing
>>
> If you have a better idea about how to make the slave encoder probing
> better (and/or more generic to support stuff other than i2c), please
> send RFC patch.  (And if you already did this, please send updated
> version, see previous point about sometimes missing patches.)

Also, will send an updated fix.

Sebastian

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Ville Syrjälä]

On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
 On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
  On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
   +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
   +{
   +   struct drm_display_mode *adj = dcrtc-crtc.mode;
   +   uint32_t val = 0;
   +
   +   if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
   +   val |= CFG_CSC_YUV_CCIR709;
   +   if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
   +   val |= CFG_CSC_RGB_STUDIO;
   +
   +   /*
   +* In auto mode, set the colorimetry, based upon the HDMI spec.
   +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
   +* ITU601.  It may be more appropriate to set this depending on
   +* the source - but what if the graphic frame is YUV and the
   +* video frame is RGB?
   +*/
   +   if ((adj-hdisplay == 1280  adj-vdisplay == 720 
   +!(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
   +   (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
   +   if (dcrtc-csc_yuv_mode == CSC_AUTO)
   +   val |= CFG_CSC_YUV_CCIR709;
   +   }
   +
   +   /*
   +* We assume we're connected to a TV-like device, so the YUV-RGB
   +* conversion should produce a limited range.  We should set this
   +* depending on the connectors attached to this CRTC, and what
   +* kind of device they report being connected.
   +*/
   +   if (dcrtc-csc_rgb_mode == CSC_AUTO)
   +   val |= CFG_CSC_RGB_STUDIO;
 
  In the intel driver we check whether it's an cea mode with
  drm_match_cea_mode, e.g. in intel_hdmi.c:
 
if (intel_hdmi-color_range_auto) {
/* See CEA-861-E - 5.1 Default Encoding Parameters */
if (intel_hdmi-has_hdmi_sink 
drm_match_cea_mode(adjusted_mode)  1)
intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
else
intel_hdmi-color_range = 0;
}
 
  (The color_range gets then propageted to the right place since different
  platforms have different ways to do that in intel hw).
 
  Unfortunately, this disagrees with the HDMI v1.3a specification:
 
  | Default Colorimetry
  |
  | ...
  |
  | 480p, 480i, 576p, 576i, 240p and 288p
  |
  | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
  | video formats described in CEA-861-D is based on SMPTE 170M.
  |
  | 1080i, 1080p and 720p
  |
  | The default colorimetry for high-definition video formats (1080i, 1080p 
  and
  | 720p) described in CEA-861-D is based on ITU-R BT.709-5.

I think this was pretty much copy pasted from CEA-861-D which is very
vague.

CEA-861-E is a bit better, and more clearly states that if the sink can
receive YCbCr, then the source should use it by default for CE formats,
and the default colorimetry depends on whether it's SD or HD. It also
states that when transmitting IT or CE formats as RGB, the color space
is the one defined in the EDID. CEA-861-D only made that statement for
IT formats, and left the RGB CE format case out in the cold.

  As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
  CEA when dealing with HDMI specifics.
 
  So, according to the HDMI specification, the default is that it's only
  1080i, 1080p and 720p which are 709, the others are 601.  This does not
  correspond with drm_match_cea_mode(adjusted_mode)  1.

We're mixing our apples and oranges here. The logic in i915 has to do
with the default RGB quantization range only.

 Hm, sounds like we need to improve stuff a bit, maybe add a common cea
 helper to the drm core with a bool is_hdmi to decide whether it's
 palin CEA or HDMI-CEA. Ville (who's written the current code and the
 match cea mode helper) can you please take a look?

Currently i915 only outputs RGB, so based on CEA-861-E setting C=00 is
the right thing to do. So I think the only thing we could improve is to
use YCbCr for CE formats by default, but first we need to implement
YCbCr output support :P

Oh and the other thing someone should do is fix the intel Xv code to
use BT.709 CSC matrix for HD content. I believe that code is hardcoded
for BT.601 currently, which may explain the last weirdness reported in
that CEA bug or ours.

  Unfortunately, given DRM's structure, there's no way for the CRTC layer
  to really know what its driving, and this setting is at the CRTC layer,
  not the output layer.  It may be that you have the CRTC duplicated
  between two different display devices with different characteristics,
  which means you have to choose one - or just not bother.  I've chosen
  the latter because it's a simpler solution, and is in no way any more
  correct than any other solution.
 
  This is also why these are exported to userspace via properties, so if
  userspace knows better, it can deal with those issues.
 
 Yeah, 

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Ville Syrjälä]

On Tue, Jul 02, 2013 at 07:23:27PM +0100, Russell King - ARM Linux wrote:
 On Tue, Jul 02, 2013 at 09:01:55PM +0300, Ville Syrjälä wrote:
  On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
   On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
   li...@arm.linux.org.uk wrote:
| Default Colorimetry
|
| ...
|
| 480p, 480i, 576p, 576i, 240p and 288p
|
| The default colorimetry for all 480-line, 576-line, 240-line, and 
288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
|
| 1080i, 1080p and 720p
|
| The default colorimetry for high-definition video formats (1080i, 
1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.
  
  I think this was pretty much copy pasted from CEA-861-D which is very
  vague.
  
  CEA-861-E is a bit better, and more clearly states that if the sink can
  receive YCbCr, then the source should use it by default for CE formats,
  and the default colorimetry depends on whether it's SD or HD. It also
  states that when transmitting IT or CE formats as RGB, the color space
  is the one defined in the EDID. CEA-861-D only made that statement for
  IT formats, and left the RGB CE format case out in the cold.
 
 Actually, what I'm doing there is probably wrong when you consider
 what is going on:
 
  Overlay (YUV) - YUV-RGB colorspace conversion
   |
   v
  Graphic (RGB) ---(colorkey) HDMI
 
 These bits control the YUV-RGB colorspace conversion.  The is it 601
 or 709 colorspace question applies more to the colorspace of the
 overlay image.  As far as I can tell, that is unspecified within our
 normal video playback programs - there's provision to communicate that
 information (they certainly don't seem to look for any kind of Xv
 attribute).
 
 The is it computer or studio RGB question (I think - I can't say
 because the documentation is hellishly poor, and you now have as much
 information on this as I do) refers to the colorspace of the RGB side.
 
 So, maybe I should move the YUV colorspace setting to be a drm_plane
 property?  But then how do we know what format it is supposed to be?
 Do we just pick one and hope it's right?  Do we try to autodetect it
 from the size of the drm_plane framebuffer?  What if something
 downscales a HD YUV framebuffer to something smaller because the
 display is smaller?

Yes a plane property would be the right thing to use here. I'm not sure
we need any automagic default in the kernel for this stuff. I'm thinking
we just default to BT.601 (or something else if the hardware is really
weird and doesn't do BT.601) and let userspace override if it wants.

My plan for such a property has thus far been an enum called csc matrix
(or something vaguely similar) and the values could just be something
like BT.601, BT.709 etc.

Calling the property csc matrix has one downside though; What if the
hardware CSC is fully programmable and we want to push an actual matrix
from userspace. That property might also like to be called csc matrix,
so maybe we want try to come up with a better name for this first guy.
Or maybe it should be csc matrix = custom + csc matrix custom =
actual matrix. There's also the question of the format of the
coefficients in the custom matrix. We may need some HW specifics
there...

 What I can say is that I've watched many hours of content with my driver
 and at 720p output resolution, I prefer it converting the YUV between
 709 to studio RGB - otherwise the blacks are too black and I find that
 I have to adjust the brightness/contrast to bring the black levels up
 compared to a standard TV broadcast.
 
  Oh and the other thing someone should do is fix the intel Xv code to
  use BT.709 CSC matrix for HD content. I believe that code is hardcoded
  for BT.601 currently, which may explain the last weirdness reported in
  that CEA bug or ours.
 
 How do you propose to switch between the two?

An Xv port attribute should do. Google found me XV_COLORSPACE, which
seems to be the name the radeon folks picked for it.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Rob Clark]

On Mon, Jul 1, 2013 at 5:26 PM, Dave Airlie  wrote:
>> Of course, I share the idea of true, full-blown of_drm_something
>> helpers. But the DT patch, is not an improvement but a real fix as in
>> "make DRM not break on some platforms". From that on, I can start
>> digging into DRM API and improve DT support here and there.
>>
>> So for the three patches I mentioned above, they are all minor
>> improvements of the API. Minor, because I cannot ever sent _the_ single
>> perfect patch just because I don't know the API well enough, yet.
>>
>> Just based on my personal experience: TDA998x driver got merged the way it
>> is _although_ I addressed some concerns - the fixes are not merged
>> _although_ I provided experimental results. This is of course
>> disappointing for me, but I am sure it will work out soon and I will
>> get back to happily send improvements for the platforms I can test on.
>
> To be honest I've no idea what those tda998x patches could fix or
> break, so they go on my no ideas list and I hope if they get reposted
> someone will tell me what they do.

IIRC, a few of those patches were breaking things on tilcdc, which
someone needs to get to the bottom of..  and for that I'm relying a
bit on Darren as he is the one with both hw to test with tilcdc plus
an hdmi tester.  And I think understands the issue better than I do at
this point.

> I could probably be more motivated towards poking other people to
> review stuff, but I mostly hope in vain that the ARM people will cross
> review things for other ARM chips, I had a bit of that happening for a
> while at the start, but it seems to have died off. Now I'm mostly
> relying on Rob to have some clue what I'm merging is sane.
>
> My current priority for merging stuff is:
> core patches that affect all platforms,
> core patches that affect x86
> drivers that I maintain by default
> core patches that affect ARM
> misc ARM drivers that fall through cracks.
>
> Maybe I can persuade Rob to become a sub maintainer for all of the SoC
> drivers but I suspect he'd try and hurt me in real life.

/me runs :-P

well, anyways, I do at least try to review arm patches.  Things
sometimes do fall on the floor when I'm busy in other areas.

BR,
-R

> I'll take another look at the tda patches but I may still have no idea
> what they are doing.
>
> Dave.
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Rob Clark]

On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
 wrote:
> On 07/01/13 02:01, Dave Airlie wrote:
>>
>> how about instead of writing:
>> "However, at least I've taken the time to_think_  about what I'm doing
>> and realise that there_is_  scope here for the DRM core to improve,
>>
>> rather than burying this stuff deep inside my driver like everyone else
>> has.  That's no reason to penalise patches from the "good guys" who think"
>>
>> you go with
>> "I noticed this piece of functionality could be refactored, here is a
>> patch adding them to
>> the core, does anyone think its a good idea?"
>
>
> Dave,
>
> at least on this point I do share Russell's impression. I've sent
> bunch of patches improving TDA998x and DRM+DT:
> - TDA998x irq handling - ignored
> - TDA998x sync fix - ignored

At least the sync fix, looks like I missed it (it probably is a good
idea to CC me if you want me to look at it).  Looks like there was
some follow-up discussion on both patches, unless I missed seeing a
newer version of those patches.

Sometimes if you think a patch has been ignored/forgotten, it doesn't
hurt to ping on mailing list or #dri-devel..  a lot of us are working
not just on kernel (the relatively small part in the whole linux
graphics stack), but also mesa and/or x11.  Some times things end up
several pages down in the mail folder.  It's not because we are all
sitting on a beach drinking margaritas, or because we don't like you.
It is just because we are busy and missed it.

Last few months I've been pretty buried in r/e + gallium driver for
new gpu, so I wasn't always checking dri-devel list every day.  At
least now I am in drm-driver mode again ;-)

> - Fix drm I2C slave encoder probing
>
> I am aware that this is not an easy job nor one you get much
> appreciation for. But, back when TDA998x driver was published,
> all my comments were basically answered with "Oh, I know. Maybe
> someday somebody will fix it".

If you have a better idea about how to make the slave encoder probing
better (and/or more generic to support stuff other than i2c), please
send RFC patch.  (And if you already did this, please send updated
version, see previous point about sometimes missing patches.)


BR,
-R


> I am not being paid for any of this, but have a strong intrinsic
> motivation here. But I am loosing interest in sending fixes for
> DRM stuff because my (personal) impression is the same Russell
> has: Depending on who sends patches, they get merged independent
> of how broken they are - others are discussed to death.
>
> Sebastian
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/13 11:42, Daniel Vetter wrote:
> On Mon, Jul 01, 2013 at 10:52:03AM +0200, Sebastian Hesselbarth wrote:
>> at least on this point I do share Russell's impression. I've sent
>> bunch of patches improving TDA998x and DRM+DT:
>> - TDA998x irq handling - ignored
>> - TDA998x sync fix - ignored
>> - Fix drm I2C slave encoder probing
>>
>> I am aware that this is not an easy job nor one you get much
>> appreciation for. But, back when TDA998x driver was published,
>> all my comments were basically answered with "Oh, I know. Maybe
>> someday somebody will fix it".
>
> I guess part of the problem here is that in the arm world we don't (yet)
> have many full-blown drivers and not many people to fix up stuff or chime
> in with good review. And sometimes that just means that someone puts down
> his foot and says "this is how we do it" - at least for drm/i915 I
> sometimes have to do that to unblock a massive bikeshed-fest.
>
>> I am not being paid for any of this, but have a strong intrinsic
>> motivation here. But I am loosing interest in sending fixes for
>> DRM stuff because my (personal) impression is the same Russell
>> has: Depending on who sends patches, they get merged independent
>> of how broken they are - others are discussed to death.
>
> Hm, we run fairly extensive QA for drm/i915, and thus far the drm core
> stuff hasn't really blown up badly for us. So can you please point at
> examples where crap was merged and shouldn't have been?

Don't get me wrong, "broken" above didn't mean it doesn't work at all,
but with SoC graphic drivers arising, requirements shift from "some
known implementations" to "you never know what will be combined with
e.g. a specific CRTC". So from that latter point-of-view, I consider
TDA998x for example as broken. It may work with tilcdc in some modes,
but as Darren Etheridge stated, it breaks as soon as you touch TDA998x
driver. At least for that, I confirmed the timings of Russell's driver
and the fixes posted with a scope and a bunch of modes and monitors/tv.

For the timing fix of TDA998x, Darren now is trying to also confirm
every single sync line of tilcdc and I really like to see his Tested-by
on the patch - just because Russell's driver and the CuBox are the only
testbeds I have on this.

> Wrt to bikeshed to death I know that drm folks are a bit prone to that (me
> included), but recently I haven't spotted a case where this happened. ARM
> stuff excluded ofc since I don't follow that too closely. There's also
> that Dave is sometimes a bit swamped, but pinging him on irc about lost
> patches works well (at least for stuff I care about).

Hmm, I understand that it is _very_ time consuming work on your side.
But for me - with limited insight of DRM core - it is not a good starter
to make the API DT aware. The DRM API documentation _is_ limited wrt
intentions of the way it is done.

Of course, I share the idea of true, full-blown of_drm_something
helpers. But the DT patch, is not an improvement but a real fix as in
"make DRM not break on some platforms". From that on, I can start
digging into DRM API and improve DT support here and there.

So for the three patches I mentioned above, they are all minor
improvements of the API. Minor, because I cannot ever sent _the_ single
perfect patch just because I don't know the API well enough, yet.

Just based on my personal experience: TDA998x driver got merged the way 
it is _although_ I addressed some concerns - the fixes are not merged
_although_ I provided experimental results. This is of course
disappointing for me, but I am sure it will work out soon and I will
get back to happily send improvements for the platforms I can test on.

Sebastian

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Mon, Jul 01, 2013 at 10:52:03AM +0200, Sebastian Hesselbarth wrote:
> On 07/01/13 02:01, Dave Airlie wrote:
> >how about instead of writing:
> >"However, at least I've taken the time to_think_  about what I'm doing
> >and realise that there_is_  scope here for the DRM core to improve,
> >rather than burying this stuff deep inside my driver like everyone else
> >has.  That's no reason to penalise patches from the "good guys" who think"
> >
> >you go with
> >"I noticed this piece of functionality could be refactored, here is a
> >patch adding them to
> >the core, does anyone think its a good idea?"
> 
> Dave,
> 
> at least on this point I do share Russell's impression. I've sent
> bunch of patches improving TDA998x and DRM+DT:
> - TDA998x irq handling - ignored
> - TDA998x sync fix - ignored
> - Fix drm I2C slave encoder probing
> 
> I am aware that this is not an easy job nor one you get much
> appreciation for. But, back when TDA998x driver was published,
> all my comments were basically answered with "Oh, I know. Maybe
> someday somebody will fix it".

I guess part of the problem here is that in the arm world we don't (yet)
have many full-blown drivers and not many people to fix up stuff or chime
in with good review. And sometimes that just means that someone puts down
his foot and says "this is how we do it" - at least for drm/i915 I
sometimes have to do that to unblock a massive bikeshed-fest.

> I am not being paid for any of this, but have a strong intrinsic
> motivation here. But I am loosing interest in sending fixes for
> DRM stuff because my (personal) impression is the same Russell
> has: Depending on who sends patches, they get merged independent
> of how broken they are - others are discussed to death.

Hm, we run fairly extensive QA for drm/i915, and thus far the drm core
stuff hasn't really blown up badly for us. So can you please point at
examples where crap was merged and shouldn't have been?

Wrt to bikeshed to death I know that drm folks are a bit prone to that (me
included), but recently I haven't spotted a case where this happened. ARM
stuff excluded ofc since I don't follow that too closely. There's also
that Dave is sometimes a bit swamped, but pinging him on irc about lost
patches works well (at least for stuff I care about).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/13 02:01, Dave Airlie wrote:
> how about instead of writing:
> "However, at least I've taken the time to_think_  about what I'm doing
> and realise that there_is_  scope here for the DRM core to improve,
> rather than burying this stuff deep inside my driver like everyone else
> has.  That's no reason to penalise patches from the "good guys" who think"
>
> you go with
> "I noticed this piece of functionality could be refactored, here is a
> patch adding them to
> the core, does anyone think its a good idea?"

Dave,

at least on this point I do share Russell's impression. I've sent
bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored
- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much
appreciation for. But, back when TDA998x driver was published,
all my comments were basically answered with "Oh, I know. Maybe
someday somebody will fix it".

I am not being paid for any of this, but have a strong intrinsic
motivation here. But I am loosing interest in sending fixes for
DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent
of how broken they are - others are discussed to death.

Sebastian

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

On Mon, Jul 1, 2013 at 10:17 AM, Russell King - ARM Linux
 wrote:
> On Mon, Jul 01, 2013 at 10:01:30AM +1000, Dave Airlie wrote:
>> OMG I'm working in a subsystem where stuff is being developed, with only
>> a few resources! I know my full time job isn't maintaining a 500,000
>> line subsystem,
>> and the sub maintainers and developers do a great job refactoring
>> where required.
>>
>> lots of other driver authors have made substantial changes to the drm core as
>> they've written drivers, you spot one bit of refactoring and its a
>> major shortcoming
>> of the entire subsystem and development community.
>>
>> how about instead of writing:
>> "However, at least I've taken the time to _think_ about what I'm doing
>> and realise that there _is_ scope here for the DRM core to improve,
>> rather than burying this stuff deep inside my driver like everyone else
>> has.  That's no reason to penalise patches from the "good guys" who think"
>>
>> you go with
>> "I noticed this piece of functionality could be refactored, here is a
>> patch adding them to
>> the core, does anyone think its a good idea?"
>>
>> As Daniel pointed out every driver submitted has refactored things,
>> even exynos did a
>> lot of work to be the first ARM driver we shipped, the DRM core
>> doesn't write itself and
>> I fully expect driver authors to be the ones that tell me what needs
>> refactoring, since
>> they are on the pointy end, but to state that you are the only person
>> ever to think about
>> things is frankly being a dick.
>>
>> Lets try for less dick, and more productive in future.
>
> And you can stick your dick back where you got it from David.  Really,
> your response is totally uncalled for.
>
> Let's try realising that I only have very limited time to put into this
> and unlike the other submitters who have been _paid_ to get their code
> sorted, this is being done purely for free - which means it's really
> low priority.  As you should already realise because I've stated already
> that I *really* don't care whether it gets into mainline or not.

Really not every driver maintainer is paid to submit their drivers, if
you'd any clue
you'd understand that you aren't special. We currently have a number of non-paid
maintainer drivers being submitted and worked on, via for one, gma500
is maintain
in spare time, the tegra driver isn't purely a work of paid
dedication, I maintain
5 drivers currently, and I certainly don't do all of that in my day
job. Keeping attitude off
this list is what I do, I don't care if you don't appreciate the work
put in by other developers
to the drm subsystem, but I do and I won't have someone come in here
saying "you guys
suck" when clearly they've no historical perspective and as such are
plainly trolling.

Yes the DRM has growing pains, yes there is a lots of legacy shit in
the way of making
things clean, yes writing a driver for SoC is more painful than
necessary, but overall
its a majorly moving target, 5 years ago DRM/KMS hadn't considered ARM, now its
probably most of what we have to consider.

> If you want stuff to be refactored in DRM, you need to find someone
> with more time to do it.

I pointed out I didn't want stuff refactored, I wanted a driver author
to suggest
possible refactorings with a patch to add the interfaces, and suggest its a
good idea.

Split the patch, one to add the additions to the core without changing
anything, then just
have your driver use them. If other driver writes want to use them they'll
figure it out, and maybe someone else will go around and clean up the other
drivers.

I'm really hoping the OLPC guys pick up this work and care enough to
merge it, if not,
its a bit of a waste.

Dave.

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

On Sun, Jun 30, 2013 at 10:52 PM, Russell King - ARM Linux
 wrote:
> On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
>> On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
>> > +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
>> > +{
>> > +   struct drm_display_mode *adj = >crtc.mode;
>> > +   uint32_t val = 0;
>> > +
>> > +   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
>> > +   val |= CFG_CSC_YUV_CCIR709;
>> > +   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
>> > +   val |= CFG_CSC_RGB_STUDIO;
>> > +
>> > +   /*
>> > +* In auto mode, set the colorimetry, based upon the HDMI spec.
>> > +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
>> > +* ITU601.  It may be more appropriate to set this depending on
>> > +* the source - but what if the graphic frame is YUV and the
>> > +* video frame is RGB?
>> > +*/
>> > +   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
>> > +!(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
>> > +   (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
>> > +   if (dcrtc->csc_yuv_mode == CSC_AUTO)
>> > +   val |= CFG_CSC_YUV_CCIR709;
>> > +   }
>> > +
>> > +   /*
>> > +* We assume we're connected to a TV-like device, so the YUV->RGB
>> > +* conversion should produce a limited range.  We should set this
>> > +* depending on the connectors attached to this CRTC, and what
>> > +* kind of device they report being connected.
>> > +*/
>> > +   if (dcrtc->csc_rgb_mode == CSC_AUTO)
>> > +   val |= CFG_CSC_RGB_STUDIO;
>>
>> In the intel driver we check whether it's an cea mode with
>> drm_match_cea_mode, e.g. in intel_hdmi.c:
>>
>>   if (intel_hdmi->color_range_auto) {
>>   /* See CEA-861-E - 5.1 Default Encoding Parameters */
>>   if (intel_hdmi->has_hdmi_sink &&
>>   drm_match_cea_mode(adjusted_mode) > 1)
>>   intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
>>   else
>>   intel_hdmi->color_range = 0;
>>   }
>>
>> (The color_range gets then propageted to the right place since different
>> platforms have different ways to do that in intel hw).
>
> Unfortunately, this disagrees with the HDMI v1.3a specification:
>
> | Default Colorimetry
> |
> | ...
> |
> | 480p, 480i, 576p, 576i, 240p and 288p
> |
> | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
> | video formats described in CEA-861-D is based on SMPTE 170M.
> |
> | 1080i, 1080p and 720p
> |
> | The default colorimetry for high-definition video formats (1080i, 1080p and
> | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
>
> As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
> CEA when dealing with HDMI specifics.
>
> So, according to the HDMI specification, the default is that it's only
> 1080i, 1080p and 720p which are 709, the others are 601.  This does not
> correspond with "drm_match_cea_mode(adjusted_mode) > 1".
>
> Unfortunately, given DRM's structure, there's no way for the CRTC layer
> to really know what its driving, and this setting is at the CRTC layer,
> not the output layer.  It may be that you have the CRTC duplicated
> between two different display devices with different characteristics,
> which means you have to "choose" one - or just not bother.  I've chosen
> the latter because it's a simpler solution, and is in no way any more
> correct than any other solution.
>
> This is also why these are exported to userspace via properties, so if
> userspace knows better, it can deal with those issues.
>
>> > +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
>> > *dev,
>> > +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
>> > +{
>> > +   struct armada_framebuffer *dfb;
>> > +   uint8_t format, config;
>> > +   int ret;
>> > +
>> > +   switch (mode->pixel_format) {
>> > +#define FMT(drm, fmt, mod) \
>> > +   case DRM_FORMAT_##drm:  \
>> > +   format = CFG_##fmt; \
>> > +   config = mod;   \
>> > +   break
>> > +   FMT(RGB565, 565,CFG_SWAPRB);
>> > +   FMT(BGR565, 565,0);
>> > +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
>> > +   FMT(ABGR1555,   1555,   0);
>> > +   FMT(RGB888, 888PACK,CFG_SWAPRB);
>> > +   FMT(BGR888, 888PACK,0);
>> > +   FMT(XRGB,   X888,   CFG_SWAPRB);
>> > +   FMT(XBGR,   X888,   0);
>> > +   FMT(ARGB,   ,   CFG_SWAPRB);
>> > +   FMT(ABGR,   ,   0);
>> > +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
>> > +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
>> > +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
>> > +   FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
>> > +   FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Mon, Jul 01, 2013 at 10:01:30AM +1000, Dave Airlie wrote:
> OMG I'm working in a subsystem where stuff is being developed, with only
> a few resources! I know my full time job isn't maintaining a 500,000
> line subsystem,
> and the sub maintainers and developers do a great job refactoring
> where required.
> 
> lots of other driver authors have made substantial changes to the drm core as
> they've written drivers, you spot one bit of refactoring and its a
> major shortcoming
> of the entire subsystem and development community.
> 
> how about instead of writing:
> "However, at least I've taken the time to _think_ about what I'm doing
> and realise that there _is_ scope here for the DRM core to improve,
> rather than burying this stuff deep inside my driver like everyone else
> has.  That's no reason to penalise patches from the "good guys" who think"
> 
> you go with
> "I noticed this piece of functionality could be refactored, here is a
> patch adding them to
> the core, does anyone think its a good idea?"
> 
> As Daniel pointed out every driver submitted has refactored things,
> even exynos did a
> lot of work to be the first ARM driver we shipped, the DRM core
> doesn't write itself and
> I fully expect driver authors to be the ones that tell me what needs
> refactoring, since
> they are on the pointy end, but to state that you are the only person
> ever to think about
> things is frankly being a dick.
> 
> Lets try for less dick, and more productive in future.

And you can stick your dick back where you got it from David.  Really,
your response is totally uncalled for.

Let's try realising that I only have very limited time to put into this
and unlike the other submitters who have been _paid_ to get their code
sorted, this is being done purely for free - which means it's really
low priority.  As you should already realise because I've stated already
that I *really* don't care whether it gets into mainline or not.

I am *NOT* planning on spending any time on this during the next week
as I have *paid* work to do, and probably not next weekend nor the
following week either.  So the next time that I'm likely to do anything
on this is in a fortnight or longer.

If you want stuff to be refactored in DRM, you need to find someone
with more time to do it.

And before you continue making a mountain out of a molehill, as you
seem to like doing, the "let's make it a core thing" is REALLY BLOODY
SIMPLE.  All it takes is for the header file to go into include/drm.
It's now available to anyone who wants to use that - and all that's
left is to sort out all those drivers *WHICH I CANT TEST* to use
that stuff.

And, frankly, when I was going to post this latest RFC, I gave serious
consideration to quite simply not posting it to dri-devel and copying
you or any other person listed as a maintainer for DRM, and only posting
it to the people interested in it on the ARM side, because I'd already
given up hope of it ever being merged into mainline.

Your totally over the top and rediculous response just confirms that.

Further postings will now omit you and dri-devel.

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Mon, Jul 01, 2013 at 10:52:03AM +0200, Sebastian Hesselbarth wrote:
 On 07/01/13 02:01, Dave Airlie wrote:
 how about instead of writing:
 However, at least I've taken the time to_think_  about what I'm doing
 and realise that there_is_  scope here for the DRM core to improve,
 rather than burying this stuff deep inside my driver like everyone else
 has.  That's no reason to penalise patches from the good guys who think
 
 you go with
 I noticed this piece of functionality could be refactored, here is a
 patch adding them to
 the core, does anyone think its a good idea?
 
 Dave,
 
 at least on this point I do share Russell's impression. I've sent
 bunch of patches improving TDA998x and DRM+DT:
 - TDA998x irq handling - ignored
 - TDA998x sync fix - ignored
 - Fix drm I2C slave encoder probing
 
 I am aware that this is not an easy job nor one you get much
 appreciation for. But, back when TDA998x driver was published,
 all my comments were basically answered with Oh, I know. Maybe
 someday somebody will fix it.

I guess part of the problem here is that in the arm world we don't (yet)
have many full-blown drivers and not many people to fix up stuff or chime
in with good review. And sometimes that just means that someone puts down
his foot and says this is how we do it - at least for drm/i915 I
sometimes have to do that to unblock a massive bikeshed-fest.

 I am not being paid for any of this, but have a strong intrinsic
 motivation here. But I am loosing interest in sending fixes for
 DRM stuff because my (personal) impression is the same Russell
 has: Depending on who sends patches, they get merged independent
 of how broken they are - others are discussed to death.

Hm, we run fairly extensive QA for drm/i915, and thus far the drm core
stuff hasn't really blown up badly for us. So can you please point at
examples where crap was merged and shouldn't have been?

Wrt to bikeshed to death I know that drm folks are a bit prone to that (me
included), but recently I haven't spotted a case where this happened. ARM
stuff excluded ofc since I don't follow that too closely. There's also
that Dave is sometimes a bit swamped, but pinging him on irc about lost
patches works well (at least for stuff I care about).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Mon, Jul 01, 2013 at 10:01:30AM +1000, Dave Airlie wrote:
 OMG I'm working in a subsystem where stuff is being developed, with only
 a few resources! I know my full time job isn't maintaining a 500,000
 line subsystem,
 and the sub maintainers and developers do a great job refactoring
 where required.
 
 lots of other driver authors have made substantial changes to the drm core as
 they've written drivers, you spot one bit of refactoring and its a
 major shortcoming
 of the entire subsystem and development community.
 
 how about instead of writing:
 However, at least I've taken the time to _think_ about what I'm doing
 and realise that there _is_ scope here for the DRM core to improve,
 rather than burying this stuff deep inside my driver like everyone else
 has.  That's no reason to penalise patches from the good guys who think
 
 you go with
 I noticed this piece of functionality could be refactored, here is a
 patch adding them to
 the core, does anyone think its a good idea?
 
 As Daniel pointed out every driver submitted has refactored things,
 even exynos did a
 lot of work to be the first ARM driver we shipped, the DRM core
 doesn't write itself and
 I fully expect driver authors to be the ones that tell me what needs
 refactoring, since
 they are on the pointy end, but to state that you are the only person
 ever to think about
 things is frankly being a dick.
 
 Lets try for less dick, and more productive in future.

And you can stick your dick back where you got it from David.  Really,
your response is totally uncalled for.

Let's try realising that I only have very limited time to put into this
and unlike the other submitters who have been _paid_ to get their code
sorted, this is being done purely for free - which means it's really
low priority.  As you should already realise because I've stated already
that I *really* don't care whether it gets into mainline or not.

I am *NOT* planning on spending any time on this during the next week
as I have *paid* work to do, and probably not next weekend nor the
following week either.  So the next time that I'm likely to do anything
on this is in a fortnight or longer.

If you want stuff to be refactored in DRM, you need to find someone
with more time to do it.

And before you continue making a mountain out of a molehill, as you
seem to like doing, the let's make it a core thing is REALLY BLOODY
SIMPLE.  All it takes is for the header file to go into include/drm.
It's now available to anyone who wants to use that - and all that's
left is to sort out all those drivers *WHICH I CANT TEST* to use
that stuff.

And, frankly, when I was going to post this latest RFC, I gave serious
consideration to quite simply not posting it to dri-devel and copying
you or any other person listed as a maintainer for DRM, and only posting
it to the people interested in it on the ARM side, because I'd already
given up hope of it ever being merged into mainline.

Your totally over the top and rediculous response just confirms that.

Further postings will now omit you and dri-devel.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/13 02:01, Dave Airlie wrote:

how about instead of writing:
However, at least I've taken the time to_think_  about what I'm doing
and realise that there_is_  scope here for the DRM core to improve,
rather than burying this stuff deep inside my driver like everyone else
has.  That's no reason to penalise patches from the good guys who think

you go with
I noticed this piece of functionality could be refactored, here is a
patch adding them to
the core, does anyone think its a good idea?


Dave,

at least on this point I do share Russell's impression. I've sent
bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored
- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much
appreciation for. But, back when TDA998x driver was published,
all my comments were basically answered with Oh, I know. Maybe
someday somebody will fix it.

I am not being paid for any of this, but have a strong intrinsic
motivation here. But I am loosing interest in sending fixes for
DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent
of how broken they are - others are discussed to death.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/13 11:42, Daniel Vetter wrote:

On Mon, Jul 01, 2013 at 10:52:03AM +0200, Sebastian Hesselbarth wrote:

at least on this point I do share Russell's impression. I've sent
bunch of patches improving TDA998x and DRM+DT:
- TDA998x irq handling - ignored
- TDA998x sync fix - ignored
- Fix drm I2C slave encoder probing

I am aware that this is not an easy job nor one you get much
appreciation for. But, back when TDA998x driver was published,
all my comments were basically answered with Oh, I know. Maybe
someday somebody will fix it.


I guess part of the problem here is that in the arm world we don't (yet)
have many full-blown drivers and not many people to fix up stuff or chime
in with good review. And sometimes that just means that someone puts down
his foot and says this is how we do it - at least for drm/i915 I
sometimes have to do that to unblock a massive bikeshed-fest.


I am not being paid for any of this, but have a strong intrinsic
motivation here. But I am loosing interest in sending fixes for
DRM stuff because my (personal) impression is the same Russell
has: Depending on who sends patches, they get merged independent
of how broken they are - others are discussed to death.


Hm, we run fairly extensive QA for drm/i915, and thus far the drm core
stuff hasn't really blown up badly for us. So can you please point at
examples where crap was merged and shouldn't have been?


Don't get me wrong, broken above didn't mean it doesn't work at all,
but with SoC graphic drivers arising, requirements shift from some
known implementations to you never know what will be combined with
e.g. a specific CRTC. So from that latter point-of-view, I consider
TDA998x for example as broken. It may work with tilcdc in some modes,
but as Darren Etheridge stated, it breaks as soon as you touch TDA998x
driver. At least for that, I confirmed the timings of Russell's driver
and the fixes posted with a scope and a bunch of modes and monitors/tv.

For the timing fix of TDA998x, Darren now is trying to also confirm
every single sync line of tilcdc and I really like to see his Tested-by
on the patch - just because Russell's driver and the CuBox are the only
testbeds I have on this.


Wrt to bikeshed to death I know that drm folks are a bit prone to that (me
included), but recently I haven't spotted a case where this happened. ARM
stuff excluded ofc since I don't follow that too closely. There's also
that Dave is sometimes a bit swamped, but pinging him on irc about lost
patches works well (at least for stuff I care about).


Hmm, I understand that it is _very_ time consuming work on your side.
But for me - with limited insight of DRM core - it is not a good starter
to make the API DT aware. The DRM API documentation _is_ limited wrt
intentions of the way it is done.

Of course, I share the idea of true, full-blown of_drm_something
helpers. But the DT patch, is not an improvement but a real fix as in
make DRM not break on some platforms. From that on, I can start
digging into DRM API and improve DT support here and there.

So for the three patches I mentioned above, they are all minor
improvements of the API. Minor, because I cannot ever sent _the_ single
perfect patch just because I don't know the API well enough, yet.

Just based on my personal experience: TDA998x driver got merged the way 
it is _although_ I addressed some concerns - the fixes are not merged

_although_ I provided experimental results. This is of course
disappointing for me, but I am sure it will work out soon and I will
get back to happily send improvements for the platforms I can test on.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

 Of course, I share the idea of true, full-blown of_drm_something
 helpers. But the DT patch, is not an improvement but a real fix as in
 make DRM not break on some platforms. From that on, I can start
 digging into DRM API and improve DT support here and there.

 So for the three patches I mentioned above, they are all minor
 improvements of the API. Minor, because I cannot ever sent _the_ single
 perfect patch just because I don't know the API well enough, yet.

 Just based on my personal experience: TDA998x driver got merged the way it
 is _although_ I addressed some concerns - the fixes are not merged
 _although_ I provided experimental results. This is of course
 disappointing for me, but I am sure it will work out soon and I will
 get back to happily send improvements for the platforms I can test on.

To be honest I've no idea what those tda998x patches could fix or
break, so they go on my no ideas list and I hope if they get reposted
someone will tell me what they do.

I could probably be more motivated towards poking other people to
review stuff, but I mostly hope in vain that the ARM people will cross
review things for other ARM chips, I had a bit of that happening for a
while at the start, but it seems to have died off. Now I'm mostly
relying on Rob to have some clue what I'm merging is sane.

My current priority for merging stuff is:
core patches that affect all platforms,
core patches that affect x86
drivers that I maintain by default
core patches that affect ARM
misc ARM drivers that fall through cracks.

Maybe I can persuade Rob to become a sub maintainer for all of the SoC
drivers but I suspect he'd try and hurt me in real life.

I'll take another look at the tda patches but I may still have no idea
what they are doing.

Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Rob Clark]

On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:
 On 07/01/13 02:01, Dave Airlie wrote:

 how about instead of writing:
 However, at least I've taken the time to_think_  about what I'm doing
 and realise that there_is_  scope here for the DRM core to improve,

 rather than burying this stuff deep inside my driver like everyone else
 has.  That's no reason to penalise patches from the good guys who think

 you go with
 I noticed this piece of functionality could be refactored, here is a
 patch adding them to
 the core, does anyone think its a good idea?


 Dave,

 at least on this point I do share Russell's impression. I've sent
 bunch of patches improving TDA998x and DRM+DT:
 - TDA998x irq handling - ignored
 - TDA998x sync fix - ignored

At least the sync fix, looks like I missed it (it probably is a good
idea to CC me if you want me to look at it).  Looks like there was
some follow-up discussion on both patches, unless I missed seeing a
newer version of those patches.

Sometimes if you think a patch has been ignored/forgotten, it doesn't
hurt to ping on mailing list or #dri-devel..  a lot of us are working
not just on kernel (the relatively small part in the whole linux
graphics stack), but also mesa and/or x11.  Some times things end up
several pages down in the mail folder.  It's not because we are all
sitting on a beach drinking margaritas, or because we don't like you.
It is just because we are busy and missed it.

Last few months I've been pretty buried in r/e + gallium driver for
new gpu, so I wasn't always checking dri-devel list every day.  At
least now I am in drm-driver mode again ;-)

 - Fix drm I2C slave encoder probing

 I am aware that this is not an easy job nor one you get much
 appreciation for. But, back when TDA998x driver was published,
 all my comments were basically answered with Oh, I know. Maybe
 someday somebody will fix it.

If you have a better idea about how to make the slave encoder probing
better (and/or more generic to support stuff other than i2c), please
send RFC patch.  (And if you already did this, please send updated
version, see previous point about sometimes missing patches.)


BR,
-R


 I am not being paid for any of this, but have a strong intrinsic
 motivation here. But I am loosing interest in sending fixes for
 DRM stuff because my (personal) impression is the same Russell
 has: Depending on who sends patches, they get merged independent
 of how broken they are - others are discussed to death.

 Sebastian


 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Rob Clark]

On Mon, Jul 1, 2013 at 5:26 PM, Dave Airlie airl...@gmail.com wrote:
 Of course, I share the idea of true, full-blown of_drm_something
 helpers. But the DT patch, is not an improvement but a real fix as in
 make DRM not break on some platforms. From that on, I can start
 digging into DRM API and improve DT support here and there.

 So for the three patches I mentioned above, they are all minor
 improvements of the API. Minor, because I cannot ever sent _the_ single
 perfect patch just because I don't know the API well enough, yet.

 Just based on my personal experience: TDA998x driver got merged the way it
 is _although_ I addressed some concerns - the fixes are not merged
 _although_ I provided experimental results. This is of course
 disappointing for me, but I am sure it will work out soon and I will
 get back to happily send improvements for the platforms I can test on.

 To be honest I've no idea what those tda998x patches could fix or
 break, so they go on my no ideas list and I hope if they get reposted
 someone will tell me what they do.

IIRC, a few of those patches were breaking things on tilcdc, which
someone needs to get to the bottom of..  and for that I'm relying a
bit on Darren as he is the one with both hw to test with tilcdc plus
an hdmi tester.  And I think understands the issue better than I do at
this point.

 I could probably be more motivated towards poking other people to
 review stuff, but I mostly hope in vain that the ARM people will cross
 review things for other ARM chips, I had a bit of that happening for a
 while at the start, but it seems to have died off. Now I'm mostly
 relying on Rob to have some clue what I'm merging is sane.

 My current priority for merging stuff is:
 core patches that affect all platforms,
 core patches that affect x86
 drivers that I maintain by default
 core patches that affect ARM
 misc ARM drivers that fall through cracks.

 Maybe I can persuade Rob to become a sub maintainer for all of the SoC
 drivers but I suspect he'd try and hurt me in real life.

/me runs :-P

well, anyways, I do at least try to review arm patches.  Things
sometimes do fall on the floor when I'm busy in other areas.

BR,
-R

 I'll take another look at the tda patches but I may still have no idea
 what they are doing.

 Dave.
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
 wrote:
> On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
>> On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
>> > +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
>> > +{
>> > +   struct drm_display_mode *adj = >crtc.mode;
>> > +   uint32_t val = 0;
>> > +
>> > +   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
>> > +   val |= CFG_CSC_YUV_CCIR709;
>> > +   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
>> > +   val |= CFG_CSC_RGB_STUDIO;
>> > +
>> > +   /*
>> > +* In auto mode, set the colorimetry, based upon the HDMI spec.
>> > +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
>> > +* ITU601.  It may be more appropriate to set this depending on
>> > +* the source - but what if the graphic frame is YUV and the
>> > +* video frame is RGB?
>> > +*/
>> > +   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
>> > +!(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
>> > +   (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
>> > +   if (dcrtc->csc_yuv_mode == CSC_AUTO)
>> > +   val |= CFG_CSC_YUV_CCIR709;
>> > +   }
>> > +
>> > +   /*
>> > +* We assume we're connected to a TV-like device, so the YUV->RGB
>> > +* conversion should produce a limited range.  We should set this
>> > +* depending on the connectors attached to this CRTC, and what
>> > +* kind of device they report being connected.
>> > +*/
>> > +   if (dcrtc->csc_rgb_mode == CSC_AUTO)
>> > +   val |= CFG_CSC_RGB_STUDIO;
>>
>> In the intel driver we check whether it's an cea mode with
>> drm_match_cea_mode, e.g. in intel_hdmi.c:
>>
>>   if (intel_hdmi->color_range_auto) {
>>   /* See CEA-861-E - 5.1 Default Encoding Parameters */
>>   if (intel_hdmi->has_hdmi_sink &&
>>   drm_match_cea_mode(adjusted_mode) > 1)
>>   intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
>>   else
>>   intel_hdmi->color_range = 0;
>>   }
>>
>> (The color_range gets then propageted to the right place since different
>> platforms have different ways to do that in intel hw).
>
> Unfortunately, this disagrees with the HDMI v1.3a specification:
>
> | Default Colorimetry
> |
> | ...
> |
> | 480p, 480i, 576p, 576i, 240p and 288p
> |
> | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
> | video formats described in CEA-861-D is based on SMPTE 170M.
> |
> | 1080i, 1080p and 720p
> |
> | The default colorimetry for high-definition video formats (1080i, 1080p and
> | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
>
> As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
> CEA when dealing with HDMI specifics.
>
> So, according to the HDMI specification, the default is that it's only
> 1080i, 1080p and 720p which are 709, the others are 601.  This does not
> correspond with "drm_match_cea_mode(adjusted_mode) > 1".

Hm, sounds like we need to improve stuff a bit, maybe add a common cea
helper to the drm core with a bool is_hdmi to decide whether it's
palin CEA or HDMI-CEA. Ville (who's written the current code and the
match cea mode helper) can you please take a look?

> Unfortunately, given DRM's structure, there's no way for the CRTC layer
> to really know what its driving, and this setting is at the CRTC layer,
> not the output layer.  It may be that you have the CRTC duplicated
> between two different display devices with different characteristics,
> which means you have to "choose" one - or just not bother.  I've chosen
> the latter because it's a simpler solution, and is in no way any more
> correct than any other solution.
>
> This is also why these are exported to userspace via properties, so if
> userspace knows better, it can deal with those issues.

Yeah, allowing userspace to overwrite the default selection makes lots
of sense, we expose similar properties.

>> > +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
>> > *dev,
>> > +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
>> > +{
>> > +   struct armada_framebuffer *dfb;
>> > +   uint8_t format, config;
>> > +   int ret;
>> > +
>> > +   switch (mode->pixel_format) {
>> > +#define FMT(drm, fmt, mod) \
>> > +   case DRM_FORMAT_##drm:  \
>> > +   format = CFG_##fmt; \
>> > +   config = mod;   \
>> > +   break
>> > +   FMT(RGB565, 565,CFG_SWAPRB);
>> > +   FMT(BGR565, 565,0);
>> > +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
>> > +   FMT(ABGR1555,   1555,   0);
>> > +   FMT(RGB888, 888PACK,CFG_SWAPRB);
>> > +   FMT(BGR888, 888PACK,0);
>> > +   FMT(XRGB,   X888,   CFG_SWAPRB);
>> > +   FMT(XBGR,   X888,   0);
>> > +   FMT(ARGB,   ,   CFG_SWAPRB);
>> > +   

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sun, Jun 30, 2013 at 3:41 PM, Russell King - ARM Linux
 wrote:
> On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
>> On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
>> > On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
>> > > + mutex_lock(>struct_mutex);
>> > > + free = drm_mm_search_free(>linear, size, align, 0);
>> > > + if (free)
>> > > + obj->linear = drm_mm_get_block(free, size, align);
>> > > + mutex_unlock(>struct_mutex);
>> >
>> > The two-stage search_free+get_block drm_mm interface is something I'd like
>> > to remove since it' complicates the interface for no gain. And the
>> > preallocation trick for atomic contexts is pretty racy as-is. Can you
>> > please convert this over to the insert_node interfaces which take a
>> > preallocate drm_mm_node?
>>
>> Yes and no.  This is only racy if it is callable from atomic contexts,
>> which the above isn't, because it takes a mutex, and the above is the
>> only place which allocations against that pool are done.  Mutexes can't
>> be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
>> interfaces.
>>
>> However, the insert_node interfaces appear not to solve any kind of race.
>> All they do is allow the kmalloc to be moved out of this region into
>> the user of this code sequence, while offering no additional locking or
>> safety.  So the mutex lock around a call to drm_mm_insert_node*() is
>> still required.
>>
>> As the kmalloc() happens beneath the mutex lock, another thread can't race
>> with an allocation in the above code anyway.  The *only* reason I'll change
>> this is that it is nicer to the system not to hold locks while allocating
>> memory.
>
> Err.  There's bugs here in the other drivers such as i915 which follow
> your suggestion.  The problem is this:
>
> Every time they allocate using drm_mm_insert_node(), they kmalloc a new
> struct drm_mm_node for this function to fill in and attach.
>
> When they've done with the allocation, they call drm_mm_put_block().
> This places the node onto the mm's unused_nodes list provided we don't
> already have MM_UNUSED_TARGET nodes on that list.

Yeah, for drivers that never use the atomic alloc stuff it's pointless
waste of memory.

> So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
> at which point we then start freeing any further nodes - and with no
> way to pull these off that list, they all just sit there not being
> used.
>
> The only function which takes nodes off that list is drm_mm_kmalloc(),
> which is declared statically, and so isn't available to drivers.
>
> Are drivers which use the drm_mm_insert_node() function expected to
> also use drm_mm_remove_node(), kfreeing the node after that call,
> rather than using drm_mm_put_block() ?

Yeah, with the insert_node interface deallocing the node is the
driver's responsibility.

> Either way, I think someone needs to fix the other drivers and Cc those
> patches to Stable...

Yeah, I think it's time that I move that item up my todo list a lot.
And also add some decent kerneldoc to the drm_mm stuff.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

Right, so, incrementally, the changes are (this may not entirely apply
to the version I've posted because I have several patches on top of
that.)

I've also added locking to the calls to drm_mm_dump_table() as otherwise
these walk those lists without holding any locks what so ever, which
could mean that a block is removed while we're walking the list.

 drivers/gpu/drm/armada/armada_debugfs.c |   17 ++--
 drivers/gpu/drm/armada/armada_fb.c  |   43 ++-
 drivers/gpu/drm/armada/armada_fbdev.c   |   20 ++
 drivers/gpu/drm/armada/armada_gem.c |   29 +++--
 4 files changed, 68 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c 
b/drivers/gpu/drm/armada/armada_debugfs.c
index f42f3ab..60e1038 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -23,16 +23,27 @@ static int armada_debugfs_gem_offs_show(struct seq_file *m, 
void *data)
struct drm_info_node *node = m->private;
struct drm_device *dev = node->minor->dev;
struct drm_gem_mm *mm = dev->mm_private;
+   int ret;
+
+   mutex_lock(>struct_mutex);
+   ret = drm_mm_dump_table(m, >offset_manager);
+   mutex_unlock(>struct_mutex);

-   return drm_mm_dump_table(m, >offset_manager);
+   return ret;
 }

 static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 {
struct drm_info_node *node = m->private;
-   struct armada_private *priv = node->minor->dev->dev_private;
+   struct drm_device *dev = node->minor->dev;
+   struct armada_private *priv = dev->dev_private;
+   int ret;

-   return drm_mm_dump_table(m, >linear);
+   mutex_lock(>struct_mutex);
+   ret = drm_mm_dump_table(m, >linear);
+   mutex_unlock(>struct_mutex);
+
+   return ret;
 }

 static int armada_debugfs_reg_show(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/armada/armada_fb.c 
b/drivers/gpu/drm/armada/armada_fb.c
index 28965e3..97fbd61 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -79,6 +79,9 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,

dfb->fmt = format;
dfb->mod = config;
+   dfb->obj = obj;
+
+   drm_helper_mode_fill_fb_struct(>fb, mode);

ret = drm_framebuffer_init(dev, >fb, _fb_funcs);
if (ret) {
@@ -86,14 +89,13 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,
return ERR_PTR(ret);
}

-   drm_helper_mode_fill_fb_struct(>fb, mode);
-
/*
-* Take a reference on our object - the caller is expected
-* to drop their reference to it.
+* Take a reference on our object as we're successful - the
+* caller already holds a reference, which keeps us safe for
+* the above call, but the caller will drop their reference
+* to it.  Hence we need to take our own reference.
 */
drm_gem_object_reference(>obj);
-   dfb->obj = obj;

return dfb;
 }
@@ -111,39 +113,44 @@ static struct drm_framebuffer *armada_fb_create(struct 
drm_device *dev,
mode->pitches[2]);

/* We can only handle a single plane at the moment */
-   if (drm_format_num_planes(mode->pixel_format) > 1)
-   return ERR_PTR(-EINVAL);
+   if (drm_format_num_planes(mode->pixel_format) > 1) {
+   ret = -EINVAL;
+   goto err;
+   }

obj = armada_gem_object_lookup(dev, dfile, mode->handles[0]);
if (!obj) {
-   DRM_ERROR("failed to lookup gem object\n");
-   return ERR_PTR(-ENOENT);
+   ret = -ENOENT;
+   goto err;
}

if (obj->obj.import_attach && !obj->sgt) {
ret = armada_gem_map_import(obj);
if (ret)
-   goto unref;
+   goto err_unref;
}

/* Framebuffer objects must have a valid device address for scanout */
if (obj->dev_addr == DMA_ERROR_CODE) {
ret = -EINVAL;
-   goto unref;
+   goto err_unref;
}

dfb = armada_framebuffer_create(dev, mode, obj);
-   ret = IS_ERR(dfb) ? PTR_ERR(dfb) : 0;
+   if (IS_ERR(dfb)) {
+   ret = PTR_ERR(dfb);
+   goto err;
+   }

-unref:
drm_gem_object_unreference_unlocked(>obj);

-   if (ret) {
-   DRM_ERROR("failed to initialize framebuffer: %d\n", ret);
-   return ERR_PTR(ret);
-   }
-
return >fb;
+
+ err_unref:
+   drm_gem_object_unreference_unlocked(>obj);
+ err:
+   DRM_ERROR("failed to initialize framebuffer: %d\n", ret);
+   return ERR_PTR(ret);
 }

 static void armada_output_poll_changed(struct drm_device *dev)
diff --git a/drivers/gpu/drm/armada/armada_fbdev.c 

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
> On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
> > On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> > > + mutex_lock(>struct_mutex);
> > > + free = drm_mm_search_free(>linear, size, align, 0);
> > > + if (free)
> > > + obj->linear = drm_mm_get_block(free, size, align);
> > > + mutex_unlock(>struct_mutex);
> > 
> > The two-stage search_free+get_block drm_mm interface is something I'd like
> > to remove since it' complicates the interface for no gain. And the
> > preallocation trick for atomic contexts is pretty racy as-is. Can you
> > please convert this over to the insert_node interfaces which take a
> > preallocate drm_mm_node?
> 
> Yes and no.  This is only racy if it is callable from atomic contexts,
> which the above isn't, because it takes a mutex, and the above is the
> only place which allocations against that pool are done.  Mutexes can't
> be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
> interfaces.
> 
> However, the insert_node interfaces appear not to solve any kind of race.
> All they do is allow the kmalloc to be moved out of this region into
> the user of this code sequence, while offering no additional locking or
> safety.  So the mutex lock around a call to drm_mm_insert_node*() is
> still required.
> 
> As the kmalloc() happens beneath the mutex lock, another thread can't race
> with an allocation in the above code anyway.  The *only* reason I'll change
> this is that it is nicer to the system not to hold locks while allocating
> memory.

Err.  There's bugs here in the other drivers such as i915 which follow
your suggestion.  The problem is this:

Every time they allocate using drm_mm_insert_node(), they kmalloc a new
struct drm_mm_node for this function to fill in and attach.

When they've done with the allocation, they call drm_mm_put_block().
This places the node onto the mm's unused_nodes list provided we don't
already have MM_UNUSED_TARGET nodes on that list.

So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
at which point we then start freeing any further nodes - and with no
way to pull these off that list, they all just sit there not being
used.

The only function which takes nodes off that list is drm_mm_kmalloc(),
which is declared statically, and so isn't available to drivers.

Are drivers which use the drm_mm_insert_node() function expected to
also use drm_mm_remove_node(), kfreeing the node after that call,
rather than using drm_mm_put_block() ?

Either way, I think someone needs to fix the other drivers and Cc those
patches to Stable...

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> This patch adds support for the pair of LCD controllers on the Marvell
> Armada 510 SoCs.  This driver supports:
> - multiple contiguous scanout buffers for video and graphics
> - shm backed cacheable buffer objects for X pixmaps for Vivante GPU
>   acceleration
> - dual lcd0 and lcd1 crt operation
> - video overlay on each LCD crt
> - page flipping of the main scanout buffers
> 
> Included in this commit is the core driver with no output support; output
> support is platform and encoder driver dependent.
> 
> Signed-off-by: Russell King 

Found one more ...

[snip]

> +int
> +armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
> +{
> + struct armada_private *priv = dev->dev_private;
> + size_t size = obj->obj.size;
> +
> + if (obj->page || obj->linear)
> + return 0;
> +
> + /*
> +  * If it is a small allocation (typically cursor, which will
> +  * be 32x64 or 64x32 ARGB pixels) try to get it from the system.
> +  * Framebuffers will never be this small (our minimum size for
> +  * framebuffers is larger than this anyway.)  Such objects are
> +  * only accessed by the CPU so we don't need any special handing
> +  * here.
> +  */
> + if (size <= 8192) {
> + unsigned int order = get_order(size);
> + struct page *p = alloc_pages(GFP_KERNEL, order);
> +
> + if (p) {
> + obj->addr = page_address(p);
> + obj->phys_addr = page_to_phys(p);
> + obj->page = p;
> +
> + memset(obj->addr, 0, PAGE_ALIGN(size));
> + }
> + }
> +
> + /*
> +  * We could grab something from CMA if it's enabled, but that
> +  * involves building in a problem:
> +  *
> +  * CMA's interface uses dma_alloc_coherent(), which provides us
> +  * with an CPU virtual address and a device address.
> +  *
> +  * The CPU virtual address may be either an address in the kernel
> +  * direct mapped region (for example, as it would be on x86) or
> +  * it may be remapped into another part of kernel memory space
> +  * (eg, as it would be on ARM.)  This means virt_to_phys() on the
> +  * returned virtual address is invalid depending on the architecture
> +  * implementation.
> +  *
> +  * The device address may also not be a physical address; it may
> +  * be that there is some kind of remapping between the device and
> +  * system RAM, which makes the use of the device address also
> +  * unsafe to re-use as a physical address.
> +  *
> +  * This makes DRM usage of dma_alloc_coherent() in a generic way
> +  * at best very questionable and unsafe.
> +  */
> +
> + /* Otherwise, grab it from our linear allocation */
> + if (!obj->page) {
> + struct drm_mm_node *free;
> + unsigned align = min_t(unsigned, size, SZ_2M);
> + void __iomem *ptr;
> +
> + mutex_lock(>struct_mutex);
> + free = drm_mm_search_free(>linear, size, align, 0);
> + if (free)
> + obj->linear = drm_mm_get_block(free, size, align);
> + mutex_unlock(>struct_mutex);

The two-stage search_free+get_block drm_mm interface is something I'd like
to remove since it' complicates the interface for no gain. And the
preallocation trick for atomic contexts is pretty racy as-is. Can you
please convert this over to the insert_node interfaces which take a
preallocate drm_mm_node?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
> On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> > +   mutex_lock(>struct_mutex);
> > +   free = drm_mm_search_free(>linear, size, align, 0);
> > +   if (free)
> > +   obj->linear = drm_mm_get_block(free, size, align);
> > +   mutex_unlock(>struct_mutex);
> 
> The two-stage search_free+get_block drm_mm interface is something I'd like
> to remove since it' complicates the interface for no gain. And the
> preallocation trick for atomic contexts is pretty racy as-is. Can you
> please convert this over to the insert_node interfaces which take a
> preallocate drm_mm_node?

Yes and no.  This is only racy if it is callable from atomic contexts,
which the above isn't, because it takes a mutex, and the above is the
only place which allocations against that pool are done.  Mutexes can't
be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
interfaces.

However, the insert_node interfaces appear not to solve any kind of race.
All they do is allow the kmalloc to be moved out of this region into
the user of this code sequence, while offering no additional locking or
safety.  So the mutex lock around a call to drm_mm_insert_node*() is
still required.

As the kmalloc() happens beneath the mutex lock, another thread can't race
with an allocation in the above code anyway.  The *only* reason I'll change
this is that it is nicer to the system not to hold locks while allocating
memory.

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> This patch adds support for the pair of LCD controllers on the Marvell
> Armada 510 SoCs.  This driver supports:
> - multiple contiguous scanout buffers for video and graphics
> - shm backed cacheable buffer objects for X pixmaps for Vivante GPU
>   acceleration
> - dual lcd0 and lcd1 crt operation
> - video overlay on each LCD crt
> - page flipping of the main scanout buffers
> 
> Included in this commit is the core driver with no output support; output
> support is platform and encoder driver dependent.
> 
> Signed-off-by: Russell King 

Upfront disclaimer: I have no clue about ARM/DT integration issues, so I
don't have any opinion/comments on those.

I've spotted a few other things though, see below. With those addressed
this patch is

Reviewed-by: Daniel Vetter 

Cheers, Daniel
> ---

[snip]

> +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
> +{
> + struct drm_display_mode *adj = >crtc.mode;
> + uint32_t val = 0;
> +
> + if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
> + val |= CFG_CSC_YUV_CCIR709;
> + if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
> + val |= CFG_CSC_RGB_STUDIO;
> +
> + /*
> +  * In auto mode, set the colorimetry, based upon the HDMI spec.
> +  * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
> +  * ITU601.  It may be more appropriate to set this depending on
> +  * the source - but what if the graphic frame is YUV and the
> +  * video frame is RGB?
> +  */
> + if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
> +  !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
> + (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
> + if (dcrtc->csc_yuv_mode == CSC_AUTO)
> + val |= CFG_CSC_YUV_CCIR709;
> + }
> +
> + /*
> +  * We assume we're connected to a TV-like device, so the YUV->RGB
> +  * conversion should produce a limited range.  We should set this
> +  * depending on the connectors attached to this CRTC, and what
> +  * kind of device they report being connected.
> +  */
> + if (dcrtc->csc_rgb_mode == CSC_AUTO)
> + val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with
drm_match_cea_mode, e.g. in intel_hdmi.c:

if (intel_hdmi->color_range_auto) {
/* See CEA-861-E - 5.1 Default Encoding Parameters */
if (intel_hdmi->has_hdmi_sink &&
drm_match_cea_mode(adjusted_mode) > 1)
intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
else
intel_hdmi->color_range = 0;
}

(The color_range gets then propageted to the right place since different
platforms have different ways to do that in intel hw).

> +
> + return val;
> +}
> +

[snip]

> +struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
> + struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
> +{
> + struct armada_framebuffer *dfb;
> + uint8_t format, config;
> + int ret;
> +
> + switch (mode->pixel_format) {
> +#define FMT(drm, fmt, mod)   \
> + case DRM_FORMAT_##drm:  \
> + format = CFG_##fmt; \
> + config = mod;   \
> + break
> + FMT(RGB565, 565,CFG_SWAPRB);
> + FMT(BGR565, 565,0);
> + FMT(ARGB1555,   1555,   CFG_SWAPRB);
> + FMT(ABGR1555,   1555,   0);
> + FMT(RGB888, 888PACK,CFG_SWAPRB);
> + FMT(BGR888, 888PACK,0);
> + FMT(XRGB,   X888,   CFG_SWAPRB);
> + FMT(XBGR,   X888,   0);
> + FMT(ARGB,   ,   CFG_SWAPRB);
> + FMT(ABGR,   ,   0);
> + FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
> + FMT(UYVY,   422PACK,CFG_YUV2RGB);
> + FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
> + FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
> + FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
> + FMT(YVU422, 422,CFG_YUV2RGB);
> + FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
> + FMT(YVU420, 420,CFG_YUV2RGB);
> + FMT(C8, PSEUDO8,0);
> +#undef FMT
> + default:
> + return ERR_PTR(-EINVAL);
> + }
> +
> + dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
> + if (!dfb) {
> + DRM_ERROR("failed to allocate Armada fb object\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + dfb->fmt = format;
> + dfb->mod = config;
> +
> + ret = drm_framebuffer_init(dev, >fb, _fb_funcs);
> + if (ret) {
> + kfree(dfb);
> + return ERR_PTR(ret);
> + }

drm_framebuffer_init publishes the fb object to the 

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
> On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> > +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
> > +{
> > +   struct drm_display_mode *adj = >crtc.mode;
> > +   uint32_t val = 0;
> > +
> > +   if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
> > +   val |= CFG_CSC_YUV_CCIR709;
> > +   if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
> > +   val |= CFG_CSC_RGB_STUDIO;
> > +
> > +   /*
> > +* In auto mode, set the colorimetry, based upon the HDMI spec.
> > +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
> > +* ITU601.  It may be more appropriate to set this depending on
> > +* the source - but what if the graphic frame is YUV and the
> > +* video frame is RGB?
> > +*/
> > +   if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
> > +!(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
> > +   (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
> > +   if (dcrtc->csc_yuv_mode == CSC_AUTO)
> > +   val |= CFG_CSC_YUV_CCIR709;
> > +   }
> > +
> > +   /*
> > +* We assume we're connected to a TV-like device, so the YUV->RGB
> > +* conversion should produce a limited range.  We should set this
> > +* depending on the connectors attached to this CRTC, and what
> > +* kind of device they report being connected.
> > +*/
> > +   if (dcrtc->csc_rgb_mode == CSC_AUTO)
> > +   val |= CFG_CSC_RGB_STUDIO;
> 
> In the intel driver we check whether it's an cea mode with
> drm_match_cea_mode, e.g. in intel_hdmi.c:
> 
>   if (intel_hdmi->color_range_auto) {
>   /* See CEA-861-E - 5.1 Default Encoding Parameters */
>   if (intel_hdmi->has_hdmi_sink &&
>   drm_match_cea_mode(adjusted_mode) > 1)
>   intel_hdmi->color_range = HDMI_COLOR_RANGE_16_235;
>   else
>   intel_hdmi->color_range = 0;
>   }
> 
> (The color_range gets then propageted to the right place since different
> platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

| Default Colorimetry
| 
| ...
| 
| 480p, 480i, 576p, 576i, 240p and 288p
| 
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
| 
| 1080i, 1080p and 720p
| 
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only
1080i, 1080p and 720p which are 709, the others are 601.  This does not
correspond with "drm_match_cea_mode(adjusted_mode) > 1".

Unfortunately, given DRM's structure, there's no way for the CRTC layer
to really know what its driving, and this setting is at the CRTC layer,
not the output layer.  It may be that you have the CRTC duplicated
between two different display devices with different characteristics,
which means you have to "choose" one - or just not bother.  I've chosen
the latter because it's a simpler solution, and is in no way any more
correct than any other solution.

This is also why these are exported to userspace via properties, so if
userspace knows better, it can deal with those issues.

> > +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
> > *dev,
> > +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
> > +{
> > +   struct armada_framebuffer *dfb;
> > +   uint8_t format, config;
> > +   int ret;
> > +
> > +   switch (mode->pixel_format) {
> > +#define FMT(drm, fmt, mod) \
> > +   case DRM_FORMAT_##drm:  \
> > +   format = CFG_##fmt; \
> > +   config = mod;   \
> > +   break
> > +   FMT(RGB565, 565,CFG_SWAPRB);
> > +   FMT(BGR565, 565,0);
> > +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
> > +   FMT(ABGR1555,   1555,   0);
> > +   FMT(RGB888, 888PACK,CFG_SWAPRB);
> > +   FMT(BGR888, 888PACK,0);
> > +   FMT(XRGB,   X888,   CFG_SWAPRB);
> > +   FMT(XBGR,   X888,   0);
> > +   FMT(ARGB,   ,   CFG_SWAPRB);
> > +   FMT(ABGR,   ,   0);
> > +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
> > +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
> > +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
> > +   FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
> > +   FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
> > +   FMT(YVU422, 422,CFG_YUV2RGB);
> > +   FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
> > +   FMT(YVU420, 420,CFG_YUV2RGB);
> > +   FMT(C8, 

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King]

This patch adds support for the pair of LCD controllers on the Marvell
Armada 510 SoCs.  This driver supports:
- multiple contiguous scanout buffers for video and graphics
- shm backed cacheable buffer objects for X pixmaps for Vivante GPU
  acceleration
- dual lcd0 and lcd1 crt operation
- video overlay on each LCD crt
- page flipping of the main scanout buffers

Included in this commit is the core driver with no output support; output
support is platform and encoder driver dependent.

Signed-off-by: Russell King 
---
 drivers/gpu/drm/Kconfig |2 +
 drivers/gpu/drm/Makefile|1 +
 drivers/gpu/drm/armada/Kconfig  |   15 +
 drivers/gpu/drm/armada/Makefile |7 +
 drivers/gpu/drm/armada/armada_510.c |   86 +++
 drivers/gpu/drm/armada/armada_crtc.c|  861 +++
 drivers/gpu/drm/armada/armada_crtc.h|   74 +++
 drivers/gpu/drm/armada/armada_debugfs.c |  187 +++
 drivers/gpu/drm/armada/armada_drm.h |  112 
 drivers/gpu/drm/armada/armada_drv.c |  381 ++
 drivers/gpu/drm/armada/armada_fb.c  |  155 ++
 drivers/gpu/drm/armada/armada_fb.h  |   24 +
 drivers/gpu/drm/armada/armada_fbdev.c   |  206 
 drivers/gpu/drm/armada/armada_gem.c |  541 +++
 drivers/gpu/drm/armada/armada_gem.h |   48 ++
 drivers/gpu/drm/armada/armada_hw.h  |  316 +++
 drivers/gpu/drm/armada/armada_ioctl.h   |   45 ++
 drivers/gpu/drm/armada/armada_ioctlP.h  |   18 +
 drivers/gpu/drm/armada/armada_output.c  |  159 ++
 drivers/gpu/drm/armada/armada_output.h  |   39 ++
 drivers/gpu/drm/armada/armada_overlay.c |  478 +
 drivers/gpu/drm/armada/armada_slave.c   |  139 +
 drivers/gpu/drm/armada/armada_slave.h   |   26 +
 drivers/gpu/drm/armada/drm_helper.h |   31 ++
 24 files changed, 3951 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/armada/Kconfig
 create mode 100644 drivers/gpu/drm/armada/Makefile
 create mode 100644 drivers/gpu/drm/armada/armada_510.c
 create mode 100644 drivers/gpu/drm/armada/armada_crtc.c
 create mode 100644 drivers/gpu/drm/armada/armada_crtc.h
 create mode 100644 drivers/gpu/drm/armada/armada_debugfs.c
 create mode 100644 drivers/gpu/drm/armada/armada_drm.h
 create mode 100644 drivers/gpu/drm/armada/armada_drv.c
 create mode 100644 drivers/gpu/drm/armada/armada_fb.c
 create mode 100644 drivers/gpu/drm/armada/armada_fb.h
 create mode 100644 drivers/gpu/drm/armada/armada_fbdev.c
 create mode 100644 drivers/gpu/drm/armada/armada_gem.c
 create mode 100644 drivers/gpu/drm/armada/armada_gem.h
 create mode 100644 drivers/gpu/drm/armada/armada_hw.h
 create mode 100644 drivers/gpu/drm/armada/armada_ioctl.h
 create mode 100644 drivers/gpu/drm/armada/armada_ioctlP.h
 create mode 100644 drivers/gpu/drm/armada/armada_output.c
 create mode 100644 drivers/gpu/drm/armada/armada_output.h
 create mode 100644 drivers/gpu/drm/armada/armada_overlay.c
 create mode 100644 drivers/gpu/drm/armada/armada_slave.c
 create mode 100644 drivers/gpu/drm/armada/armada_slave.h
 create mode 100644 drivers/gpu/drm/armada/drm_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1e82882..ae8a57f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -213,6 +213,8 @@ source "drivers/gpu/drm/mgag200/Kconfig"

 source "drivers/gpu/drm/cirrus/Kconfig"

+source "drivers/gpu/drm/armada/Kconfig"
+
 source "drivers/gpu/drm/shmobile/Kconfig"

 source "drivers/gpu/drm/tegra/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 0d59b24..b458168 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_DRM_EXYNOS) +=exynos/
 obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
+obj-$(CONFIG_DRM_ARMADA) += armada/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-$(CONFIG_DRM_TEGRA) += tegra/
 obj-$(CONFIG_DRM_OMAP) += omapdrm/
diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig
new file mode 100644
index 000..c7a0a94
--- /dev/null
+++ b/drivers/gpu/drm/armada/Kconfig
@@ -0,0 +1,15 @@
+config DRM_ARMADA
+   tristate "DRM support for Marvell Armada SoCs"
+   depends on DRM && HAVE_CLK
+   select FB_CFB_FILLRECT
+   select FB_CFB_COPYAREA
+   select FB_CFB_IMAGEBLIT
+   select DRM_KMS_HELPER
+   help
+ Support the "LCD" controllers found on the Marvell Armada 510
+ devices.  There are two controllers on the device, each controller
+ supports graphics and video overlays.
+
+ This driver provides no built-in acceleration; acceleration is
+ performed by other IP found on the SoC.  This driver provides
+ kernel mode setting and buffer management to userspace.
diff --git a/drivers/gpu/drm/armada/Makefile b/drivers/gpu/drm/armada/Makefile
new file mode 100644
index 000..d6f43e0
--- 

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
 This patch adds support for the pair of LCD controllers on the Marvell
 Armada 510 SoCs.  This driver supports:
 - multiple contiguous scanout buffers for video and graphics
 - shm backed cacheable buffer objects for X pixmaps for Vivante GPU
   acceleration
 - dual lcd0 and lcd1 crt operation
 - video overlay on each LCD crt
 - page flipping of the main scanout buffers
 
 Included in this commit is the core driver with no output support; output
 support is platform and encoder driver dependent.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk

Upfront disclaimer: I have no clue about ARM/DT integration issues, so I
don't have any opinion/comments on those.

I've spotted a few other things though, see below. With those addressed
this patch is

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

Cheers, Daniel
 ---

[snip]

 +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 +{
 + struct drm_display_mode *adj = dcrtc-crtc.mode;
 + uint32_t val = 0;
 +
 + if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
 + val |= CFG_CSC_YUV_CCIR709;
 + if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
 + val |= CFG_CSC_RGB_STUDIO;
 +
 + /*
 +  * In auto mode, set the colorimetry, based upon the HDMI spec.
 +  * 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
 +  * ITU601.  It may be more appropriate to set this depending on
 +  * the source - but what if the graphic frame is YUV and the
 +  * video frame is RGB?
 +  */
 + if ((adj-hdisplay == 1280  adj-vdisplay == 720 
 +  !(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
 + (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
 + if (dcrtc-csc_yuv_mode == CSC_AUTO)
 + val |= CFG_CSC_YUV_CCIR709;
 + }
 +
 + /*
 +  * We assume we're connected to a TV-like device, so the YUV-RGB
 +  * conversion should produce a limited range.  We should set this
 +  * depending on the connectors attached to this CRTC, and what
 +  * kind of device they report being connected.
 +  */
 + if (dcrtc-csc_rgb_mode == CSC_AUTO)
 + val |= CFG_CSC_RGB_STUDIO;

In the intel driver we check whether it's an cea mode with
drm_match_cea_mode, e.g. in intel_hdmi.c:

if (intel_hdmi-color_range_auto) {
/* See CEA-861-E - 5.1 Default Encoding Parameters */
if (intel_hdmi-has_hdmi_sink 
drm_match_cea_mode(adjusted_mode)  1)
intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
else
intel_hdmi-color_range = 0;
}

(The color_range gets then propageted to the right place since different
platforms have different ways to do that in intel hw).

 +
 + return val;
 +}
 +

[snip]

 +struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
 + struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
 +{
 + struct armada_framebuffer *dfb;
 + uint8_t format, config;
 + int ret;
 +
 + switch (mode-pixel_format) {
 +#define FMT(drm, fmt, mod)   \
 + case DRM_FORMAT_##drm:  \
 + format = CFG_##fmt; \
 + config = mod;   \
 + break
 + FMT(RGB565, 565,CFG_SWAPRB);
 + FMT(BGR565, 565,0);
 + FMT(ARGB1555,   1555,   CFG_SWAPRB);
 + FMT(ABGR1555,   1555,   0);
 + FMT(RGB888, 888PACK,CFG_SWAPRB);
 + FMT(BGR888, 888PACK,0);
 + FMT(XRGB,   X888,   CFG_SWAPRB);
 + FMT(XBGR,   X888,   0);
 + FMT(ARGB,   ,   CFG_SWAPRB);
 + FMT(ABGR,   ,   0);
 + FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
 + FMT(UYVY,   422PACK,CFG_YUV2RGB);
 + FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
 + FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
 + FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
 + FMT(YVU422, 422,CFG_YUV2RGB);
 + FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
 + FMT(YVU420, 420,CFG_YUV2RGB);
 + FMT(C8, PSEUDO8,0);
 +#undef FMT
 + default:
 + return ERR_PTR(-EINVAL);
 + }
 +
 + dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
 + if (!dfb) {
 + DRM_ERROR(failed to allocate Armada fb object\n);
 + return ERR_PTR(-ENOMEM);
 + }
 +
 + dfb-fmt = format;
 + dfb-mod = config;
 +
 + ret = drm_framebuffer_init(dev, dfb-fb, armada_fb_funcs);
 + if (ret) {
 + kfree(dfb);
 + return ERR_PTR(ret);
 + }

drm_framebuffer_init publishes the fb object to the world, hence
initialization of all invariant state _must_ be done beforehand. This is a
new 

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
 This patch adds support for the pair of LCD controllers on the Marvell
 Armada 510 SoCs.  This driver supports:
 - multiple contiguous scanout buffers for video and graphics
 - shm backed cacheable buffer objects for X pixmaps for Vivante GPU
   acceleration
 - dual lcd0 and lcd1 crt operation
 - video overlay on each LCD crt
 - page flipping of the main scanout buffers
 
 Included in this commit is the core driver with no output support; output
 support is platform and encoder driver dependent.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk

Found one more ...

[snip]

 +int
 +armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 +{
 + struct armada_private *priv = dev-dev_private;
 + size_t size = obj-obj.size;
 +
 + if (obj-page || obj-linear)
 + return 0;
 +
 + /*
 +  * If it is a small allocation (typically cursor, which will
 +  * be 32x64 or 64x32 ARGB pixels) try to get it from the system.
 +  * Framebuffers will never be this small (our minimum size for
 +  * framebuffers is larger than this anyway.)  Such objects are
 +  * only accessed by the CPU so we don't need any special handing
 +  * here.
 +  */
 + if (size = 8192) {
 + unsigned int order = get_order(size);
 + struct page *p = alloc_pages(GFP_KERNEL, order);
 +
 + if (p) {
 + obj-addr = page_address(p);
 + obj-phys_addr = page_to_phys(p);
 + obj-page = p;
 +
 + memset(obj-addr, 0, PAGE_ALIGN(size));
 + }
 + }
 +
 + /*
 +  * We could grab something from CMA if it's enabled, but that
 +  * involves building in a problem:
 +  *
 +  * CMA's interface uses dma_alloc_coherent(), which provides us
 +  * with an CPU virtual address and a device address.
 +  *
 +  * The CPU virtual address may be either an address in the kernel
 +  * direct mapped region (for example, as it would be on x86) or
 +  * it may be remapped into another part of kernel memory space
 +  * (eg, as it would be on ARM.)  This means virt_to_phys() on the
 +  * returned virtual address is invalid depending on the architecture
 +  * implementation.
 +  *
 +  * The device address may also not be a physical address; it may
 +  * be that there is some kind of remapping between the device and
 +  * system RAM, which makes the use of the device address also
 +  * unsafe to re-use as a physical address.
 +  *
 +  * This makes DRM usage of dma_alloc_coherent() in a generic way
 +  * at best very questionable and unsafe.
 +  */
 +
 + /* Otherwise, grab it from our linear allocation */
 + if (!obj-page) {
 + struct drm_mm_node *free;
 + unsigned align = min_t(unsigned, size, SZ_2M);
 + void __iomem *ptr;
 +
 + mutex_lock(dev-struct_mutex);
 + free = drm_mm_search_free(priv-linear, size, align, 0);
 + if (free)
 + obj-linear = drm_mm_get_block(free, size, align);
 + mutex_unlock(dev-struct_mutex);

The two-stage search_free+get_block drm_mm interface is something I'd like
to remove since it' complicates the interface for no gain. And the
preallocation trick for atomic contexts is pretty racy as-is. Can you
please convert this over to the insert_node interfaces which take a
preallocate drm_mm_node?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
  +{
  +   struct drm_display_mode *adj = dcrtc-crtc.mode;
  +   uint32_t val = 0;
  +
  +   if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
  +   val |= CFG_CSC_RGB_STUDIO;
  +
  +   /*
  +* In auto mode, set the colorimetry, based upon the HDMI spec.
  +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
  +* ITU601.  It may be more appropriate to set this depending on
  +* the source - but what if the graphic frame is YUV and the
  +* video frame is RGB?
  +*/
  +   if ((adj-hdisplay == 1280  adj-vdisplay == 720 
  +!(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
  +   (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
  +   if (dcrtc-csc_yuv_mode == CSC_AUTO)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   }
  +
  +   /*
  +* We assume we're connected to a TV-like device, so the YUV-RGB
  +* conversion should produce a limited range.  We should set this
  +* depending on the connectors attached to this CRTC, and what
  +* kind of device they report being connected.
  +*/
  +   if (dcrtc-csc_rgb_mode == CSC_AUTO)
  +   val |= CFG_CSC_RGB_STUDIO;
 
 In the intel driver we check whether it's an cea mode with
 drm_match_cea_mode, e.g. in intel_hdmi.c:
 
   if (intel_hdmi-color_range_auto) {
   /* See CEA-861-E - 5.1 Default Encoding Parameters */
   if (intel_hdmi-has_hdmi_sink 
   drm_match_cea_mode(adjusted_mode)  1)
   intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
   else
   intel_hdmi-color_range = 0;
   }
 
 (The color_range gets then propageted to the right place since different
 platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

| Default Colorimetry
| 
| ...
| 
| 480p, 480i, 576p, 576i, 240p and 288p
| 
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
| 
| 1080i, 1080p and 720p
| 
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only
1080i, 1080p and 720p which are 709, the others are 601.  This does not
correspond with drm_match_cea_mode(adjusted_mode)  1.

Unfortunately, given DRM's structure, there's no way for the CRTC layer
to really know what its driving, and this setting is at the CRTC layer,
not the output layer.  It may be that you have the CRTC duplicated
between two different display devices with different characteristics,
which means you have to choose one - or just not bother.  I've chosen
the latter because it's a simpler solution, and is in no way any more
correct than any other solution.

This is also why these are exported to userspace via properties, so if
userspace knows better, it can deal with those issues.

  +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
  *dev,
  +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
  +{
  +   struct armada_framebuffer *dfb;
  +   uint8_t format, config;
  +   int ret;
  +
  +   switch (mode-pixel_format) {
  +#define FMT(drm, fmt, mod) \
  +   case DRM_FORMAT_##drm:  \
  +   format = CFG_##fmt; \
  +   config = mod;   \
  +   break
  +   FMT(RGB565, 565,CFG_SWAPRB);
  +   FMT(BGR565, 565,0);
  +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
  +   FMT(ABGR1555,   1555,   0);
  +   FMT(RGB888, 888PACK,CFG_SWAPRB);
  +   FMT(BGR888, 888PACK,0);
  +   FMT(XRGB,   X888,   CFG_SWAPRB);
  +   FMT(XBGR,   X888,   0);
  +   FMT(ARGB,   ,   CFG_SWAPRB);
  +   FMT(ABGR,   ,   0);
  +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
  +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
  +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
  +   FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU422, 422,CFG_YUV2RGB);
  +   FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU420, 420,CFG_YUV2RGB);
  +   FMT(C8, PSEUDO8,0);
  +#undef FMT
  +   default:
  +   return ERR_PTR(-EINVAL);
  +   }
  +
  +   dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
  +   if (!dfb) {
  + 

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +   mutex_lock(dev-struct_mutex);
  +   free = drm_mm_search_free(priv-linear, size, align, 0);
  +   if (free)
  +   obj-linear = drm_mm_get_block(free, size, align);
  +   mutex_unlock(dev-struct_mutex);
 
 The two-stage search_free+get_block drm_mm interface is something I'd like
 to remove since it' complicates the interface for no gain. And the
 preallocation trick for atomic contexts is pretty racy as-is. Can you
 please convert this over to the insert_node interfaces which take a
 preallocate drm_mm_node?

Yes and no.  This is only racy if it is callable from atomic contexts,
which the above isn't, because it takes a mutex, and the above is the
only place which allocations against that pool are done.  Mutexes can't
be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
interfaces.

However, the insert_node interfaces appear not to solve any kind of race.
All they do is allow the kmalloc to be moved out of this region into
the user of this code sequence, while offering no additional locking or
safety.  So the mutex lock around a call to drm_mm_insert_node*() is
still required.

As the kmalloc() happens beneath the mutex lock, another thread can't race
with an allocation in the above code anyway.  The *only* reason I'll change
this is that it is nicer to the system not to hold locks while allocating
memory.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
 On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
  On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
   + mutex_lock(dev-struct_mutex);
   + free = drm_mm_search_free(priv-linear, size, align, 0);
   + if (free)
   + obj-linear = drm_mm_get_block(free, size, align);
   + mutex_unlock(dev-struct_mutex);
  
  The two-stage search_free+get_block drm_mm interface is something I'd like
  to remove since it' complicates the interface for no gain. And the
  preallocation trick for atomic contexts is pretty racy as-is. Can you
  please convert this over to the insert_node interfaces which take a
  preallocate drm_mm_node?
 
 Yes and no.  This is only racy if it is callable from atomic contexts,
 which the above isn't, because it takes a mutex, and the above is the
 only place which allocations against that pool are done.  Mutexes can't
 be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
 interfaces.
 
 However, the insert_node interfaces appear not to solve any kind of race.
 All they do is allow the kmalloc to be moved out of this region into
 the user of this code sequence, while offering no additional locking or
 safety.  So the mutex lock around a call to drm_mm_insert_node*() is
 still required.
 
 As the kmalloc() happens beneath the mutex lock, another thread can't race
 with an allocation in the above code anyway.  The *only* reason I'll change
 this is that it is nicer to the system not to hold locks while allocating
 memory.

Err.  There's bugs here in the other drivers such as i915 which follow
your suggestion.  The problem is this:

Every time they allocate using drm_mm_insert_node(), they kmalloc a new
struct drm_mm_node for this function to fill in and attach.

When they've done with the allocation, they call drm_mm_put_block().
This places the node onto the mm's unused_nodes list provided we don't
already have MM_UNUSED_TARGET nodes on that list.

So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
at which point we then start freeing any further nodes - and with no
way to pull these off that list, they all just sit there not being
used.

The only function which takes nodes off that list is drm_mm_kmalloc(),
which is declared statically, and so isn't available to drivers.

Are drivers which use the drm_mm_insert_node() function expected to
also use drm_mm_remove_node(), kfreeing the node after that call,
rather than using drm_mm_put_block() ?

Either way, I think someone needs to fix the other drivers and Cc those
patches to Stable...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

Right, so, incrementally, the changes are (this may not entirely apply
to the version I've posted because I have several patches on top of
that.)

I've also added locking to the calls to drm_mm_dump_table() as otherwise
these walk those lists without holding any locks what so ever, which
could mean that a block is removed while we're walking the list.

 drivers/gpu/drm/armada/armada_debugfs.c |   17 ++--
 drivers/gpu/drm/armada/armada_fb.c  |   43 ++-
 drivers/gpu/drm/armada/armada_fbdev.c   |   20 ++
 drivers/gpu/drm/armada/armada_gem.c |   29 +++--
 4 files changed, 68 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c 
b/drivers/gpu/drm/armada/armada_debugfs.c
index f42f3ab..60e1038 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -23,16 +23,27 @@ static int armada_debugfs_gem_offs_show(struct seq_file *m, 
void *data)
struct drm_info_node *node = m-private;
struct drm_device *dev = node-minor-dev;
struct drm_gem_mm *mm = dev-mm_private;
+   int ret;
+
+   mutex_lock(dev-struct_mutex);
+   ret = drm_mm_dump_table(m, mm-offset_manager);
+   mutex_unlock(dev-struct_mutex);
 
-   return drm_mm_dump_table(m, mm-offset_manager);
+   return ret;
 }
 
 static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 {
struct drm_info_node *node = m-private;
-   struct armada_private *priv = node-minor-dev-dev_private;
+   struct drm_device *dev = node-minor-dev;
+   struct armada_private *priv = dev-dev_private;
+   int ret;
 
-   return drm_mm_dump_table(m, priv-linear);
+   mutex_lock(dev-struct_mutex);
+   ret = drm_mm_dump_table(m, priv-linear);
+   mutex_unlock(dev-struct_mutex);
+
+   return ret;
 }
 
 static int armada_debugfs_reg_show(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/armada/armada_fb.c 
b/drivers/gpu/drm/armada/armada_fb.c
index 28965e3..97fbd61 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -79,6 +79,9 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,
 
dfb-fmt = format;
dfb-mod = config;
+   dfb-obj = obj;
+
+   drm_helper_mode_fill_fb_struct(dfb-fb, mode);
 
ret = drm_framebuffer_init(dev, dfb-fb, armada_fb_funcs);
if (ret) {
@@ -86,14 +89,13 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,
return ERR_PTR(ret);
}
 
-   drm_helper_mode_fill_fb_struct(dfb-fb, mode);
-
/*
-* Take a reference on our object - the caller is expected
-* to drop their reference to it.
+* Take a reference on our object as we're successful - the
+* caller already holds a reference, which keeps us safe for
+* the above call, but the caller will drop their reference
+* to it.  Hence we need to take our own reference.
 */
drm_gem_object_reference(obj-obj);
-   dfb-obj = obj;
 
return dfb;
 }
@@ -111,39 +113,44 @@ static struct drm_framebuffer *armada_fb_create(struct 
drm_device *dev,
mode-pitches[2]);
 
/* We can only handle a single plane at the moment */
-   if (drm_format_num_planes(mode-pixel_format)  1)
-   return ERR_PTR(-EINVAL);
+   if (drm_format_num_planes(mode-pixel_format)  1) {
+   ret = -EINVAL;
+   goto err;
+   }
 
obj = armada_gem_object_lookup(dev, dfile, mode-handles[0]);
if (!obj) {
-   DRM_ERROR(failed to lookup gem object\n);
-   return ERR_PTR(-ENOENT);
+   ret = -ENOENT;
+   goto err;
}
 
if (obj-obj.import_attach  !obj-sgt) {
ret = armada_gem_map_import(obj);
if (ret)
-   goto unref;
+   goto err_unref;
}
 
/* Framebuffer objects must have a valid device address for scanout */
if (obj-dev_addr == DMA_ERROR_CODE) {
ret = -EINVAL;
-   goto unref;
+   goto err_unref;
}
 
dfb = armada_framebuffer_create(dev, mode, obj);
-   ret = IS_ERR(dfb) ? PTR_ERR(dfb) : 0;
+   if (IS_ERR(dfb)) {
+   ret = PTR_ERR(dfb);
+   goto err;
+   }
 
-unref:
drm_gem_object_unreference_unlocked(obj-obj);
 
-   if (ret) {
-   DRM_ERROR(failed to initialize framebuffer: %d\n, ret);
-   return ERR_PTR(ret);
-   }
-
return dfb-fb;
+
+ err_unref:
+   drm_gem_object_unreference_unlocked(obj-obj);
+ err:
+   DRM_ERROR(failed to initialize framebuffer: %d\n, ret);
+   return ERR_PTR(ret);
 }
 
 static void armada_output_poll_changed(struct drm_device *dev)
diff --git a/drivers/gpu/drm/armada/armada_fbdev.c 

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sun, Jun 30, 2013 at 3:41 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
 On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
  On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
   + mutex_lock(dev-struct_mutex);
   + free = drm_mm_search_free(priv-linear, size, align, 0);
   + if (free)
   + obj-linear = drm_mm_get_block(free, size, align);
   + mutex_unlock(dev-struct_mutex);
 
  The two-stage search_free+get_block drm_mm interface is something I'd like
  to remove since it' complicates the interface for no gain. And the
  preallocation trick for atomic contexts is pretty racy as-is. Can you
  please convert this over to the insert_node interfaces which take a
  preallocate drm_mm_node?

 Yes and no.  This is only racy if it is callable from atomic contexts,
 which the above isn't, because it takes a mutex, and the above is the
 only place which allocations against that pool are done.  Mutexes can't
 be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
 interfaces.

 However, the insert_node interfaces appear not to solve any kind of race.
 All they do is allow the kmalloc to be moved out of this region into
 the user of this code sequence, while offering no additional locking or
 safety.  So the mutex lock around a call to drm_mm_insert_node*() is
 still required.

 As the kmalloc() happens beneath the mutex lock, another thread can't race
 with an allocation in the above code anyway.  The *only* reason I'll change
 this is that it is nicer to the system not to hold locks while allocating
 memory.

 Err.  There's bugs here in the other drivers such as i915 which follow
 your suggestion.  The problem is this:

 Every time they allocate using drm_mm_insert_node(), they kmalloc a new
 struct drm_mm_node for this function to fill in and attach.

 When they've done with the allocation, they call drm_mm_put_block().
 This places the node onto the mm's unused_nodes list provided we don't
 already have MM_UNUSED_TARGET nodes on that list.

Yeah, for drivers that never use the atomic alloc stuff it's pointless
waste of memory.

 So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
 at which point we then start freeing any further nodes - and with no
 way to pull these off that list, they all just sit there not being
 used.

 The only function which takes nodes off that list is drm_mm_kmalloc(),
 which is declared statically, and so isn't available to drivers.

 Are drivers which use the drm_mm_insert_node() function expected to
 also use drm_mm_remove_node(), kfreeing the node after that call,
 rather than using drm_mm_put_block() ?

Yeah, with the insert_node interface deallocing the node is the
driver's responsibility.

 Either way, I think someone needs to fix the other drivers and Cc those
 patches to Stable...

Yeah, I think it's time that I move that item up my todo list a lot.
And also add some decent kerneldoc to the drm_mm stuff.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Daniel Vetter]

On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
  +{
  +   struct drm_display_mode *adj = dcrtc-crtc.mode;
  +   uint32_t val = 0;
  +
  +   if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
  +   val |= CFG_CSC_RGB_STUDIO;
  +
  +   /*
  +* In auto mode, set the colorimetry, based upon the HDMI spec.
  +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
  +* ITU601.  It may be more appropriate to set this depending on
  +* the source - but what if the graphic frame is YUV and the
  +* video frame is RGB?
  +*/
  +   if ((adj-hdisplay == 1280  adj-vdisplay == 720 
  +!(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
  +   (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
  +   if (dcrtc-csc_yuv_mode == CSC_AUTO)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   }
  +
  +   /*
  +* We assume we're connected to a TV-like device, so the YUV-RGB
  +* conversion should produce a limited range.  We should set this
  +* depending on the connectors attached to this CRTC, and what
  +* kind of device they report being connected.
  +*/
  +   if (dcrtc-csc_rgb_mode == CSC_AUTO)
  +   val |= CFG_CSC_RGB_STUDIO;

 In the intel driver we check whether it's an cea mode with
 drm_match_cea_mode, e.g. in intel_hdmi.c:

   if (intel_hdmi-color_range_auto) {
   /* See CEA-861-E - 5.1 Default Encoding Parameters */
   if (intel_hdmi-has_hdmi_sink 
   drm_match_cea_mode(adjusted_mode)  1)
   intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
   else
   intel_hdmi-color_range = 0;
   }

 (The color_range gets then propageted to the right place since different
 platforms have different ways to do that in intel hw).

 Unfortunately, this disagrees with the HDMI v1.3a specification:

 | Default Colorimetry
 |
 | ...
 |
 | 480p, 480i, 576p, 576i, 240p and 288p
 |
 | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
 | video formats described in CEA-861-D is based on SMPTE 170M.
 |
 | 1080i, 1080p and 720p
 |
 | The default colorimetry for high-definition video formats (1080i, 1080p and
 | 720p) described in CEA-861-D is based on ITU-R BT.709-5.

 As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
 CEA when dealing with HDMI specifics.

 So, according to the HDMI specification, the default is that it's only
 1080i, 1080p and 720p which are 709, the others are 601.  This does not
 correspond with drm_match_cea_mode(adjusted_mode)  1.

Hm, sounds like we need to improve stuff a bit, maybe add a common cea
helper to the drm core with a bool is_hdmi to decide whether it's
palin CEA or HDMI-CEA. Ville (who's written the current code and the
match cea mode helper) can you please take a look?

 Unfortunately, given DRM's structure, there's no way for the CRTC layer
 to really know what its driving, and this setting is at the CRTC layer,
 not the output layer.  It may be that you have the CRTC duplicated
 between two different display devices with different characteristics,
 which means you have to choose one - or just not bother.  I've chosen
 the latter because it's a simpler solution, and is in no way any more
 correct than any other solution.

 This is also why these are exported to userspace via properties, so if
 userspace knows better, it can deal with those issues.

Yeah, allowing userspace to overwrite the default selection makes lots
of sense, we expose similar properties.

  +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
  *dev,
  +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
  +{
  +   struct armada_framebuffer *dfb;
  +   uint8_t format, config;
  +   int ret;
  +
  +   switch (mode-pixel_format) {
  +#define FMT(drm, fmt, mod) \
  +   case DRM_FORMAT_##drm:  \
  +   format = CFG_##fmt; \
  +   config = mod;   \
  +   break
  +   FMT(RGB565, 565,CFG_SWAPRB);
  +   FMT(BGR565, 565,0);
  +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
  +   FMT(ABGR1555,   1555,   0);
  +   FMT(RGB888, 888PACK,CFG_SWAPRB);
  +   FMT(BGR888, 888PACK,0);
  +   FMT(XRGB,   X888,   CFG_SWAPRB);
  +   FMT(XBGR,   X888,   0);
  +   FMT(ARGB,   ,   CFG_SWAPRB);
  +   FMT(ABGR,   ,   0);
  +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
  +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
  +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
  +   

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

On Sun, Jun 30, 2013 at 10:52 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
  +{
  +   struct drm_display_mode *adj = dcrtc-crtc.mode;
  +   uint32_t val = 0;
  +
  +   if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
  +   val |= CFG_CSC_RGB_STUDIO;
  +
  +   /*
  +* In auto mode, set the colorimetry, based upon the HDMI spec.
  +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
  +* ITU601.  It may be more appropriate to set this depending on
  +* the source - but what if the graphic frame is YUV and the
  +* video frame is RGB?
  +*/
  +   if ((adj-hdisplay == 1280  adj-vdisplay == 720 
  +!(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
  +   (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
  +   if (dcrtc-csc_yuv_mode == CSC_AUTO)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   }
  +
  +   /*
  +* We assume we're connected to a TV-like device, so the YUV-RGB
  +* conversion should produce a limited range.  We should set this
  +* depending on the connectors attached to this CRTC, and what
  +* kind of device they report being connected.
  +*/
  +   if (dcrtc-csc_rgb_mode == CSC_AUTO)
  +   val |= CFG_CSC_RGB_STUDIO;

 In the intel driver we check whether it's an cea mode with
 drm_match_cea_mode, e.g. in intel_hdmi.c:

   if (intel_hdmi-color_range_auto) {
   /* See CEA-861-E - 5.1 Default Encoding Parameters */
   if (intel_hdmi-has_hdmi_sink 
   drm_match_cea_mode(adjusted_mode)  1)
   intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
   else
   intel_hdmi-color_range = 0;
   }

 (The color_range gets then propageted to the right place since different
 platforms have different ways to do that in intel hw).

 Unfortunately, this disagrees with the HDMI v1.3a specification:

 | Default Colorimetry
 |
 | ...
 |
 | 480p, 480i, 576p, 576i, 240p and 288p
 |
 | The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
 | video formats described in CEA-861-D is based on SMPTE 170M.
 |
 | 1080i, 1080p and 720p
 |
 | The default colorimetry for high-definition video formats (1080i, 1080p and
 | 720p) described in CEA-861-D is based on ITU-R BT.709-5.

 As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
 CEA when dealing with HDMI specifics.

 So, according to the HDMI specification, the default is that it's only
 1080i, 1080p and 720p which are 709, the others are 601.  This does not
 correspond with drm_match_cea_mode(adjusted_mode)  1.

 Unfortunately, given DRM's structure, there's no way for the CRTC layer
 to really know what its driving, and this setting is at the CRTC layer,
 not the output layer.  It may be that you have the CRTC duplicated
 between two different display devices with different characteristics,
 which means you have to choose one - or just not bother.  I've chosen
 the latter because it's a simpler solution, and is in no way any more
 correct than any other solution.

 This is also why these are exported to userspace via properties, so if
 userspace knows better, it can deal with those issues.

  +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
  *dev,
  +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
  +{
  +   struct armada_framebuffer *dfb;
  +   uint8_t format, config;
  +   int ret;
  +
  +   switch (mode-pixel_format) {
  +#define FMT(drm, fmt, mod) \
  +   case DRM_FORMAT_##drm:  \
  +   format = CFG_##fmt; \
  +   config = mod;   \
  +   break
  +   FMT(RGB565, 565,CFG_SWAPRB);
  +   FMT(BGR565, 565,0);
  +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
  +   FMT(ABGR1555,   1555,   0);
  +   FMT(RGB888, 888PACK,CFG_SWAPRB);
  +   FMT(BGR888, 888PACK,0);
  +   FMT(XRGB,   X888,   CFG_SWAPRB);
  +   FMT(XBGR,   X888,   0);
  +   FMT(ARGB,   ,   CFG_SWAPRB);
  +   FMT(ABGR,   ,   0);
  +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
  +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
  +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
  +   FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU422, 422,CFG_YUV2RGB);
  +   FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU420, 420,CFG_YUV2RGB);
  +   FMT(C8, PSEUDO8,0);
  +#undef FMT
  +   default:
  +   

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Dave Airlie]

On Mon, Jul 1, 2013 at 10:17 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Mon, Jul 01, 2013 at 10:01:30AM +1000, Dave Airlie wrote:
 OMG I'm working in a subsystem where stuff is being developed, with only
 a few resources! I know my full time job isn't maintaining a 500,000
 line subsystem,
 and the sub maintainers and developers do a great job refactoring
 where required.

 lots of other driver authors have made substantial changes to the drm core as
 they've written drivers, you spot one bit of refactoring and its a
 major shortcoming
 of the entire subsystem and development community.

 how about instead of writing:
 However, at least I've taken the time to _think_ about what I'm doing
 and realise that there _is_ scope here for the DRM core to improve,
 rather than burying this stuff deep inside my driver like everyone else
 has.  That's no reason to penalise patches from the good guys who think

 you go with
 I noticed this piece of functionality could be refactored, here is a
 patch adding them to
 the core, does anyone think its a good idea?

 As Daniel pointed out every driver submitted has refactored things,
 even exynos did a
 lot of work to be the first ARM driver we shipped, the DRM core
 doesn't write itself and
 I fully expect driver authors to be the ones that tell me what needs
 refactoring, since
 they are on the pointy end, but to state that you are the only person
 ever to think about
 things is frankly being a dick.

 Lets try for less dick, and more productive in future.

 And you can stick your dick back where you got it from David.  Really,
 your response is totally uncalled for.

 Let's try realising that I only have very limited time to put into this
 and unlike the other submitters who have been _paid_ to get their code
 sorted, this is being done purely for free - which means it's really
 low priority.  As you should already realise because I've stated already
 that I *really* don't care whether it gets into mainline or not.

Really not every driver maintainer is paid to submit their drivers, if
you'd any clue
you'd understand that you aren't special. We currently have a number of non-paid
maintainer drivers being submitted and worked on, via for one, gma500
is maintain
in spare time, the tegra driver isn't purely a work of paid
dedication, I maintain
5 drivers currently, and I certainly don't do all of that in my day
job. Keeping attitude off
this list is what I do, I don't care if you don't appreciate the work
put in by other developers
to the drm subsystem, but I do and I won't have someone come in here
saying you guys
suck when clearly they've no historical perspective and as such are
plainly trolling.

Yes the DRM has growing pains, yes there is a lots of legacy shit in
the way of making
things clean, yes writing a driver for SoC is more painful than
necessary, but overall
its a majorly moving target, 5 years ago DRM/KMS hadn't considered ARM, now its
probably most of what we have to consider.

 If you want stuff to be refactored in DRM, you need to find someone
 with more time to do it.

I pointed out I didn't want stuff refactored, I wanted a driver author
to suggest
possible refactorings with a patch to add the interfaces, and suggest its a
good idea.

Split the patch, one to add the additions to the core without changing
anything, then just
have your driver use them. If other driver writes want to use them they'll
figure it out, and maybe someone else will go around and clean up the other
drivers.

I'm really hoping the OLPC guys pick up this work and care enough to
merge it, if not,
its a bit of a waste.

Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel