Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized

[Stéphane Marchesin]

On Tue, Apr 24, 2018 at 6:04 AM, Daniel Vetter  wrote:
> On Fri, Apr 20, 2018 at 09:55:32AM -0400, Sean Paul wrote:
>> On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
>> > This fixes a NULL pointer dereference that can happen if the UDL
>> > driver is unloaded before the framebuffer is initialized. This can
>> > happen e.g. if the USB device is unplugged right after it was plugged
>> > in.
>> >
>>
>> JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
>> practice to add a Suggested-by tag to give credit.
>>
>> Reviewed-by: Sean Paul 
>
> I think a bit more detail in the commit message on why this is even
> possible would be good. I think it can only happen when you only plug in
> the udl, without anything connected.

Hehe, I was just reviewing this code internally, so I can answer that one :)

It happens when fbdev is disabled (which is the case for Chrome OS).
Even though intialization of the fbdev part is optional (it's done in
udlfb_create which is the callback for fb_probe()), the teardown isn't
optional (udl_driver_unload -> udl_fbdev_cleanup ->
udl_fbdev_destroy).

Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
always set during udl_fbdev_init.

Stéphane


>
> In that case fbdev setup will be delayed until something shows up (so we
> don't pin a too small fb for it, a feature requested by soc people). This
> can easily be tested:
> First:
> - plug in udl device
> - wait a minute or so (to make it clear it's not a race)
> - unplug
>
> And then:
> - plug in an udl device, but with something connected.
> - unplug right away.
>
> I expect that in the first case you'll always blow up, but in the 2nd one
> you'll never blow up (no matter how fast you unplug).
>
> Cheers, Daniel
>
>
>
>>
>> > Signed-off-by: Emil Lundmark 
>> > ---
>> >  drivers/gpu/drm/udl/udl_fb.c | 8 +---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> > index 2ebdc6d5a76e..5754e37f741b 100644
>> > --- a/drivers/gpu/drm/udl/udl_fb.c
>> > +++ b/drivers/gpu/drm/udl/udl_fb.c
>> > @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>> >  {
>> > drm_fb_helper_unregister_fbi(>helper);
>> > drm_fb_helper_fini(>helper);
>> > -   drm_framebuffer_unregister_private(>ufb.base);
>> > -   drm_framebuffer_cleanup(>ufb.base);
>> > -   drm_gem_object_put_unlocked(>ufb.obj->base);
>> > +   if (ufbdev->ufb.obj) {
>> > +   drm_framebuffer_unregister_private(>ufb.base);
>> > +   drm_framebuffer_cleanup(>ufb.base);
>> > +   drm_gem_object_put_unlocked(>ufb.obj->base);
>> > +   }
>> >  }
>> >
>> >  int udl_fbdev_init(struct drm_device *dev)
>> > --
>> > 2.17.0.484.g0c8726318c-goog
>> >
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized

[Stéphane Marchesin]

On Tue, Apr 24, 2018 at 6:04 AM, Daniel Vetter  wrote:
> On Fri, Apr 20, 2018 at 09:55:32AM -0400, Sean Paul wrote:
>> On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
>> > This fixes a NULL pointer dereference that can happen if the UDL
>> > driver is unloaded before the framebuffer is initialized. This can
>> > happen e.g. if the USB device is unplugged right after it was plugged
>> > in.
>> >
>>
>> JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
>> practice to add a Suggested-by tag to give credit.
>>
>> Reviewed-by: Sean Paul 
>
> I think a bit more detail in the commit message on why this is even
> possible would be good. I think it can only happen when you only plug in
> the udl, without anything connected.

Hehe, I was just reviewing this code internally, so I can answer that one :)

It happens when fbdev is disabled (which is the case for Chrome OS).
Even though intialization of the fbdev part is optional (it's done in
udlfb_create which is the callback for fb_probe()), the teardown isn't
optional (udl_driver_unload -> udl_fbdev_cleanup ->
udl_fbdev_destroy).

Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
always set during udl_fbdev_init.

Stéphane


>
> In that case fbdev setup will be delayed until something shows up (so we
> don't pin a too small fb for it, a feature requested by soc people). This
> can easily be tested:
> First:
> - plug in udl device
> - wait a minute or so (to make it clear it's not a race)
> - unplug
>
> And then:
> - plug in an udl device, but with something connected.
> - unplug right away.
>
> I expect that in the first case you'll always blow up, but in the 2nd one
> you'll never blow up (no matter how fast you unplug).
>
> Cheers, Daniel
>
>
>
>>
>> > Signed-off-by: Emil Lundmark 
>> > ---
>> >  drivers/gpu/drm/udl/udl_fb.c | 8 +---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> > index 2ebdc6d5a76e..5754e37f741b 100644
>> > --- a/drivers/gpu/drm/udl/udl_fb.c
>> > +++ b/drivers/gpu/drm/udl/udl_fb.c
>> > @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>> >  {
>> > drm_fb_helper_unregister_fbi(>helper);
>> > drm_fb_helper_fini(>helper);
>> > -   drm_framebuffer_unregister_private(>ufb.base);
>> > -   drm_framebuffer_cleanup(>ufb.base);
>> > -   drm_gem_object_put_unlocked(>ufb.obj->base);
>> > +   if (ufbdev->ufb.obj) {
>> > +   drm_framebuffer_unregister_private(>ufb.base);
>> > +   drm_framebuffer_cleanup(>ufb.base);
>> > +   drm_gem_object_put_unlocked(>ufb.obj->base);
>> > +   }
>> >  }
>> >
>> >  int udl_fbdev_init(struct drm_device *dev)
>> > --
>> > 2.17.0.484.g0c8726318c-goog
>> >
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Thu, Jul 13, 2017 at 10:42 AM, Ville Syrjälä
<ville.syrj...@linux.intel.com> wrote:
> On Thu, Jul 13, 2017 at 09:23:11AM -0700, Stéphane Marchesin wrote:
>> On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
>> <ville.syrj...@linux.intel.com> wrote:
>> > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>> >> <ville.syrj...@linux.intel.com> wrote:
>> >> >
>> >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> >> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> >> > >
>> >> > > > In several instances the driver passes an 'enum pipe' value to a
>> >> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x 
>> >> > > > and
>> >> > > > TRANSCODER_x have the same values this doesn't cause functional
>> >> > > > problems. Still it is incorrect and causes clang to generate 
>> >> > > > warnings
>> >> > > > like this:
>> >> > > >
>> >> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> >> > > >   conversion from enumeration type 'enum transcoder' to different
>> >> > > >   enumeration type 'enum pipe' [-Wenum-conversion]
>> >> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> >> > > >
>> >> > > > Change the code to pass values of the type expected by the callee.
>> >> > > >
>> >> > > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>> >> > > > ---
>> >> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> >> > > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
>> >> > > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
>> >> > > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
>> >> > > >  4 files changed, 14 insertions(+), 8 deletions(-)
>> >> > >
>> >> > > Ping, any comments on this patch?
>> >> >
>> >> > I'm not convinced the patch is making things any better really. To
>> >> > fix this really properly, I think we'd need to introduce a new enum
>> >> > pch_transcoder and thus avoid the confusion of which type of
>> >> > transcoder we're talking about. Currently most places expect an
>> >> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> >> > when dealing with CPU transcoders. But there are some exceptions
>> >> > of course.
>> >>
>> >>
>> >> I don't follow -- these functions take an enum transcoder; what's
>> >> wrong about passing what they expect? It seems like what you are
>> >> asking for has nothing to do with the warning here...
>> >
>> > There's a warning? I don't get any.
>>
>> Yup, clang generates a warning.
>>
>> >
>> > Anyways, I just don't see much point in blindly changing the types
>> > because it doesn't actually solve the underlying confusion for human
>> > readers. It might even make it worse, not sure.
>>
>> The function expects type A, you pass type B, how can that ever be the
>> right thing to do?
>
> Because maybe the function should be taking in type B instead.

So, if you think this is wrong, can you fix this warning in a way that
you'd like?

Stéphane

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

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Thu, Jul 13, 2017 at 10:42 AM, Ville Syrjälä
 wrote:
> On Thu, Jul 13, 2017 at 09:23:11AM -0700, Stéphane Marchesin wrote:
>> On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
>>  wrote:
>> > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>> >>  wrote:
>> >> >
>> >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> >> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> >> > >
>> >> > > > In several instances the driver passes an 'enum pipe' value to a
>> >> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x 
>> >> > > > and
>> >> > > > TRANSCODER_x have the same values this doesn't cause functional
>> >> > > > problems. Still it is incorrect and causes clang to generate 
>> >> > > > warnings
>> >> > > > like this:
>> >> > > >
>> >> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> >> > > >   conversion from enumeration type 'enum transcoder' to different
>> >> > > >   enumeration type 'enum pipe' [-Wenum-conversion]
>> >> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> >> > > >
>> >> > > > Change the code to pass values of the type expected by the callee.
>> >> > > >
>> >> > > > Signed-off-by: Matthias Kaehlcke 
>> >> > > > ---
>> >> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> >> > > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
>> >> > > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
>> >> > > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
>> >> > > >  4 files changed, 14 insertions(+), 8 deletions(-)
>> >> > >
>> >> > > Ping, any comments on this patch?
>> >> >
>> >> > I'm not convinced the patch is making things any better really. To
>> >> > fix this really properly, I think we'd need to introduce a new enum
>> >> > pch_transcoder and thus avoid the confusion of which type of
>> >> > transcoder we're talking about. Currently most places expect an
>> >> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> >> > when dealing with CPU transcoders. But there are some exceptions
>> >> > of course.
>> >>
>> >>
>> >> I don't follow -- these functions take an enum transcoder; what's
>> >> wrong about passing what they expect? It seems like what you are
>> >> asking for has nothing to do with the warning here...
>> >
>> > There's a warning? I don't get any.
>>
>> Yup, clang generates a warning.
>>
>> >
>> > Anyways, I just don't see much point in blindly changing the types
>> > because it doesn't actually solve the underlying confusion for human
>> > readers. It might even make it worse, not sure.
>>
>> The function expects type A, you pass type B, how can that ever be the
>> right thing to do?
>
> Because maybe the function should be taking in type B instead.

So, if you think this is wrong, can you fix this warning in a way that
you'd like?

Stéphane

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

Re: [PATCH] drm/udl: Make page_flip asynchronous

[Stéphane Marchesin]

On Mon, Jul 10, 2017 at 11:58 PM, Daniel Vetter  wrote:
> On Fri, Jul 7, 2017 at 7:48 AM, Dawid Kurek  
> wrote:
>> In page_flip vblank is sent with no delay. Driver does not know when the
>> actual update is present on the display and has no means for getting
>> this information from a device. It is practically impossible to say
>> exactly *when* as there is also i.e. a usb delay.
>>
>> When we are unable to determine when the vblank actually happens we may
>> assume it will behave accordingly, i.e. it will present frames with
>> proper timing. In the worst case scenario it should take up to duration
>> of one frame (we may get new frame in the device just after presenting
>> current one so we would need to wait for the whole frame).
>>
>> Because of the asynchronous nature of the delay we need to synchronize:
>>  * read/write vrefresh/page_flip data when changing mode and
>>preparing/executing vblank
>>  * USB requests to prevent interleaved access to URBs for two different
>>frame buffers
>>
>> All those changes are backports from ChromeOS:
>>   1. https://chromium-review.googlesource.com/250622
>>   2. https://chromium-review.googlesource.com/249450
>>   partially, only change in udl_modeset.c for 'udl_flip_queue'
>>   3. https://chromium-review.googlesource.com/321378
>>   4. https://chromium-review.googlesource.com/324119
>> + fixes for checkpatch and latest drm changes
>>
>> Cc: h...@chromium.org
>> Cc: marc...@chromium.org
>> Cc: za...@chromium.org
>> Cc: db...@google.com
>> Signed-off-by: Dawid Kurek 
>
> Can't we roll this driver over to the atomic helpers instead? There
> you get nonblocking pretty much for free ... I'm not sure extending
> the old modeset code has all that much benefit really.

This code certainly has value by itself; it makes the driver more
efficient. I think the best can sometimes be the enemy of the good --
this code is here and written, but I don't think any of us is going to
tackle atomic for udl.

Stéphane


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

Re: [PATCH] drm/udl: Make page_flip asynchronous

[Stéphane Marchesin]

On Mon, Jul 10, 2017 at 11:58 PM, Daniel Vetter  wrote:
> On Fri, Jul 7, 2017 at 7:48 AM, Dawid Kurek  
> wrote:
>> In page_flip vblank is sent with no delay. Driver does not know when the
>> actual update is present on the display and has no means for getting
>> this information from a device. It is practically impossible to say
>> exactly *when* as there is also i.e. a usb delay.
>>
>> When we are unable to determine when the vblank actually happens we may
>> assume it will behave accordingly, i.e. it will present frames with
>> proper timing. In the worst case scenario it should take up to duration
>> of one frame (we may get new frame in the device just after presenting
>> current one so we would need to wait for the whole frame).
>>
>> Because of the asynchronous nature of the delay we need to synchronize:
>>  * read/write vrefresh/page_flip data when changing mode and
>>preparing/executing vblank
>>  * USB requests to prevent interleaved access to URBs for two different
>>frame buffers
>>
>> All those changes are backports from ChromeOS:
>>   1. https://chromium-review.googlesource.com/250622
>>   2. https://chromium-review.googlesource.com/249450
>>   partially, only change in udl_modeset.c for 'udl_flip_queue'
>>   3. https://chromium-review.googlesource.com/321378
>>   4. https://chromium-review.googlesource.com/324119
>> + fixes for checkpatch and latest drm changes
>>
>> Cc: h...@chromium.org
>> Cc: marc...@chromium.org
>> Cc: za...@chromium.org
>> Cc: db...@google.com
>> Signed-off-by: Dawid Kurek 
>
> Can't we roll this driver over to the atomic helpers instead? There
> you get nonblocking pretty much for free ... I'm not sure extending
> the old modeset code has all that much benefit really.

This code certainly has value by itself; it makes the driver more
efficient. I think the best can sometimes be the enemy of the good --
this code is here and written, but I don't think any of us is going to
tackle atomic for udl.

Stéphane


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

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
<ville.syrj...@linux.intel.com> wrote:
> On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>> <ville.syrj...@linux.intel.com> wrote:
>> >
>> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> > >
>> > > > In several instances the driver passes an 'enum pipe' value to a
>> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
>> > > > TRANSCODER_x have the same values this doesn't cause functional
>> > > > problems. Still it is incorrect and causes clang to generate warnings
>> > > > like this:
>> > > >
>> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> > > >   conversion from enumeration type 'enum transcoder' to different
>> > > >   enumeration type 'enum pipe' [-Wenum-conversion]
>> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> > > >
>> > > > Change the code to pass values of the type expected by the callee.
>> > > >
>> > > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> > > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
>> > > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
>> > > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
>> > > >  4 files changed, 14 insertions(+), 8 deletions(-)
>> > >
>> > > Ping, any comments on this patch?
>> >
>> > I'm not convinced the patch is making things any better really. To
>> > fix this really properly, I think we'd need to introduce a new enum
>> > pch_transcoder and thus avoid the confusion of which type of
>> > transcoder we're talking about. Currently most places expect an
>> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> > when dealing with CPU transcoders. But there are some exceptions
>> > of course.
>>
>>
>> I don't follow -- these functions take an enum transcoder; what's
>> wrong about passing what they expect? It seems like what you are
>> asking for has nothing to do with the warning here...
>
> There's a warning? I don't get any.

Yup, clang generates a warning.

>
> Anyways, I just don't see much point in blindly changing the types
> because it doesn't actually solve the underlying confusion for human
> readers. It might even make it worse, not sure.

The function expects type A, you pass type B, how can that ever be the
right thing to do?

Stéphane


>
> --
> Ville Syrjälä
> Intel OTC

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
 wrote:
> On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>>  wrote:
>> >
>> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> > >
>> > > > In several instances the driver passes an 'enum pipe' value to a
>> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
>> > > > TRANSCODER_x have the same values this doesn't cause functional
>> > > > problems. Still it is incorrect and causes clang to generate warnings
>> > > > like this:
>> > > >
>> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> > > >   conversion from enumeration type 'enum transcoder' to different
>> > > >   enumeration type 'enum pipe' [-Wenum-conversion]
>> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> > > >
>> > > > Change the code to pass values of the type expected by the callee.
>> > > >
>> > > > Signed-off-by: Matthias Kaehlcke 
>> > > > ---
>> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> > > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
>> > > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
>> > > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
>> > > >  4 files changed, 14 insertions(+), 8 deletions(-)
>> > >
>> > > Ping, any comments on this patch?
>> >
>> > I'm not convinced the patch is making things any better really. To
>> > fix this really properly, I think we'd need to introduce a new enum
>> > pch_transcoder and thus avoid the confusion of which type of
>> > transcoder we're talking about. Currently most places expect an
>> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> > when dealing with CPU transcoders. But there are some exceptions
>> > of course.
>>
>>
>> I don't follow -- these functions take an enum transcoder; what's
>> wrong about passing what they expect? It seems like what you are
>> asking for has nothing to do with the warning here...
>
> There's a warning? I don't get any.

Yup, clang generates a warning.

>
> Anyways, I just don't see much point in blindly changing the types
> because it doesn't actually solve the underlying confusion for human
> readers. It might even make it worse, not sure.

The function expects type A, you pass type B, how can that ever be the
right thing to do?

Stéphane


>
> --
> Ville Syrjälä
> Intel OTC

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
 wrote:
>
> On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> >
> > > In several instances the driver passes an 'enum pipe' value to a
> > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> > > TRANSCODER_x have the same values this doesn't cause functional
> > > problems. Still it is incorrect and causes clang to generate warnings
> > > like this:
> > >
> > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> > >   conversion from enumeration type 'enum transcoder' to different
> > >   enumeration type 'enum pipe' [-Wenum-conversion]
> > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > >
> > > Change the code to pass values of the type expected by the callee.
> > >
> > > Signed-off-by: Matthias Kaehlcke 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
> > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
> > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
> > >  4 files changed, 14 insertions(+), 8 deletions(-)
> >
> > Ping, any comments on this patch?
>
> I'm not convinced the patch is making things any better really. To
> fix this really properly, I think we'd need to introduce a new enum
> pch_transcoder and thus avoid the confusion of which type of
> transcoder we're talking about. Currently most places expect an
> enum pipe when dealing with PCH transcoders, and enum transcoder
> when dealing with CPU transcoders. But there are some exceptions
> of course.


I don't follow -- these functions take an enum transcoder; what's
wrong about passing what they expect? It seems like what you are
asking for has nothing to do with the warning here...

Stéphane

>
> --
> Ville Syrjälä
> Intel OTC
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Stéphane Marchesin]

On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
 wrote:
>
> On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> >
> > > In several instances the driver passes an 'enum pipe' value to a
> > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> > > TRANSCODER_x have the same values this doesn't cause functional
> > > problems. Still it is incorrect and causes clang to generate warnings
> > > like this:
> > >
> > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> > >   conversion from enumeration type 'enum transcoder' to different
> > >   enumeration type 'enum pipe' [-Wenum-conversion]
> > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > >
> > > Change the code to pass values of the type expected by the callee.
> > >
> > > Signed-off-by: Matthias Kaehlcke 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > >  drivers/gpu/drm/i915/intel_dp.c  | 6 --
> > >  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
> > >  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
> > >  4 files changed, 14 insertions(+), 8 deletions(-)
> >
> > Ping, any comments on this patch?
>
> I'm not convinced the patch is making things any better really. To
> fix this really properly, I think we'd need to introduce a new enum
> pch_transcoder and thus avoid the confusion of which type of
> transcoder we're talking about. Currently most places expect an
> enum pipe when dealing with PCH transcoders, and enum transcoder
> when dealing with CPU transcoders. But there are some exceptions
> of course.


I don't follow -- these functions take an enum transcoder; what's
wrong about passing what they expect? It seems like what you are
asking for has nothing to do with the warning here...

Stéphane

>
> --
> Ville Syrjälä
> Intel OTC
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/2] drm/panel: add innolux,p079zca panel driver

[Stéphane Marchesin]

can you add bps here too?

On Tue, Mar 21, 2017 at 6:53 PM, Chris Zhong  wrote:
> Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI
> panel.
>
> Signed-off-by: Chris Zhong 
> Reviewed-by: Sean Paul 
> Tested-by: Brian Norris 
> ---
>
> Changes in v3:
> - printk err after regulator_disable(innolux->supply)
>
> Changes in v2:
> - add some error check
> - always use Low power mode to send commend
> - add comments for all the sleep
> - use DRM_DEV_ERROR instead of dev_err
>
>  drivers/gpu/drm/panel/Kconfig |  11 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 345 
> ++
>  3 files changed, 357 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 62aba97..99dd010 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -18,6 +18,17 @@ config DRM_PANEL_SIMPLE
>   that it can be automatically turned off when the panel goes into a
>   low power state.
>
> +config DRM_PANEL_INNOLUX_P079ZCA
> +   tristate "Innolux P079ZCA panel"
> +   depends on OF
> +   depends on DRM_MIPI_DSI
> +   depends on BACKLIGHT_CLASS_DEVICE
> +   help
> + Say Y here if you want to enable support for Innolux P079ZCA
> + TFT-LCD modules. The panel has a 1024x768 resolution and uses
> + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> + the host and has a built-in LED backlight.
> +
>  config DRM_PANEL_JDI_LT070ME05000
> tristate "JDI LT070ME05000 WUXGA DSI panel"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index a5c7ec0..bda2aa4 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
> +obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
>  obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
> panel-panasonic-vvx10f034n00.o
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
> b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> new file mode 100644
> index 000..9f3423a0
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -0,0 +1,345 @@
> +/*
> + * Copyright (c) 2017, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct innolux_panel {
> +   struct drm_panel base;
> +   struct mipi_dsi_device *link;
> +
> +   struct backlight_device *backlight;
> +   struct regulator *supply;
> +   struct gpio_desc *enable_gpio;
> +
> +   bool prepared;
> +   bool enabled;
> +};
> +
> +static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel)
> +{
> +   return container_of(panel, struct innolux_panel, base);
> +}
> +
> +static int innolux_panel_disable(struct drm_panel *panel)
> +{
> +   struct innolux_panel *innolux = to_innolux_panel(panel);
> +   int err;
> +
> +   if (!innolux->enabled)
> +   return 0;
> +
> +   if (innolux->backlight) {
> +   innolux->backlight->props.power = FB_BLANK_POWERDOWN;
> +   backlight_update_status(innolux->backlight);
> +   }
> +
> +   err = mipi_dsi_dcs_set_display_off(innolux->link);
> +   if (err < 0)
> +   DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> + err);
> +
> +   innolux->enabled = false;
> +
> +   return 0;
> +}
> +
> +static int innolux_panel_unprepare(struct drm_panel *panel)
> +{
> +   struct innolux_panel *innolux = to_innolux_panel(panel);
> +   int err;
> +
> +   if (!innolux->prepared)
> +   return 0;
> +
> +   err = mipi_dsi_dcs_enter_sleep_mode(innolux->link);
> +   if (err < 0) {
> +   DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> + err);
> +   return err;
> +   }
> +
> +   gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> +
> +   /* T8: 80ms - 1000ms */
> +   msleep(80);
> +
> +   err = regulator_disable(innolux->supply);
> +   if (err < 0)
> +   return err;
> +
> +   innolux->prepared = false;
> +
> +

Re: [PATCH v3 2/2] drm/panel: add innolux,p079zca panel driver

[Stéphane Marchesin]

can you add bps here too?

On Tue, Mar 21, 2017 at 6:53 PM, Chris Zhong  wrote:
> Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI
> panel.
>
> Signed-off-by: Chris Zhong 
> Reviewed-by: Sean Paul 
> Tested-by: Brian Norris 
> ---
>
> Changes in v3:
> - printk err after regulator_disable(innolux->supply)
>
> Changes in v2:
> - add some error check
> - always use Low power mode to send commend
> - add comments for all the sleep
> - use DRM_DEV_ERROR instead of dev_err
>
>  drivers/gpu/drm/panel/Kconfig |  11 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 345 
> ++
>  3 files changed, 357 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 62aba97..99dd010 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -18,6 +18,17 @@ config DRM_PANEL_SIMPLE
>   that it can be automatically turned off when the panel goes into a
>   low power state.
>
> +config DRM_PANEL_INNOLUX_P079ZCA
> +   tristate "Innolux P079ZCA panel"
> +   depends on OF
> +   depends on DRM_MIPI_DSI
> +   depends on BACKLIGHT_CLASS_DEVICE
> +   help
> + Say Y here if you want to enable support for Innolux P079ZCA
> + TFT-LCD modules. The panel has a 1024x768 resolution and uses
> + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> + the host and has a built-in LED backlight.
> +
>  config DRM_PANEL_JDI_LT070ME05000
> tristate "JDI LT070ME05000 WUXGA DSI panel"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index a5c7ec0..bda2aa4 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
> +obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
>  obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
> panel-panasonic-vvx10f034n00.o
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
> b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> new file mode 100644
> index 000..9f3423a0
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -0,0 +1,345 @@
> +/*
> + * Copyright (c) 2017, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct innolux_panel {
> +   struct drm_panel base;
> +   struct mipi_dsi_device *link;
> +
> +   struct backlight_device *backlight;
> +   struct regulator *supply;
> +   struct gpio_desc *enable_gpio;
> +
> +   bool prepared;
> +   bool enabled;
> +};
> +
> +static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel)
> +{
> +   return container_of(panel, struct innolux_panel, base);
> +}
> +
> +static int innolux_panel_disable(struct drm_panel *panel)
> +{
> +   struct innolux_panel *innolux = to_innolux_panel(panel);
> +   int err;
> +
> +   if (!innolux->enabled)
> +   return 0;
> +
> +   if (innolux->backlight) {
> +   innolux->backlight->props.power = FB_BLANK_POWERDOWN;
> +   backlight_update_status(innolux->backlight);
> +   }
> +
> +   err = mipi_dsi_dcs_set_display_off(innolux->link);
> +   if (err < 0)
> +   DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
> + err);
> +
> +   innolux->enabled = false;
> +
> +   return 0;
> +}
> +
> +static int innolux_panel_unprepare(struct drm_panel *panel)
> +{
> +   struct innolux_panel *innolux = to_innolux_panel(panel);
> +   int err;
> +
> +   if (!innolux->prepared)
> +   return 0;
> +
> +   err = mipi_dsi_dcs_enter_sleep_mode(innolux->link);
> +   if (err < 0) {
> +   DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
> + err);
> +   return err;
> +   }
> +
> +   gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> +
> +   /* T8: 80ms - 1000ms */
> +   msleep(80);
> +
> +   err = regulator_disable(innolux->supply);
> +   if (err < 0)
> +   return err;
> +
> +   innolux->prepared = false;
> +
> +   return 0;
> +}
> +
> +static int innolux_panel_prepare(struct drm_panel *panel)
> +{
> 

Re: [PATCH v3 2/2] drm/panel: simple: Add support BOE nv101wxmn51

[Stéphane Marchesin]

Reviewed-by: Stéphane Marchesin <marc...@chromium.org>


On Tue, Dec 13, 2016 at 7:19 PM, Caesar Wang <w...@rock-chips.com> wrote:
> 10.1WXGA is a color active matrix TFT LCD module using amorphous silicon
> TFT's as an active switching devices. It can be supported by the
> simple-panel driver.
>
> Read the panel default edid information:
>
> EDID MODE DETAILS
> name = 
> pixel_clock = 71900
> lvds_dual_channel = 0
> refresh = 0
> ha = 1280
> hbl = 160
> hso = 48
> hspw = 32
> hborder = 0
> va = 800
> vbl = 32
> vso = 3
> vspw = 5
> vborder = 0
> phsync = +
> pvsync = -
> x_mm = 0
> y_mm = 0
> drm_display_mode
> .hdisplay = 1280
> .hsync_start = 1328
> .hsync_end = 1360
> .htotal = 1440
> .vdisplay = 800
> .vsync_start = 803
> .vsync_end = 808
> .vtotal = 832
>
> There are two modes in the edid:
> Detailed mode1: Clock 71.900 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800  803  808  832 vborder 0
>+hsync -vsync
> Detailed mode2: Clock 57.500 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800  803  808  832 vborder 0
>+hsync -vsync
>
> Add the both edid to support more modes for BOE nv101wxmn51.
>
> Signed-off-by: Caesar Wang <w...@rock-chips.com>
> ---
>
> Changes in v3:
> - As Stéphane commented on https://patchwork.kernel.org/patch/9465911,
>   add downclock mode for edid.
>
> Changes in v2:
> - fix the vsync_start and vsync_end from the edid.
> - change the commit.
>
>  drivers/gpu/drm/panel/panel-simple.c | 45 
> 
>  1 file changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 06aaf79..1ce25b5 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -668,6 +668,48 @@ static const struct panel_desc avic_tm070ddh03 = {
> },
>  };
>
> +static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
> +   {
> +   .clock = 71900,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 60,
> +   },
> +   {
> +   .clock = 57500,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 48,
> +   },
> +};
> +
> +static const struct panel_desc boe_nv101wxmn51 = {
> +   .modes = boe_nv101wxmn51_modes,
> +   .num_modes = ARRAY_SIZE(boe_nv101wxmn51_modes),
> +   .bpc = 8,
> +   .size = {
> +   .width = 217,
> +   .height = 136,
> +   },
> +   .delay = {
> +   .prepare = 210,
> +   .enable = 50,
> +   .unprepare = 160,
> +   },
> +};
> +
>  static const struct drm_display_mode chunghwa_claa070wp03xg_mode = {
> .clock = 66770,
> .hdisplay = 800,
> @@ -1748,6 +1790,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "avic,tm070ddh03",
> .data = _tm070ddh03,
> }, {
> +   .compatible = "boe,nv101wxmn51",
> +   .data = _nv101wxmn51,
> +   }, {
> .compatible = "chunghwa,claa070wp03xg",
> .data = _claa070wp03xg,
> }, {
> --
> 2.7.4
>

Re: [PATCH v3 2/2] drm/panel: simple: Add support BOE nv101wxmn51

[Stéphane Marchesin]

Reviewed-by: Stéphane Marchesin 


On Tue, Dec 13, 2016 at 7:19 PM, Caesar Wang  wrote:
> 10.1WXGA is a color active matrix TFT LCD module using amorphous silicon
> TFT's as an active switching devices. It can be supported by the
> simple-panel driver.
>
> Read the panel default edid information:
>
> EDID MODE DETAILS
> name = 
> pixel_clock = 71900
> lvds_dual_channel = 0
> refresh = 0
> ha = 1280
> hbl = 160
> hso = 48
> hspw = 32
> hborder = 0
> va = 800
> vbl = 32
> vso = 3
> vspw = 5
> vborder = 0
> phsync = +
> pvsync = -
> x_mm = 0
> y_mm = 0
> drm_display_mode
> .hdisplay = 1280
> .hsync_start = 1328
> .hsync_end = 1360
> .htotal = 1440
> .vdisplay = 800
> .vsync_start = 803
> .vsync_end = 808
> .vtotal = 832
>
> There are two modes in the edid:
> Detailed mode1: Clock 71.900 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800  803  808  832 vborder 0
>+hsync -vsync
> Detailed mode2: Clock 57.500 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800  803  808  832 vborder 0
>+hsync -vsync
>
> Add the both edid to support more modes for BOE nv101wxmn51.
>
> Signed-off-by: Caesar Wang 
> ---
>
> Changes in v3:
> - As Stéphane commented on https://patchwork.kernel.org/patch/9465911,
>   add downclock mode for edid.
>
> Changes in v2:
> - fix the vsync_start and vsync_end from the edid.
> - change the commit.
>
>  drivers/gpu/drm/panel/panel-simple.c | 45 
> 
>  1 file changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 06aaf79..1ce25b5 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -668,6 +668,48 @@ static const struct panel_desc avic_tm070ddh03 = {
> },
>  };
>
> +static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
> +   {
> +   .clock = 71900,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 60,
> +   },
> +   {
> +   .clock = 57500,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 48,
> +   },
> +};
> +
> +static const struct panel_desc boe_nv101wxmn51 = {
> +   .modes = boe_nv101wxmn51_modes,
> +   .num_modes = ARRAY_SIZE(boe_nv101wxmn51_modes),
> +   .bpc = 8,
> +   .size = {
> +   .width = 217,
> +   .height = 136,
> +   },
> +   .delay = {
> +   .prepare = 210,
> +   .enable = 50,
> +   .unprepare = 160,
> +   },
> +};
> +
>  static const struct drm_display_mode chunghwa_claa070wp03xg_mode = {
> .clock = 66770,
> .hdisplay = 800,
> @@ -1748,6 +1790,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "avic,tm070ddh03",
> .data = _tm070ddh03,
> }, {
> +   .compatible = "boe,nv101wxmn51",
> +   .data = _nv101wxmn51,
> +   }, {
> .compatible = "chunghwa,claa070wp03xg",
> .data = _claa070wp03xg,
> }, {
> --
> 2.7.4
>

Re: [PATCH v2 2/2] drm/panel: simple: Add support BOE nv101wxmn51

[Stéphane Marchesin]

On Wed, Dec 7, 2016 at 11:26 PM, Caesar Wang  wrote:
> 10.1WXGA is a color active matrix TFT LCD module using amorphous silicon
> TFT's as an active switching devices. It can be supported by the
> simple-panel driver.
>
> Read the panel edid information;
>
> EDID MODE DETAILS
> name = 
> pixel_clock = 71900
> lvds_dual_channel = 0
> refresh = 0
> ha = 1280
> hbl = 160
> hso = 48
> hspw = 32
> hborder = 0
> va = 800
> vbl = 32
> vso = 3
> vspw = 5
> vborder = 0
> phsync = +
> pvsync = -
> x_mm = 0
> y_mm = 0
> drm_display_mode
> .hdisplay = 1280
> .hsync_start = 1328
> .hsync_end = 1360
> .htotal = 1440
> .vdisplay = 800
> .vsync_start = 803
> .vsync_end = 808
> .vtotal = 832
>
> Signed-off-by: Caesar Wang 
> ---
>
> Changes in v2:
> - fix the vsync_start and vsync_end from the edid.
> - change the commit.
>
>  drivers/gpu/drm/panel/panel-simple.c | 31 +++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 06aaf79..7c90f16 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -668,6 +668,34 @@ static const struct panel_desc avic_tm070ddh03 = {
> },
>  };
>
> +static const struct drm_display_mode boe_nv101wxmn51_mode = {
> +   .clock = 71900,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_desc boe_nv101wxmn51 = {
> +   .modes = _nv101wxmn51_mode,
> +   .num_modes = 1,

There are two modes in the EDID (there is a downclock one). Can you
add both modes?

Stéphane

> +   .bpc = 8,
> +   .size = {
> +   .width = 217,
> +   .height = 136,
> +   },
> +   .delay = {
> +   .prepare = 210,
> +   .enable = 50,
> +   .unprepare = 160,
> +   },
> +};
> +
>  static const struct drm_display_mode chunghwa_claa070wp03xg_mode = {
> .clock = 66770,
> .hdisplay = 800,
> @@ -1748,6 +1776,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "avic,tm070ddh03",
> .data = _tm070ddh03,
> }, {
> +   .compatible = "boe,nv101wxmn51",
> +   .data = _nv101wxmn51,
> +   }, {
> .compatible = "chunghwa,claa070wp03xg",
> .data = _claa070wp03xg,
> }, {
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/panel: simple: Add support BOE nv101wxmn51

[Stéphane Marchesin]

On Wed, Dec 7, 2016 at 11:26 PM, Caesar Wang  wrote:
> 10.1WXGA is a color active matrix TFT LCD module using amorphous silicon
> TFT's as an active switching devices. It can be supported by the
> simple-panel driver.
>
> Read the panel edid information;
>
> EDID MODE DETAILS
> name = 
> pixel_clock = 71900
> lvds_dual_channel = 0
> refresh = 0
> ha = 1280
> hbl = 160
> hso = 48
> hspw = 32
> hborder = 0
> va = 800
> vbl = 32
> vso = 3
> vspw = 5
> vborder = 0
> phsync = +
> pvsync = -
> x_mm = 0
> y_mm = 0
> drm_display_mode
> .hdisplay = 1280
> .hsync_start = 1328
> .hsync_end = 1360
> .htotal = 1440
> .vdisplay = 800
> .vsync_start = 803
> .vsync_end = 808
> .vtotal = 832
>
> Signed-off-by: Caesar Wang 
> ---
>
> Changes in v2:
> - fix the vsync_start and vsync_end from the edid.
> - change the commit.
>
>  drivers/gpu/drm/panel/panel-simple.c | 31 +++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 06aaf79..7c90f16 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -668,6 +668,34 @@ static const struct panel_desc avic_tm070ddh03 = {
> },
>  };
>
> +static const struct drm_display_mode boe_nv101wxmn51_mode = {
> +   .clock = 71900,
> +   .hdisplay = 1280,
> +   .hsync_start = 1280 + 48,
> +   .hsync_end = 1280 + 48 + 32,
> +   .htotal = 1280 + 48 + 32 + 80,
> +   .vdisplay = 800,
> +   .vsync_start = 800 + 3,
> +   .vsync_end = 800 + 3 + 5,
> +   .vtotal = 800 + 3 + 5 + 24,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_desc boe_nv101wxmn51 = {
> +   .modes = _nv101wxmn51_mode,
> +   .num_modes = 1,

There are two modes in the EDID (there is a downclock one). Can you
add both modes?

Stéphane

> +   .bpc = 8,
> +   .size = {
> +   .width = 217,
> +   .height = 136,
> +   },
> +   .delay = {
> +   .prepare = 210,
> +   .enable = 50,
> +   .unprepare = 160,
> +   },
> +};
> +
>  static const struct drm_display_mode chunghwa_claa070wp03xg_mode = {
> .clock = 66770,
> .hdisplay = 800,
> @@ -1748,6 +1776,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "avic,tm070ddh03",
> .data = _tm070ddh03,
> }, {
> +   .compatible = "boe,nv101wxmn51",
> +   .data = _nv101wxmn51,
> +   }, {
> .compatible = "chunghwa,claa070wp03xg",
> .data = _claa070wp03xg,
> }, {
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] dt-bindings: add Starry KR122EA0SRA panel binding

[Stéphane Marchesin]

On Fri, Jun 10, 2016 at 3:03 PM, Rob Clark  wrote:
> On Fri, Jun 10, 2016 at 3:52 PM, Doug Anderson  wrote:
>> Rob,
>>
>> On Fri, Jun 10, 2016 at 11:43 AM, Rob Clark  wrote:
>>> On Fri, Jun 10, 2016 at 1:02 PM, Douglas Anderson  
>>> wrote:
 The Starry KR122EA0SRA is a 12.2", 1920x1200 TFT-LCD panel connected
 using eDP interfaces.
>>>
>>> so drive-by comment... but shouldn't eDP be probe-able?  Not sure why
>>> we need panel drivers or DT bindings?
>>
>> I was wondering about that too.  As far as I can tell:
>>
>> 1. We need a panel driver because that appears to be what owns a
>> reference to the backlight / panel power regulator and that part is
>> not auto-probable.
>
> oh, hmm.. sad.. I was hoping that eDP would save us from dsi per-panel
> driver hell.. I guess being able to use panel-simple is at least an
> improvement.  But panel specific sequences is sounds like panel-simple
> won't save us all the time :-(

Yes, although you can probably make things mostly work with improper
sequencing and enough retries, a lot of ARM hw either requires
interesting sequencing, or has broken/unreliable DDC, which translates
into the use of panel simple on the sw side.

>
>> 2. As far as I could tell, there is no way to declare a generic
>> (unspecified) panel in the device tree.  Everyone seems to include
>> "simple-panel" in their compatible string but as far as I can tell
>> nothing in the kernel looks at it.
>>
>> 3. In theory, all the info specified here should match the EDID
>> exactly and thus (as you said) be probable.  However, it sounds like
>> (for power sequencing reasons) there might be reasons why you'd want
>> to know exactly what panel was present beforehand.  You might need to
>> power the panel and backlight in very specific sequences, for
>> instance.  I'm not sure it's always 100% possible in all embedded
>> designs to read the EDID before you know how the sequencing should
>> work (but, of course, I'm a NOOB).
>>
>> 4. Reading the EDID can be slow.  If you happen to know all the info
>> on the panel beforehand you can significantly speed up boot speed,
>> notably how fast you can get something on the screen.
>
> The theory is (although I think not true currently for most of the arm
> drivers) that we should be reading back from hw the current config
> from bootloader splash screen, to avoid a modeset (and conveniently
> also the need to read edid) at boot.

On some devices the firmware doesn't set any video mode, so there
isn't anything we can read back. That is our case :)

Stéphane


>
> BR,
> -R
>
>>
>> Anyway, maybe someone else who actually knows what they're talking
>> about will chime in.  ;)
>>
>> -Doug

Re: [PATCH v2 1/2] dt-bindings: add Starry KR122EA0SRA panel binding

[Stéphane Marchesin]

On Fri, Jun 10, 2016 at 3:03 PM, Rob Clark  wrote:
> On Fri, Jun 10, 2016 at 3:52 PM, Doug Anderson  wrote:
>> Rob,
>>
>> On Fri, Jun 10, 2016 at 11:43 AM, Rob Clark  wrote:
>>> On Fri, Jun 10, 2016 at 1:02 PM, Douglas Anderson  
>>> wrote:
 The Starry KR122EA0SRA is a 12.2", 1920x1200 TFT-LCD panel connected
 using eDP interfaces.
>>>
>>> so drive-by comment... but shouldn't eDP be probe-able?  Not sure why
>>> we need panel drivers or DT bindings?
>>
>> I was wondering about that too.  As far as I can tell:
>>
>> 1. We need a panel driver because that appears to be what owns a
>> reference to the backlight / panel power regulator and that part is
>> not auto-probable.
>
> oh, hmm.. sad.. I was hoping that eDP would save us from dsi per-panel
> driver hell.. I guess being able to use panel-simple is at least an
> improvement.  But panel specific sequences is sounds like panel-simple
> won't save us all the time :-(

Yes, although you can probably make things mostly work with improper
sequencing and enough retries, a lot of ARM hw either requires
interesting sequencing, or has broken/unreliable DDC, which translates
into the use of panel simple on the sw side.

>
>> 2. As far as I could tell, there is no way to declare a generic
>> (unspecified) panel in the device tree.  Everyone seems to include
>> "simple-panel" in their compatible string but as far as I can tell
>> nothing in the kernel looks at it.
>>
>> 3. In theory, all the info specified here should match the EDID
>> exactly and thus (as you said) be probable.  However, it sounds like
>> (for power sequencing reasons) there might be reasons why you'd want
>> to know exactly what panel was present beforehand.  You might need to
>> power the panel and backlight in very specific sequences, for
>> instance.  I'm not sure it's always 100% possible in all embedded
>> designs to read the EDID before you know how the sequencing should
>> work (but, of course, I'm a NOOB).
>>
>> 4. Reading the EDID can be slow.  If you happen to know all the info
>> on the panel beforehand you can significantly speed up boot speed,
>> notably how fast you can get something on the screen.
>
> The theory is (although I think not true currently for most of the arm
> drivers) that we should be reading back from hw the current config
> from bootloader splash screen, to avoid a modeset (and conveniently
> also the need to read edid) at boot.

On some devices the firmware doesn't set any video mode, so there
isn't anything we can read back. That is our case :)

Stéphane


>
> BR,
> -R
>
>>
>> Anyway, maybe someone else who actually knows what they're talking
>> about will chime in.  ;)
>>
>> -Doug

Re: [PATCH v2 4/6] drm/panel: simple: Add support for Samsung LSN122DL01-C01 2560x1600 panel

[Stéphane Marchesin]

On Wed, Jun 8, 2016 at 4:52 AM, Yakir Yang  wrote:
> The Samsung LSN122DL01-C01 is an 12.2" 2560x1600 (WQXGA) TFT-LCD panel
> connected using eDP interfaces.
>
> Signed-off-by: Yakir Yang 
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/panel/panel-simple.c | 25 +
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 41020e1..067a5c4 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1246,6 +1246,28 @@ static const struct panel_desc qd43003c0_40 = {
> .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>  };
>
> +static const struct drm_display_mode samsung_lsn122dl01_c01_mode = {
> +   .clock = 271560,
> +   .hdisplay = 2560,
> +   .hsync_start = 2560 + 48,
> +   .hsync_end = 2560 + 48 + 32,
> +   .htotal = 2560 + 48 + 32 + 80,
> +   .vdisplay = 1600,
> +   .vsync_start = 1600 + 2,
> +   .vsync_end = 1600 + 2 + 5,
> +   .vtotal = 1600 + 2 + 5 + 57,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_desc samsung_lsn122dl01_c01 = {
> +   .modes = _lsn122dl01_c01_mode,
> +   .num_modes = 1,
> +   .size = {
> +   .width = 2560,
> +   .height = 1600,

These are meant to be the physical dimensions (same thing for the
other patches btw).

Stéphane

> +   },
> +};
> +
>  static const struct drm_display_mode samsung_ltn101nt05_mode = {
> .clock = 54030,
> .hdisplay = 1024,
> @@ -1506,6 +1528,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "qiaodian,qd43003c0-40",
> .data = _40,
> }, {
> +   .compatible = "samsung,lsn122dl01-c01",
> +   .data = _lsn122dl01_c01,
> +   }, {
> .compatible = "samsung,ltn101nt05",
> .data = _ltn101nt05,
> }, {
> --
> 1.9.1
>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/panel: simple: Add support for Samsung LSN122DL01-C01 2560x1600 panel

[Stéphane Marchesin]

On Wed, Jun 8, 2016 at 4:52 AM, Yakir Yang  wrote:
> The Samsung LSN122DL01-C01 is an 12.2" 2560x1600 (WQXGA) TFT-LCD panel
> connected using eDP interfaces.
>
> Signed-off-by: Yakir Yang 
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/panel/panel-simple.c | 25 +
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 41020e1..067a5c4 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1246,6 +1246,28 @@ static const struct panel_desc qd43003c0_40 = {
> .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>  };
>
> +static const struct drm_display_mode samsung_lsn122dl01_c01_mode = {
> +   .clock = 271560,
> +   .hdisplay = 2560,
> +   .hsync_start = 2560 + 48,
> +   .hsync_end = 2560 + 48 + 32,
> +   .htotal = 2560 + 48 + 32 + 80,
> +   .vdisplay = 1600,
> +   .vsync_start = 1600 + 2,
> +   .vsync_end = 1600 + 2 + 5,
> +   .vtotal = 1600 + 2 + 5 + 57,
> +   .vrefresh = 60,
> +};
> +
> +static const struct panel_desc samsung_lsn122dl01_c01 = {
> +   .modes = _lsn122dl01_c01_mode,
> +   .num_modes = 1,
> +   .size = {
> +   .width = 2560,
> +   .height = 1600,

These are meant to be the physical dimensions (same thing for the
other patches btw).

Stéphane

> +   },
> +};
> +
>  static const struct drm_display_mode samsung_ltn101nt05_mode = {
> .clock = 54030,
> .hdisplay = 1024,
> @@ -1506,6 +1528,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "qiaodian,qd43003c0-40",
> .data = _40,
> }, {
> +   .compatible = "samsung,lsn122dl01-c01",
> +   .data = _lsn122dl01_c01,
> +   }, {
> .compatible = "samsung,ltn101nt05",
> .data = _ltn101nt05,
> }, {
> --
> 1.9.1
>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP

[Stéphane Marchesin]

On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
 wrote:
> On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>>
>>
>> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
>> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
>> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you 
>> >>can
>> >>identify both lane count and reversal state without touching anything in 
>> >>the
>> >>link training code. i am yet to upstream my changes for CHT that i can 
>> >>share
>> >>if required that does the same in intel_dp_detect without touching any line
>> >>in link training path.
>> >With my current limited knowledge of the dp hotplug (and i915 driver) I
>> >am not sure we could detect the reversed state without trying to train 1
>> >lane only. I'd be glad to look at your changes and test them on my
>> >system if you think that could help having a cleaner solution.
>> >
>> >Cheers,
>> >Benjamin
>> No, what i recommended was to do link training but in intel_dp_detect. Since
>> USB Type C cable
>> also has its own lane count restriction (it can have different lane count
>> than the one supported
>> by panel) you might have to figure that out as well. so both reversal and
>> lane count detection
>> can be done outside the modeset path and keep the code free of type C
>> changes outside
>> detection path.
>>
>> Please find below the code to do the same. Do not waste time trying to apply
>> this directly on
>> nightly since this is based on a local tree and because this is pre- atomic
>> changes code, so you
>> might have to modify chv_upfront_link_train to work on top of the latest
>> nightly code. we
>> are supposed to upstream this and is in my todo list.
>>
>
> [original patch snipped...]
>
> Hi Sivakumar,
>
> So I managed to manually re-apply your patch on top of
> drm-intel-nightly, and tried to port it to make Broadwell working too.
> It works OK if the system is already boot without any external DP used.
> In this case, the detection works and I can see my external monitor
> working properly.
>
> However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
> understand why. I think I enabled all that is mentioned in the PRM to be
> able to train the DP link, but I am obviously missing something else.
> Can you have a look?
>

Hi Benjamin,

I would recommend against this approach. Some adapters will claim that
they recovered a clock even when it isn't on the lanes you enabled,
which means that the reversal detection doesn't always work. The only
reliable way to do this is to go talk to the Chrome OS EC (you can
find these patches later in the Chrome OS tree). It's not as generic,
but we might be able to abstract that logic, maybe.

Stéphane



> My patch is now:
>
> From 11e9c42f55ae27bd6e7b0168bf24d15082c7d517 Mon Sep 17 00:00:00 2001
> From: Durgadoss R 
> Date: Mon, 3 Aug 2015 11:51:16 -0400
> Subject: [PATCH] drm/i915: Enable Upfront link training for type-C DP support
>
> To support USB type C alternate DP mode, the display driver needs to know the
> number of lanes required by DP panel as well as number of lanes that can be
> supported by the type-C cable. Sometimes, the type-C cable may limit the
> bandwidth even if Panel can support more lanes. To address these scenarios,
> the display driver will start link training with max lanes, and if the link
> training fails, the driver then falls back to x2 lanes.
>
> * Since link training is done before modeset, planes are not enabled. Only
>   encoder and the its associated PLLs are enabled.
> * Once link training is done, the encoder and its PLLs are disabled; so that
>   the subsequent modeset is not aware of these changes.
> * As of now, this is tested only on CHV.
>
> Signed-off-by: Durgadoss R 
> [BT: add broadwell support and USB-C reverted state detection]
> Signed-off-by: Benjamin Tissoires 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 163 
> +++
>  drivers/gpu/drm/i915/intel_dp.c  |  22 -
>  drivers/gpu/drm/i915/intel_drv.h |   7 ++
>  3 files changed, 190 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 818f3a3..e8ddba0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15799,3 +15799,166 @@ void intel_modeset_preclose(struct drm_device *dev, 
> struct drm_file *file)
> spin_unlock_irq(>event_lock);
> }
>  }
> +
> +static bool
> +intel_upfront_link_train_config(struct drm_device *dev,
> +   struct intel_dp *intel_dp,
> +   struct intel_crtc *crtc,
> +   uint8_t link_bw,
> +   uint8_t lane_count)
> +{
> +   struct drm_i915_private *dev_priv = dev->dev_private;
> +   struct intel_connector *connector = intel_dp->attached_connector;
> +   struct intel_encoder *encoder = 

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP

[Stéphane Marchesin]

On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:


 On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
 On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
 why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you 
 can
 identify both lane count and reversal state without touching anything in 
 the
 link training code. i am yet to upstream my changes for CHT that i can 
 share
 if required that does the same in intel_dp_detect without touching any line
 in link training path.
 With my current limited knowledge of the dp hotplug (and i915 driver) I
 am not sure we could detect the reversed state without trying to train 1
 lane only. I'd be glad to look at your changes and test them on my
 system if you think that could help having a cleaner solution.
 
 Cheers,
 Benjamin
 No, what i recommended was to do link training but in intel_dp_detect. Since
 USB Type C cable
 also has its own lane count restriction (it can have different lane count
 than the one supported
 by panel) you might have to figure that out as well. so both reversal and
 lane count detection
 can be done outside the modeset path and keep the code free of type C
 changes outside
 detection path.

 Please find below the code to do the same. Do not waste time trying to apply
 this directly on
 nightly since this is based on a local tree and because this is pre- atomic
 changes code, so you
 might have to modify chv_upfront_link_train to work on top of the latest
 nightly code. we
 are supposed to upstream this and is in my todo list.


 [original patch snipped...]

 Hi Sivakumar,

 So I managed to manually re-apply your patch on top of
 drm-intel-nightly, and tried to port it to make Broadwell working too.
 It works OK if the system is already boot without any external DP used.
 In this case, the detection works and I can see my external monitor
 working properly.

 However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
 understand why. I think I enabled all that is mentioned in the PRM to be
 able to train the DP link, but I am obviously missing something else.
 Can you have a look?


Hi Benjamin,

I would recommend against this approach. Some adapters will claim that
they recovered a clock even when it isn't on the lanes you enabled,
which means that the reversal detection doesn't always work. The only
reliable way to do this is to go talk to the Chrome OS EC (you can
find these patches later in the Chrome OS tree). It's not as generic,
but we might be able to abstract that logic, maybe.

Stéphane



 My patch is now:

 From 11e9c42f55ae27bd6e7b0168bf24d15082c7d517 Mon Sep 17 00:00:00 2001
 From: Durgadoss R durgados...@intel.com
 Date: Mon, 3 Aug 2015 11:51:16 -0400
 Subject: [PATCH] drm/i915: Enable Upfront link training for type-C DP support

 To support USB type C alternate DP mode, the display driver needs to know the
 number of lanes required by DP panel as well as number of lanes that can be
 supported by the type-C cable. Sometimes, the type-C cable may limit the
 bandwidth even if Panel can support more lanes. To address these scenarios,
 the display driver will start link training with max lanes, and if the link
 training fails, the driver then falls back to x2 lanes.

 * Since link training is done before modeset, planes are not enabled. Only
   encoder and the its associated PLLs are enabled.
 * Once link training is done, the encoder and its PLLs are disabled; so that
   the subsequent modeset is not aware of these changes.
 * As of now, this is tested only on CHV.

 Signed-off-by: Durgadoss R durgados...@intel.com
 [BT: add broadwell support and USB-C reverted state detection]
 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 163 
 +++
  drivers/gpu/drm/i915/intel_dp.c  |  22 -
  drivers/gpu/drm/i915/intel_drv.h |   7 ++
  3 files changed, 190 insertions(+), 2 deletions(-)

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 818f3a3..e8ddba0 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -15799,3 +15799,166 @@ void intel_modeset_preclose(struct drm_device *dev, 
 struct drm_file *file)
 spin_unlock_irq(dev-event_lock);
 }
  }
 +
 +static bool
 +intel_upfront_link_train_config(struct drm_device *dev,
 +   struct intel_dp *intel_dp,
 +   struct intel_crtc *crtc,
 +   uint8_t link_bw,
 +   uint8_t lane_count)
 +{
 +   struct drm_i915_private *dev_priv = dev-dev_private;
 +   struct intel_connector *connector = intel_dp-attached_connector;
 +   struct intel_encoder *encoder = connector-encoder;
 +   bool success;
 +
 +   intel_dp-link_bw = 

Re: [Nouveau] [PATCH v2 2/3] drm/ttm: introduce dma cache sync helpers

[Stéphane Marchesin]

On Tue, Jun 24, 2014 at 6:25 AM, Lucas Stach  wrote:
> Am Dienstag, den 24.06.2014, 14:27 +0200 schrieb Maarten Lankhorst:
>> op 24-06-14 14:23, Alexandre Courbot schreef:
>> > On Tue, Jun 24, 2014 at 7:55 PM, Alexandre Courbot  
>> > wrote:
>> >> On 06/24/2014 07:33 PM, Alexandre Courbot wrote:
>> >>> On 06/24/2014 07:02 PM, Russell King - ARM Linux wrote:
>>  On Tue, Jun 24, 2014 at 06:54:26PM +0900, Alexandre Courbot wrote:
>> > From: Lucas Stach 
>> >
>> > On architectures for which access to GPU memory is non-coherent,
>> > caches need to be flushed and invalidated explicitly at the
>> > appropriate places. Introduce two small helpers to make things
>> > easy for TTM-based drivers.
>> 
>>  Have you run this with DMA API debugging enabled?  I suspect you 
>>  haven't,
>>  and I recommend that you do.
>> >>>
>> >>> # cat /sys/kernel/debug/dma-api/error_count
>> >>> 162621
>> >>>
>> >>> (╯°□°）╯︵ ┻━┻)
>> >>
>> >> *puts table back on its feet*
>> >>
>> >> So, yeah - TTM memory is not allocated using the DMA API, hence we cannot
>> >> use the DMA API to sync it. Thanks Russell for pointing it out.
>> >>
>> >> The only alternative I see here is to flush the CPU caches when syncing 
>> >> for
>> >> the device, and invalidate them for the other direction. Of course if the
>> >> device has caches on its side as well the opposite operation must also be
>> >> done for it. Guess the only way is to handle it all by ourselves here. :/
>> > ... and it really sucks. Basically if we cannot use the DMA API here
>> > we will lose the convenience of having a portable API that does just
>> > the right thing for the underlying platform. Without it we would have
>> > to duplicate arm_iommu_sync_single_for_cpu/device() and we would only
>> > have support for ARM.
>> >
>> > The usage of the DMA API that we are doing might be illegal, but in
>> > essence it does exactly what we need - at least for ARM. What are the
>> > alternatives?
>> Convert TTM to use the dma api? :-)
>
> Actually TTM already has a page alloc backend using the DMA API. It's
> just not used for the standard case right now.
>
> I would argue that we should just use this page allocator (which has the
> side effect of getting pages from CMA if available -> you are actually
> free to change the caching) and do away with the other allocator in the
> ARM case.

CMA comes with its own set of (severe) limitations though, in
particular it's not possible to map arbitrary CPU pages into the GPU
without incurring a copy, you add arbitrary memory limits etc. Overall
that's not really a good pick for the long term...

Stéphane
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Nouveau] [PATCH v2 2/3] drm/ttm: introduce dma cache sync helpers

[Stéphane Marchesin]

On Tue, Jun 24, 2014 at 6:25 AM, Lucas Stach l.st...@pengutronix.de wrote:
 Am Dienstag, den 24.06.2014, 14:27 +0200 schrieb Maarten Lankhorst:
 op 24-06-14 14:23, Alexandre Courbot schreef:
  On Tue, Jun 24, 2014 at 7:55 PM, Alexandre Courbot acour...@nvidia.com 
  wrote:
  On 06/24/2014 07:33 PM, Alexandre Courbot wrote:
  On 06/24/2014 07:02 PM, Russell King - ARM Linux wrote:
  On Tue, Jun 24, 2014 at 06:54:26PM +0900, Alexandre Courbot wrote:
  From: Lucas Stach d...@lynxeye.de
 
  On architectures for which access to GPU memory is non-coherent,
  caches need to be flushed and invalidated explicitly at the
  appropriate places. Introduce two small helpers to make things
  easy for TTM-based drivers.
 
  Have you run this with DMA API debugging enabled?  I suspect you 
  haven't,
  and I recommend that you do.
 
  # cat /sys/kernel/debug/dma-api/error_count
  162621
 
  (╯°□°）╯︵ ┻━┻)
 
  *puts table back on its feet*
 
  So, yeah - TTM memory is not allocated using the DMA API, hence we cannot
  use the DMA API to sync it. Thanks Russell for pointing it out.
 
  The only alternative I see here is to flush the CPU caches when syncing 
  for
  the device, and invalidate them for the other direction. Of course if the
  device has caches on its side as well the opposite operation must also be
  done for it. Guess the only way is to handle it all by ourselves here. :/
  ... and it really sucks. Basically if we cannot use the DMA API here
  we will lose the convenience of having a portable API that does just
  the right thing for the underlying platform. Without it we would have
  to duplicate arm_iommu_sync_single_for_cpu/device() and we would only
  have support for ARM.
 
  The usage of the DMA API that we are doing might be illegal, but in
  essence it does exactly what we need - at least for ARM. What are the
  alternatives?
 Convert TTM to use the dma api? :-)

 Actually TTM already has a page alloc backend using the DMA API. It's
 just not used for the standard case right now.

 I would argue that we should just use this page allocator (which has the
 side effect of getting pages from CMA if available - you are actually
 free to change the caching) and do away with the other allocator in the
 ARM case.

