Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341181 > unrolled thread

Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Stefan Agner <stefan@agner.ch> - 2016-02-24 00:40 +0100
    Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Philipp Zabel <p.zabel@pengutronix.de> - 2016-02-24 11:40 +0100
      Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-02-25 09:00 +0100
    Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-02-24 12:10 +0100
      Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Stefan Agner <stefan@agner.ch> - 2016-02-24 20:20 +0100
      Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-02-24 21:20 +0100

#1341181 — Re: [PATCH v2 0/3] drm: introduce bus_flags for pixel clock polarity

Any comments on this?

Also added Manfred, Tomi and Boris to CC which previously attended in
similar discussions.

Previous discussions:
http://thread.gmane.org/gmane.linux.kernel.api/12830
http://thread.gmane.org/gmane.comp.video.dri.devel/96240/

I think one of the main observation so far was that the pixel clock
polarity is not a property of the mode, and therefor does not fit into
the DRM_MODE_FLAG. This has been pointed out nicely by Russel:
http://thread.gmane.org/gmane.comp.video.dri.devel/96240/focus=96260

Embedded displays connected through parallel bus make use of the
bus_formats field in drm_display_mode. This field defines what kind of
bus format the display requires. This patch follows that idea and adds
bus_flags. bus_flags can be used to define specific bus properties
required by the display, such as pixel clock or data enable polarity...


On 2016-02-08 13:57, Stefan Agner wrote:
> Hi,
> 
> This is a new & split out version of the last patch of my
> "drm/fsl-dcu: fixes and enhancements" patchset:
> https://lkml.org/lkml/2015/11/18/949
> 
> Instead of using struct drm_display_mode to convey the pixel clock
> polarity information, this patchset introduces a new field called
> bus_flags stored in struct drm_display_info.

Note that this solution has been briefly discussed on IRC:
https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2016-02-08

--
Stefan

> 
> Changes since v1:
> - Introduce bus_flags to convey the pixel clock polarity from
>   panel-simple.c to the driver.
> 
> Stefan Agner (3):
>   drm/fsl-dcu: use mode flags for hsync/vsync polarity
>   drm: introduce bus_flags in drm_display_info
>   drm/fsl-dcu: use bus_flags for pixel clock polarity
> 
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h  |  4 ++--
>  drivers/gpu/drm/panel/panel-simple.c       |  6 +++++-
>  include/drm/drm_crtc.h                     |  9 +++++++++
>  4 files changed, 29 insertions(+), 6 deletions(-)

[toc] | [next] | [standalone]

#1341792

Am Dienstag, den 23.02.2016, 15:30 -0800 schrieb Stefan Agner:
> Any comments on this?

None other that I'm all in favor. consider patch 2
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> Also added Manfred, Tomi and Boris to CC which previously attended in
> similar discussions.
> 
> Previous discussions:
> http://thread.gmane.org/gmane.linux.kernel.api/12830
> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/
> 
> I think one of the main observation so far was that the pixel clock
> polarity is not a property of the mode, and therefor does not fit into
> the DRM_MODE_FLAG. This has been pointed out nicely by Russel:
> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/focus=96260
> 
> Embedded displays connected through parallel bus make use of the
> bus_formats field in drm_display_mode. This field defines what kind of
> bus format the display requires. This patch follows that idea and adds
> bus_flags. bus_flags can be used to define specific bus properties
> required by the display, such as pixel clock or data enable polarity...

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1342929

On 2016-02-24 11:28, Philipp Zabel wrote:
> Am Dienstag, den 23.02.2016, 15:30 -0800 schrieb Stefan Agner:
>> Any comments on this?
> 
> None other that I'm all in favor. consider patch 2
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
Same here!
Acked-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>

regards
Manfred

[toc] | [prev] | [next] | [standalone]

#1341862

[Multipart message — attachments visible in raw view] — view raw

Hi,

On 24/02/16 01:30, Stefan Agner wrote:
> Any comments on this?
> 
> Also added Manfred, Tomi and Boris to CC which previously attended in
> similar discussions.
> 
> Previous discussions:
> http://thread.gmane.org/gmane.linux.kernel.api/12830
> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/
> 
> I think one of the main observation so far was that the pixel clock
> polarity is not a property of the mode, and therefor does not fit into
> the DRM_MODE_FLAG. This has been pointed out nicely by Russel:
> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/focus=96260
> 
> Embedded displays connected through parallel bus make use of the
> bus_formats field in drm_display_mode. This field defines what kind of
> bus format the display requires. This patch follows that idea and adds
> bus_flags. bus_flags can be used to define specific bus properties
> required by the display, such as pixel clock or data enable polarity...

I think it would be good to split the generic and fsl changes to
separate patches.

I agree that pixel clock polarity shouldn't be visible to userspace.

I had a look at MIPI DPI spec, and it says "The rising edge of PCLK is
used by the display module to capture pixel data.". So, I think that
means if the panels are MIPI DPI compatible, they should always sample
at rising edge. I'm sure there are exceptions, but that behaviour should
probably be the default, then.

I'm also a bit curious on what is "videomode". Why is sync polarity part
of it, and settable by the userspace, but not pixel clock polarity?
"videomode" is just whatever is in the CEA spec, because DRM originates
from the PC world? Is there any reason nowadays for the user to ever set
sync polarities?