CMA comes with its own set of (severe) limitations though, in
particular it's not possible to map arbitrary CPU pages into the GPU
without incurring a copy, you add arbitrary memory limits etc. Overall
that's not really a good pick for the long term...

Stéphane
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stéphane Marchesin]

On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
 wrote:
> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>> >On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>> >>On 06/16/2014 10:02 PM, Stephen Warren wrote:
>> >>>On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>> +#ifdef CONFIG_TEGRA124_EMC
>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>> long rate);
>> +void tegra124_emc_set_floor(unsigned long freq);
>> +void tegra124_emc_set_ceiling(unsigned long freq);
>> +#else
>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>> long rate)
>> +{ return -ENODEV; }
>> +void tegra124_emc_set_floor(unsigned long freq)
>> +{ return; }
>> +void tegra124_emc_set_ceiling(unsigned long freq)
>> +{ return; }
>> +#endif
>> >>>
>> >>>I'll repeat what I said off-list so that we can have the whole
>> >>>conversation on the list:
>> >>>
>> >>>That looks like a custom Tegra-specific API. I think it'd be much better
>> >>>to integrate this into the common clock framework as a standard clock
>> >>>constraints API. There are other use-cases for clock constraints besides
>> >>>EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>> >>>SoCs too).
>> >>
>> >>Yes, I wrote a bit in the cover letter about our requirements and how
>> >>they map to the CCF. Could you please comment on that?
>> >
>> >My comments remain the same. I believe this is something that belongs in
>> >the clock driver, or at the least, some API that takes a struct clock as
>> >its parameter, so that drivers can use the existing DT clock lookup
>> >mechanism.
>>
>> Ok, let me put this strawman here to see if I have gotten close to what you
>> have in mind:
>>
>> * add per-client accounting (Rabin's patches referenced before)
>>
>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>
>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>
>> * an EMC driver would collect bandwidth and latency requests from consumers
>> and call clk_set_floor on the EMC clock.
>>
>> * the EMC driver would also register for rate change notifications in the
>> EMC clock and would update the latency allowance registers at that point.
>
> Latency allowance registers are part of the MC rather than the EMC. So I
> think we have two options: a) have a unified driver for MC and EMC or b)
> provide two parts of the API in two drivers.
>
> Or perhaps c), create a generic framework that both MC and EMC can
> register with (bandwidth for EMC, latency for MC).