For MIPI DPI panels, sync polarity is as much a property of the panel as
pixel clock polarity: there's only one correct setting for it (usually).

 Tomi

[toc] | [prev] | [next] | [standalone]

#1342360

On 2016-02-24 03:06, Tomi Valkeinen wrote:
> Hi,
> 
> On 24/02/16 01:30, Stefan Agner wrote:
>> Any comments on this?
>>
>> Also added Manfred, Tomi and Boris to CC which previously attended in
>> similar discussions.
>>
>> Previous discussions:
>> http://thread.gmane.org/gmane.linux.kernel.api/12830
>> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/
>>
>> I think one of the main observation so far was that the pixel clock
>> polarity is not a property of the mode, and therefor does not fit into
>> the DRM_MODE_FLAG. This has been pointed out nicely by Russel:
>> http://thread.gmane.org/gmane.comp.video.dri.devel/96240/focus=96260
>>
>> Embedded displays connected through parallel bus make use of the
>> bus_formats field in drm_display_mode. This field defines what kind of
>> bus format the display requires. This patch follows that idea and adds
>> bus_flags. bus_flags can be used to define specific bus properties
>> required by the display, such as pixel clock or data enable polarity...
> 
> I think it would be good to split the generic and fsl changes to
> separate patches.

I guess we talk about PATCH 3/3?

This is intentionally in one patch: The problem is that the current
default setting of the driver is the correct setting for that one NEC
display. The patch changes the default setting of the display controller
driver, but to make sure the NEC display continues to work I need to add
the polarity flag to the display. Therefor I feel this is one logical
change... Also this way the display keeps working between every commit
(bisectability...)

The core changes (the flag defines and adding the field to struct
drm_display_info) are in a separate patch.

> 
> I agree that pixel clock polarity shouldn't be visible to userspace.
> 
> I had a look at MIPI DPI spec, and it says "The rising edge of PCLK is
> used by the display module to capture pixel data.". So, I think that
> means if the panels are MIPI DPI compatible, they should always sample
> at rising edge. I'm sure there are exceptions, but that behaviour should
> probably be the default, then.

Yes, that is the new default of the driver.

> 
> I'm also a bit curious on what is "videomode". Why is sync polarity part
> of it, and settable by the userspace, but not pixel clock polarity?
> "videomode" is just whatever is in the CEA spec, because DRM originates
> from the PC world? Is there any reason nowadays for the user to ever set
> sync polarities?
> 
> For MIPI DPI panels, sync polarity is as much a property of the panel as
> pixel clock polarity: there's only one correct setting for it (usually).
> 

We probably could add the sync properties to the bus_flags too, and
those would take precedence over the mode sync polarities...? Certainly
something which could be done in a follow up patch.

--
Stefan

[toc] | [prev] | [next] | [standalone]

#1342418

On Wed, Feb 24, 2016 at 01:06:39PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 24/02/16 01:30, Stefan Agner wrote:
> > Any comments on this?
> > 
> > Also added Manfred, Tomi and Boris to CC which previously attended in
> > similar discussions.
> > 
> > Previous discussions:
> > http://thread.gmane.org/gmane.linux.kernel.api/12830
> > http://thread.gmane.org/gmane.comp.video.dri.devel/96240/
> > 
> > I think one of the main observation so far was that the pixel clock
> > polarity is not a property of the mode, and therefor does not fit into
> > the DRM_MODE_FLAG. This has been pointed out nicely by Russel:
> > http://thread.gmane.org/gmane.comp.video.dri.devel/96240/focus=96260
> > 
> > Embedded displays connected through parallel bus make use of the
> > bus_formats field in drm_display_mode. This field defines what kind of
> > bus format the display requires. This patch follows that idea and adds
> > bus_flags. bus_flags can be used to define specific bus properties
> > required by the display, such as pixel clock or data enable polarity...
> 
> I think it would be good to split the generic and fsl changes to
> separate patches.
> 
> I agree that pixel clock polarity shouldn't be visible to userspace.
> 
> I had a look at MIPI DPI spec, and it says "The rising edge of PCLK is
> used by the display module to capture pixel data.". So, I think that
> means if the panels are MIPI DPI compatible, they should always sample
> at rising edge. I'm sure there are exceptions, but that behaviour should
> probably be the default, then.
> 
> I'm also a bit curious on what is "videomode". Why is sync polarity part
> of it, and settable by the userspace, but not pixel clock polarity?
> "videomode" is just whatever is in the CEA spec, because DRM originates
> from the PC world? Is there any reason nowadays for the user to ever set
> sync polarities?

Yes, the EDID/something will tell us the modes the display claims to
use including the sync polarities (and note that they are not the same
for every listed mode), but the way the kms API works is that the user
specifies the full timings anyway. So if we didn't have sync polarities
as part of the mode, we wouldn't know what to output unless we went
trawling through the mode list looking for a match, which may not even
be present if the user specified a custom mode.

> 
> For MIPI DPI panels, sync polarity is as much a property of the panel as
> pixel clock polarity: there's only one correct setting for it (usually).

In i915 we ignore the user requested timings almost entirely when
dealing with LVDS/eDP/DSI. The only think we keep is that size of
the active video portion, and we use that as the input size for
the panel fitter (== scaler). All the actual timings used to
drive the display come from EDID or the VBT (video BIOS table).

For external displays we do respect what the user has requested,
including the sync polarities.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web