Is there any motivation for keeping MC and EMC separate? In my mind,
the solution was always to handle those together.

Stéphane
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stéphane Marchesin]

On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
thierry.red...@gmail.com wrote:
 On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
 On 06/17/2014 06:15 PM, Stephen Warren wrote:
 On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
 On 06/16/2014 10:02 PM, Stephen Warren wrote:
 On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
 +#ifdef CONFIG_TEGRA124_EMC
 +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
 long rate);
 +void tegra124_emc_set_floor(unsigned long freq);
 +void tegra124_emc_set_ceiling(unsigned long freq);
 +#else
 +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
 long rate)
 +{ return -ENODEV; }
 +void tegra124_emc_set_floor(unsigned long freq)
 +{ return; }
 +void tegra124_emc_set_ceiling(unsigned long freq)
 +{ return; }
 +#endif
 
 I'll repeat what I said off-list so that we can have the whole
 conversation on the list:
 
 That looks like a custom Tegra-specific API. I think it'd be much better
 to integrate this into the common clock framework as a standard clock
 constraints API. There are other use-cases for clock constraints besides
 EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
 SoCs too).
 
 Yes, I wrote a bit in the cover letter about our requirements and how
 they map to the CCF. Could you please comment on that?
 
 My comments remain the same. I believe this is something that belongs in
 the clock driver, or at the least, some API that takes a struct clock as
 its parameter, so that drivers can use the existing DT clock lookup
 mechanism.

 Ok, let me put this strawman here to see if I have gotten close to what you
 have in mind:

 * add per-client accounting (Rabin's patches referenced before)

 * add clk_set_floor, to be used by cpufreq, load stats, etc.

 * add clk_set_ceiling, to be used by battery drivers, thermal, etc.

 * an EMC driver would collect bandwidth and latency requests from consumers
 and call clk_set_floor on the EMC clock.

 * the EMC driver would also register for rate change notifications in the
 EMC clock and would update the latency allowance registers at that point.

 Latency allowance registers are part of the MC rather than the EMC. So I
 think we have two options: a) have a unified driver for MC and EMC or b)
 provide two parts of the API in two drivers.

 Or perhaps c), create a generic framework that both MC and EMC can
 register with (bandwidth for EMC, latency for MC).

Is there any motivation for keeping MC and EMC separate? In my mind,
the solution was always to handle those together.

Stéphane
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Nouveau] [RFC] drm/nouveau: disable caching for VRAM BOs on ARM

[Stéphane Marchesin]

On Mon, May 26, 2014 at 7:42 PM, Alexandre Courbot  wrote:
> On Tue, May 27, 2014 at 10:07 AM, Stéphane Marchesin
>  wrote:
>> On Mon, May 26, 2014 at 5:02 PM, Alexandre Courbot  wrote:
>>> On Mon, May 26, 2014 at 6:21 PM, Lucas Stach  wrote:
>>>> Am Montag, den 26.05.2014, 09:45 +0300 schrieb Terje Bergström:
>>>>> On 23.05.2014 17:40, Alex Courbot wrote:
>>>>> > On 05/23/2014 06:59 PM, Lucas Stach wrote:
>>>>> > So after checking with more knowledgeable people, it turns out this is
>>>>> > the expected behavior on ARM and BAR regions should be mapped uncached
>>>>> > on GK20A. All the more reasons to avoid using the BAR at all.
>>>>>
>>>>> This is actually specific to Tegra.
>>>>>
>>>>> >> You may want to make yourself aware of all the quirks required for
>>>>> >> sharing memory between the GPU and CPU on an ARM host. I think there 
>>>>> >> are
>>>>> >> far more involved than what you see now and writing an replacement for
>>>>> >> TTM will not be an easy task.
>>>>> >>
>>>>> >> Doing away with the concept of two memory areas will not get you to a
>>>>> >> single unified address space. You would have to deal with things like
>>>>> >> not being able to change the caching state of pages in the systems
>>>>> >> lowmem yourself. You will still have to deal with remapping pages that
>>>>> >> aren't currently visible to the CPU (ok this is not an issue on Jetson
>>>>> >> right now as it only has 2GB of RAM), because it's in systems highmem,
>>>>> >> or even in a different LPAE area.
>>>>> >>
>>>>> >> You really want to be sure you are aware of all the consequences of
>>>>> >> this, before considering this task.
>>>>> >
>>>>> > Yep, that's why I am seeking advice here. My first hope is that with a
>>>>> > few tweaks we will be able to keep using TTM and the current nouveau_bo
>>>>> > implementation. But unless I missed something this is not going to be 
>>>>> > easy.
>>>>> >
>>>>> > We can also use something like the patch I originally sent to make it
>>>>> > work, although not with good performance, on GK20A. Not very graceful,
>>>>> > but it will allow applications to run.
>>>>> >
>>>>> > In the long run though, we will want to achieve better performance, and
>>>>> > it seems like a BO implementation targeted at UMA devices would also be
>>>>> > beneficial to quite a few desktop GPUs. So as tricky as it may be I'm
>>>>> > interested in gathering thoughts and why not giving it a first try with
>>>>> > GK20A, even if it imposes some limitations like having buffers in lowmem
>>>>> > in a first time (we can probably live with this one for a short while,
>>>>> > and 64 bits will also be coming to the rescue :))
>>>>>
>>>>> I don't think lowmem or LPAE is any problem, if the memory manager is
>>>>> designed with that in mind. Vast majority of the buffers kernel
>>>>> allocates do not need to be touched in kernel space.
>>>>>
>>>>> Actually I can't think of any buffers that we allocate on behalf of user
>>>>> space that would need to be permanently mapped also to kernel. In case
>>>>> or relocs only push buffer needs to be temporarily mapped to kernel.
>>>>>
>>>>> Ultimately even relocs are not necessary if we expose GPU virtual
>>>>> addresses directly to user space. But that's another topic.
>>>>>
>>>> Nouveau already exposes constant virtual addresses to userspace and
>>>> skips the pushbuf patching when the presumed offset from userspace is
>>>> the same as what the kernel thinks it should be.
>>>>
>>>> The problem with lowmem on ARM is that you can't unmap those pages from
>>>> the kernel cached mapping. So if you alloc a page, give it to userspace
>>>> and userspace decides to map the page WC you just produced a conflicting
>>>> mapping, which may yield undefined results on ARMv7. You may think this
>>>> is not a problem as you are not touching the kernel cached mapping, but
>>>> in fact it is. The CPUs prefetch

Re: [Nouveau] [RFC] drm/nouveau: disable caching for VRAM BOs on ARM

[Stéphane Marchesin]

On Mon, May 26, 2014 at 5:02 PM, Alexandre Courbot  wrote:
> On Mon, May 26, 2014 at 6:21 PM, Lucas Stach  wrote:
>> Am Montag, den 26.05.2014, 09:45 +0300 schrieb Terje Bergström:
>>> On 23.05.2014 17:40, Alex Courbot wrote:
>>> > On 05/23/2014 06:59 PM, Lucas Stach wrote:
>>> > So after checking with more knowledgeable people, it turns out this is
>>> > the expected behavior on ARM and BAR regions should be mapped uncached
>>> > on GK20A. All the more reasons to avoid using the BAR at all.
>>>
>>> This is actually specific to Tegra.
>>>
>>> >> You may want to make yourself aware of all the quirks required for
>>> >> sharing memory between the GPU and CPU on an ARM host. I think there are
>>> >> far more involved than what you see now and writing an replacement for
>>> >> TTM will not be an easy task.
>>> >>
>>> >> Doing away with the concept of two memory areas will not get you to a
>>> >> single unified address space. You would have to deal with things like
>>> >> not being able to change the caching state of pages in the systems
>>> >> lowmem yourself. You will still have to deal with remapping pages that
>>> >> aren't currently visible to the CPU (ok this is not an issue on Jetson
>>> >> right now as it only has 2GB of RAM), because it's in systems highmem,
>>> >> or even in a different LPAE area.
>>> >>
>>> >> You really want to be sure you are aware of all the consequences of
>>> >> this, before considering this task.
>>> >
>>> > Yep, that's why I am seeking advice here. My first hope is that with a
>>> > few tweaks we will be able to keep using TTM and the current nouveau_bo
>>> > implementation. But unless I missed something this is not going to be 
>>> > easy.
>>> >
>>> > We can also use something like the patch I originally sent to make it
>>> > work, although not with good performance, on GK20A. Not very graceful,
>>> > but it will allow applications to run.
>>> >
>>> > In the long run though, we will want to achieve better performance, and
>>> > it seems like a BO implementation targeted at UMA devices would also be
>>> > beneficial to quite a few desktop GPUs. So as tricky as it may be I'm
>>> > interested in gathering thoughts and why not giving it a first try with
>>> > GK20A, even if it imposes some limitations like having buffers in lowmem
>>> > in a first time (we can probably live with this one for a short while,
>>> > and 64 bits will also be coming to the rescue :))
>>>
>>> I don't think lowmem or LPAE is any problem, if the memory manager is
>>> designed with that in mind. Vast majority of the buffers kernel
>>> allocates do not need to be touched in kernel space.
>>>
>>> Actually I can't think of any buffers that we allocate on behalf of user
>>> space that would need to be permanently mapped also to kernel. In case
>>> or relocs only push buffer needs to be temporarily mapped to kernel.
>>>
>>> Ultimately even relocs are not necessary if we expose GPU virtual
>>> addresses directly to user space. But that's another topic.
>>>
>> Nouveau already exposes constant virtual addresses to userspace and
>> skips the pushbuf patching when the presumed offset from userspace is
>> the same as what the kernel thinks it should be.
>>
>> The problem with lowmem on ARM is that you can't unmap those pages from
>> the kernel cached mapping. So if you alloc a page, give it to userspace
>> and userspace decides to map the page WC you just produced a conflicting
>> mapping, which may yield undefined results on ARMv7. You may think this
>> is not a problem as you are not touching the kernel cached mapping, but
>> in fact it is. The CPUs prefetcher can still access this mapping.
>
> Why would this memory be mapped into the kernel?

On ARM the kernel keeps a linear mapping of lowmem using sections
(ARM's version of huge pages). This is always cached, and because the
sections are not 4k, it's a pain to remove parts of it. See
arch/arm/mm/mmu.c

That said, I don't think this issue exists on A15 (which is what those
GPUs are paired with), so it's a purely theoretical problem.

Stéphane
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Nouveau] [RFC] drm/nouveau: disable caching for VRAM BOs on ARM

[Stéphane Marchesin]

On Mon, May 26, 2014 at 5:02 PM, Alexandre Courbot gnu...@gmail.com wrote:
 On Mon, May 26, 2014 at 6:21 PM, Lucas Stach l.st...@pengutronix.de wrote:
 Am Montag, den 26.05.2014, 09:45 +0300 schrieb Terje Bergström:
 On 23.05.2014 17:40, Alex Courbot wrote:
  On 05/23/2014 06:59 PM, Lucas Stach wrote:
  So after checking with more knowledgeable people, it turns out this is
  the expected behavior on ARM and BAR regions should be mapped uncached
  on GK20A. All the more reasons to avoid using the BAR at all.

 This is actually specific to Tegra.

  You may want to make yourself aware of all the quirks required for
  sharing memory between the GPU and CPU on an ARM host. I think there are
  far more involved than what you see now and writing an replacement for
  TTM will not be an easy task.
 
  Doing away with the concept of two memory areas will not get you to a
  single unified address space. You would have to deal with things like
  not being able to change the caching state of pages in the systems
  lowmem yourself. You will still have to deal with remapping pages that
  aren't currently visible to the CPU (ok this is not an issue on Jetson
  right now as it only has 2GB of RAM), because it's in systems highmem,
  or even in a different LPAE area.
 
  You really want to be sure you are aware of all the consequences of
  this, before considering this task.
 
  Yep, that's why I am seeking advice here. My first hope is that with a
  few tweaks we will be able to keep using TTM and the current nouveau_bo
  implementation. But unless I missed something this is not going to be 
  easy.
 
  We can also use something like the patch I originally sent to make it
  work, although not with good performance, on GK20A. Not very graceful,
  but it will allow applications to run.
 
  In the long run though, we will want to achieve better performance, and
  it seems like a BO implementation targeted at UMA devices would also be
  beneficial to quite a few desktop GPUs. So as tricky as it may be I'm
  interested in gathering thoughts and why not giving it a first try with
  GK20A, even if it imposes some limitations like having buffers in lowmem
  in a first time (we can probably live with this one for a short while,
  and 64 bits will also be coming to the rescue :))

 I don't think lowmem or LPAE is any problem, if the memory manager is
 designed with that in mind. Vast majority of the buffers kernel
 allocates do not need to be touched in kernel space.

 Actually I can't think of any buffers that we allocate on behalf of user
 space that would need to be permanently mapped also to kernel. In case
 or relocs only push buffer needs to be temporarily mapped to kernel.

 Ultimately even relocs are not necessary if we expose GPU virtual
 addresses directly to user space. But that's another topic.

 Nouveau already exposes constant virtual addresses to userspace and
 skips the pushbuf patching when the presumed offset from userspace is
 the same as what the kernel thinks it should be.

 The problem with lowmem on ARM is that you can't unmap those pages from
 the kernel cached mapping. So if you alloc a page, give it to userspace
 and userspace decides to map the page WC you just produced a conflicting
 mapping, which may yield undefined results on ARMv7. You may think this
 is not a problem as you are not touching the kernel cached mapping, but
 in fact it is. The CPUs prefetcher can still access this mapping.

 Why would this memory be mapped into the kernel?

On ARM the kernel keeps a linear mapping of lowmem using sections
(ARM's version of huge pages). This is always cached, and because the
sections are not 4k, it's a pain to remove parts of it. See
arch/arm/mm/mmu.c

That said, I don't think this issue exists on A15 (which is what those
GPUs are paired with), so it's a purely theoretical problem.

Stéphane
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Nouveau] [RFC] drm/nouveau: disable caching for VRAM BOs on ARM

[Stéphane Marchesin]

On Mon, May 26, 2014 at 7:42 PM, Alexandre Courbot gnu...@gmail.com wrote:
 On Tue, May 27, 2014 at 10:07 AM, Stéphane Marchesin
 stephane.marche...@gmail.com wrote:
 On Mon, May 26, 2014 at 5:02 PM, Alexandre Courbot gnu...@gmail.com wrote:
 On Mon, May 26, 2014 at 6:21 PM, Lucas Stach l.st...@pengutronix.de wrote:
 Am Montag, den 26.05.2014, 09:45 +0300 schrieb Terje Bergström:
 On 23.05.2014 17:40, Alex Courbot wrote:
  On 05/23/2014 06:59 PM, Lucas Stach wrote:
  So after checking with more knowledgeable people, it turns out this is
  the expected behavior on ARM and BAR regions should be mapped uncached
  on GK20A. All the more reasons to avoid using the BAR at all.

 This is actually specific to Tegra.

  You may want to make yourself aware of all the quirks required for
  sharing memory between the GPU and CPU on an ARM host. I think there 
  are
  far more involved than what you see now and writing an replacement for
  TTM will not be an easy task.
 
  Doing away with the concept of two memory areas will not get you to a
  single unified address space. You would have to deal with things like
  not being able to change the caching state of pages in the systems
  lowmem yourself. You will still have to deal with remapping pages that
  aren't currently visible to the CPU (ok this is not an issue on Jetson
  right now as it only has 2GB of RAM), because it's in systems highmem,
  or even in a different LPAE area.
 
  You really want to be sure you are aware of all the consequences of
  this, before considering this task.
 
  Yep, that's why I am seeking advice here. My first hope is that with a
  few tweaks we will be able to keep using TTM and the current nouveau_bo
  implementation. But unless I missed something this is not going to be 
  easy.
 
  We can also use something like the patch I originally sent to make it
  work, although not with good performance, on GK20A. Not very graceful,
  but it will allow applications to run.
 
  In the long run though, we will want to achieve better performance, and
  it seems like a BO implementation targeted at UMA devices would also be
  beneficial to quite a few desktop GPUs. So as tricky as it may be I'm
  interested in gathering thoughts and why not giving it a first try with
  GK20A, even if it imposes some limitations like having buffers in lowmem
  in a first time (we can probably live with this one for a short while,
  and 64 bits will also be coming to the rescue :))

 I don't think lowmem or LPAE is any problem, if the memory manager is
 designed with that in mind. Vast majority of the buffers kernel
 allocates do not need to be touched in kernel space.

 Actually I can't think of any buffers that we allocate on behalf of user
 space that would need to be permanently mapped also to kernel. In case
 or relocs only push buffer needs to be temporarily mapped to kernel.

 Ultimately even relocs are not necessary if we expose GPU virtual
 addresses directly to user space. But that's another topic.

 Nouveau already exposes constant virtual addresses to userspace and
 skips the pushbuf patching when the presumed offset from userspace is
 the same as what the kernel thinks it should be.

 The problem with lowmem on ARM is that you can't unmap those pages from
 the kernel cached mapping. So if you alloc a page, give it to userspace
 and userspace decides to map the page WC you just produced a conflicting
 mapping, which may yield undefined results on ARMv7. You may think this
 is not a problem as you are not touching the kernel cached mapping, but
 in fact it is. The CPUs prefetcher can still access this mapping.

 Why would this memory be mapped into the kernel?

 On ARM the kernel keeps a linear mapping of lowmem using sections
 (ARM's version of huge pages). This is always cached, and because the
 sections are not 4k, it's a pain to remove parts of it. See
 arch/arm/mm/mmu.c

 Ah, are we talking about the directly-mapped low memory region
 starting at PAGE_OFFSET? Ok, it makes sense now, thanks.

 But it seems to me that such different mappings can also happen in
 many other scenarios as well, don't they? How is the issue handled in
 these cases?

It depends. A lot of cache controllers actually implement a solution
for that in hardware, in the cache controller. For example I think
Tegra2 is one of those platforms. And then a lot of platforms just
ignore the issue completely because it has very low probability.

Stéphane
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/tty: Don't hangup shared ttys

[Stéphane Marchesin]

On Tue, Jun 11, 2013 at 4:15 PM, Greg KH  wrote:
> On Tue, Jun 11, 2013 at 04:03:07PM -0700, Stéphane Marchesin wrote:
>> When quickly restarting X servers, we can run into a situation where
>> one X server quits while another one starts on the same tty. For a
>> while, two X servers share the tty, and when the old X server
>> eventually quits, the tty layer hangs up the tty, which among other
>> things stubs out the tty's ioctl functions. Later on, the new X
>> server (which shares the tty functions) tries to call some ioctls
>> on the tty and fails because they have been replaced with the hungup
>> versions. This in turn causes the new X server to abort.
>>
>> This patch checks the tty->count to make sure we're the last
>> consumer before hanging up a tty.
>>
>> Signed-off-by: Stéphane Marchesin 
>> ---
>>  drivers/tty/tty_io.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 6464029..62a0f02 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -619,6 +619,9 @@ static void __tty_hangup(struct tty_struct *tty, int 
>> exit_session)
>>   if (!tty)
>>   return;
>>
>> + /* Don't hangup if there are other users */
>> + if (tty->count > 1)
>> + return;
>
> What happens when you have a "real" tty that was hungup because it was
> disconnected physically from the system yet userspace had a tty open?
> You want those ttys to be hungup properly, right?  Doesn't this change
> break that?

My understanding was that they'd have a different tty_struct. Is that
not the case? If so how would you recommend fixing the problem I
described?

Stéphane
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] drivers/tty: Don't hangup shared ttys

[Stéphane Marchesin]

When quickly restarting X servers, we can run into a situation where
one X server quits while another one starts on the same tty. For a
while, two X servers share the tty, and when the old X server
eventually quits, the tty layer hangs up the tty, which among other
things stubs out the tty's ioctl functions. Later on, the new X
server (which shares the tty functions) tries to call some ioctls
on the tty and fails because they have been replaced with the hungup
versions. This in turn causes the new X server to abort.

This patch checks the tty->count to make sure we're the last
consumer before hanging up a tty.

Signed-off-by: Stéphane Marchesin 
---
 drivers/tty/tty_io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6464029..62a0f02 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -619,6 +619,9 @@ static void __tty_hangup(struct tty_struct *tty, int 
exit_session)
if (!tty)
return;
 
+   /* Don't hangup if there are other users */
+   if (tty->count > 1)
+   return;
 
spin_lock(_lock);
if (redirect && file_tty(redirect) == tty) {
-- 
1.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] drivers/tty: Don't hangup shared ttys

[Stéphane Marchesin]

When quickly restarting X servers, we can run into a situation where
one X server quits while another one starts on the same tty. For a
while, two X servers share the tty, and when the old X server
eventually quits, the tty layer hangs up the tty, which among other
things stubs out the tty's ioctl functions. Later on, the new X
server (which shares the tty functions) tries to call some ioctls
on the tty and fails because they have been replaced with the hungup
versions. This in turn causes the new X server to abort.

This patch checks the tty-count to make sure we're the last
consumer before hanging up a tty.

Signed-off-by: Stéphane Marchesin marc...@chromium.org
---
 drivers/tty/tty_io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6464029..62a0f02 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -619,6 +619,9 @@ static void __tty_hangup(struct tty_struct *tty, int 
exit_session)
if (!tty)
return;
 
+   /* Don't hangup if there are other users */
+   if (tty-count  1)
+   return;
 
spin_lock(redirect_lock);
if (redirect  file_tty(redirect) == tty) {
-- 
1.8.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/tty: Don't hangup shared ttys

[Stéphane Marchesin]

On Tue, Jun 11, 2013 at 4:15 PM, Greg KH gre...@linuxfoundation.org wrote:
 On Tue, Jun 11, 2013 at 04:03:07PM -0700, Stéphane Marchesin wrote:
 When quickly restarting X servers, we can run into a situation where
 one X server quits while another one starts on the same tty. For a
 while, two X servers share the tty, and when the old X server
 eventually quits, the tty layer hangs up the tty, which among other
 things stubs out the tty's ioctl functions. Later on, the new X
 server (which shares the tty functions) tries to call some ioctls
 on the tty and fails because they have been replaced with the hungup
 versions. This in turn causes the new X server to abort.

 This patch checks the tty-count to make sure we're the last
 consumer before hanging up a tty.

 Signed-off-by: Stéphane Marchesin marc...@chromium.org
 ---
  drivers/tty/tty_io.c | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
 index 6464029..62a0f02 100644
 --- a/drivers/tty/tty_io.c
 +++ b/drivers/tty/tty_io.c
 @@ -619,6 +619,9 @@ static void __tty_hangup(struct tty_struct *tty, int 
 exit_session)
   if (!tty)
   return;

 + /* Don't hangup if there are other users */
 + if (tty-count  1)
 + return;

 What happens when you have a real tty that was hungup because it was
 disconnected physically from the system yet userspace had a tty open?
 You want those ttys to be hungup properly, right?  Doesn't this change
 break that?

My understanding was that they'd have a different tty_struct. Is that
not the case? If so how would you recommend fixing the problem I
described?

Stéphane
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [pull] drm-intel-next

[Stéphane Marchesin]

On Thu, Sep 13, 2012 at 7:18 AM, Daniel Vetter  wrote:
> Hi Dave,
>
> The big ticket item here is the new i915 modeset infrastructure.
> Shockingly it didn't not blow up all over the place (i.e. I've managed to
> fix the ugly issues before merging). 1-2 smaller corner cases broke, but
> we have patches. Also, there's tons of patches on top of this that clean
> out cruft and fix a few bugs that couldn't be fixed with the crtc helper
> based stuff. So more stuff to come ;-)
>
> Also a few other things:
> - Tiny fix in the fb helper to go through the official dpms interface
>   instead of calling the crtc helper code.
> - forcewake code frobbery from Ben, code should be more in-line with
>   what Windows does now.
> - fixes for the render ring flush on hsw (Paulo)
> - gpu frequency tracepoint
> - vlv forcewake changes to better align it with our understanding of the
>   forcewake magic.
> - a few smaller cleanups
>
> Cheers, Daniel
>
>
> The following changes since commit d7c3b937bdf45f0b844400b7bf6fd3ed50bac604:
>
>   drm/i915: Remove __GFP_NO_KSWAPD (2012-08-27 17:11:38 +0200)
>
> are available in the git repository at:
>
>   git://people.freedesktop.org/~danvet/drm-intel 
> tags/drm-intel-next-2012-09-09
>
> for you to fetch changes up to e04190e0ecb236c51af181c18c545ea076fb9cca:
>
>   drm/fb helper: don't call drm_helper_connector_dpms directly (2012-09-08 
> 00:51:15 +0200)
>
> 
>
> Ben Widawsky (5):
>   drm/i915: Extract forcewake ack timeout
>   drm/i915: use cpu_relax() in wait_for_atomic
>   drm/i915: Change forcewake timeout to 2ms
>   drm/i915: Never read FORCEWAKE
>   drm/i915: Enable some sysfs stuff without CONFIG_PM
>
> Chris Wilson (1):
>   drm/i915: Convert remaining debugfs iterators over rings to 
> for_each_ring()
>
> Daniel Vetter (66):
>   drm/ips: move drps/ips/ilk related variables into dev_priv->ips
>   drm/i915: add a tracepoint for gpu frequency changes
>   drm/i915: align vlv forcewake with common lore
>   drm/i915: differ error message between forcwake timeouts
>   drm/i915: add crtc->enable/disable vfuncs insted of dpms
>   drm/i915: rip out crtc prepare/commit indirection
>   drm/i915: add direct encoder disable/enable infrastructure
>   drm/i915/hdmi: convert to encoder->disable/enable
>   drm/i915/tv: convert to encoder enable/disable
>   drm/i915/lvds: convert to encoder disable/enable
>   drm/i915/dp: convert to encoder disable/enable
>   drm/i915/crt: convert to encoder disable/enable
>   drm/i915/sdvo: convert to encoder disable/enable
>   drm/i915/dvo: convert to encoder disable/enable
>   drm/i915: convert dpms functions of dvo/sdvo/crt
>   drm/i915: rip out encoder->disable/enable checks
>   drm/i915: clean up encoder_prepare/commit
>   drm/i915: copy drm_crtc_helper_set_config
>   drm/i915: call set_base directly
>   drm/i915: inline intel_best_encoder
>   drm/i915: copy drm_crtc_helper_set_mode
>   drm/i915: simplify intel_crtc_prepare_encoders
>   drm/i915: rip out encoder->prepare/commit
>   drm/i915: call crtc functions directly
>   drm/i915: WARN when trying to enabled an unused crtc
>   drm/i915: Add interfaces to read out encoder/connector hw state
>   drm/i915/dp: implement get_hw_state
>   drm/i915/hdmi: implement get_hw_state
>   drm/i915/tv: implement get_hw_state
>   drm/i915/lvds: implement get_hw_state
>   drm/i915/crt: implement get_hw_state
>   drm/i915/sdvo: implement get_hw_state
>   drm/i915/dvo: implement get_hw_state
>   drm/i915: read out the modeset hw state at load and resume time

Hi Daniel,

This commit regresses modeset on the samsung series 5 chromebook (it
is basically a pineview machine with an lvds panel). I don't seem to
be able to set any mode on it any longer.

Any idea?

Stéphane

>   drm/i915: check connector hw/sw state
>   drm/i915: rip out intel_crtc->dpms_mode
>   drm/i915: rip out intel_dp->dpms_mode
>   drm/i915: ensure the force pipe A quirk is actually followed
>   drm/i915: introduce struct intel_set_config
>   drm/i915: extract modeset config save/restore code
>   drm/i915: extract intel_set_config_compute_mode_changes
>   drm/i915: extract intel_set_config_update_output_state
>   drm/i915: implement crtc helper semantics relied upon by the fb helper
>   drm/i915: don't update the fb base if there is no fb
>   drm/i915: convert pointless error checks in set_config to BUGs
>   drm/i915: don't save all the encoder/crtc state in set_config
>   drm/i915: stage modeset output changes
>   drm/i915: push crtc->fb update into pipe_set_base
>   drm/i915: remove crtc disabling special case
>   drm/i915: move output commit and crtc disabling into set_mode
>   drm/i915: extract adjusted mode computation
>   drm/i915: use staged outuput config 

Re: [pull] drm-intel-next

[Stéphane Marchesin]

On Thu, Sep 13, 2012 at 7:18 AM, Daniel Vetter dan...@ffwll.ch wrote:
 Hi Dave,

 The big ticket item here is the new i915 modeset infrastructure.
 Shockingly it didn't not blow up all over the place (i.e. I've managed to
 fix the ugly issues before merging). 1-2 smaller corner cases broke, but
 we have patches. Also, there's tons of patches on top of this that clean
 out cruft and fix a few bugs that couldn't be fixed with the crtc helper
 based stuff. So more stuff to come ;-)

 Also a few other things:
 - Tiny fix in the fb helper to go through the official dpms interface
   instead of calling the crtc helper code.
 - forcewake code frobbery from Ben, code should be more in-line with
   what Windows does now.
 - fixes for the render ring flush on hsw (Paulo)
 - gpu frequency tracepoint
 - vlv forcewake changes to better align it with our understanding of the
   forcewake magic.
 - a few smaller cleanups

 Cheers, Daniel


 The following changes since commit d7c3b937bdf45f0b844400b7bf6fd3ed50bac604:

   drm/i915: Remove __GFP_NO_KSWAPD (2012-08-27 17:11:38 +0200)

 are available in the git repository at:

   git://people.freedesktop.org/~danvet/drm-intel 
 tags/drm-intel-next-2012-09-09

 for you to fetch changes up to e04190e0ecb236c51af181c18c545ea076fb9cca:

   drm/fb helper: don't call drm_helper_connector_dpms directly (2012-09-08 
 00:51:15 +0200)

 

 Ben Widawsky (5):
   drm/i915: Extract forcewake ack timeout
   drm/i915: use cpu_relax() in wait_for_atomic
   drm/i915: Change forcewake timeout to 2ms
   drm/i915: Never read FORCEWAKE
   drm/i915: Enable some sysfs stuff without CONFIG_PM

 Chris Wilson (1):
   drm/i915: Convert remaining debugfs iterators over rings to 
 for_each_ring()

 Daniel Vetter (66):
   drm/ips: move drps/ips/ilk related variables into dev_priv-ips
   drm/i915: add a tracepoint for gpu frequency changes
   drm/i915: align vlv forcewake with common lore
   drm/i915: differ error message between forcwake timeouts
   drm/i915: add crtc-enable/disable vfuncs insted of dpms
   drm/i915: rip out crtc prepare/commit indirection
   drm/i915: add direct encoder disable/enable infrastructure
   drm/i915/hdmi: convert to encoder-disable/enable
   drm/i915/tv: convert to encoder enable/disable
   drm/i915/lvds: convert to encoder disable/enable
   drm/i915/dp: convert to encoder disable/enable
   drm/i915/crt: convert to encoder disable/enable
   drm/i915/sdvo: convert to encoder disable/enable
   drm/i915/dvo: convert to encoder disable/enable
   drm/i915: convert dpms functions of dvo/sdvo/crt
   drm/i915: rip out encoder-disable/enable checks
   drm/i915: clean up encoder_prepare/commit
   drm/i915: copypaste drm_crtc_helper_set_config
   drm/i915: call set_base directly
   drm/i915: inline intel_best_encoder
   drm/i915: copypaste drm_crtc_helper_set_mode
   drm/i915: simplify intel_crtc_prepare_encoders
   drm/i915: rip out encoder-prepare/commit
   drm/i915: call crtc functions directly
   drm/i915: WARN when trying to enabled an unused crtc
   drm/i915: Add interfaces to read out encoder/connector hw state
   drm/i915/dp: implement get_hw_state
   drm/i915/hdmi: implement get_hw_state
   drm/i915/tv: implement get_hw_state
   drm/i915/lvds: implement get_hw_state
   drm/i915/crt: implement get_hw_state
   drm/i915/sdvo: implement get_hw_state
   drm/i915/dvo: implement get_hw_state
   drm/i915: read out the modeset hw state at load and resume time

Hi Daniel,

This commit regresses modeset on the samsung series 5 chromebook (it
is basically a pineview machine with an lvds panel). I don't seem to
be able to set any mode on it any longer.

Any idea?

Stéphane

   drm/i915: check connector hw/sw state
   drm/i915: rip out intel_crtc-dpms_mode
   drm/i915: rip out intel_dp-dpms_mode
   drm/i915: ensure the force pipe A quirk is actually followed
   drm/i915: introduce struct intel_set_config
   drm/i915: extract modeset config save/restore code
   drm/i915: extract intel_set_config_compute_mode_changes
   drm/i915: extract intel_set_config_update_output_state
   drm/i915: implement crtc helper semantics relied upon by the fb helper
   drm/i915: don't update the fb base if there is no fb
   drm/i915: convert pointless error checks in set_config to BUGs
   drm/i915: don't save all the encoder/crtc state in set_config
   drm/i915: stage modeset output changes
   drm/i915: push crtc-fb update into pipe_set_base
   drm/i915: remove crtc disabling special case
   drm/i915: move output commit and crtc disabling into set_mode
   drm/i915: extract adjusted mode computation
   drm/i915: use staged outuput config in tv-mode_fixup
   drm/i915: use staged outuput config in lvds-mode_fixup