Re: [PATCH 1/4] drm/i915/doc: Convert drm_i915_query_topology_info comment to kerneldoc

[Francisco Jerez]

Looks good to me, series is:

Reviewed-by: Francisco Jerez 

Matt Roper  writes:

> This structure has a great comment describing the fields, but it's not
> currently in kerneldoc form and does not show up in the generated
> documentation.  Let's fix that and also clarify the description of what
> "subslice" refers to on gen12 platforms and beyond and that "slice" is
> no longer meaningful on Xe_HP and beyond.
>
> Signed-off-by: Matt Roper 
> ---
>  include/uapi/drm/i915_drm.h | 110 +---
>  1 file changed, 78 insertions(+), 32 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 9ab021c4d632..73e1c6180ddb 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2775,66 +2775,112 @@ struct drm_i915_query {
>   __u64 items_ptr;
>  };
>  
> -/*
> - * Data written by the kernel with query DRM_I915_QUERY_TOPOLOGY_INFO :
> - *
> - * data: contains the 3 pieces of information :
> - *
> - * - the slice mask with one bit per slice telling whether a slice is
> - *   available. The availability of slice X can be queried with the following
> - *   formula :
> - *
> - *   (data[X / 8] >> (X % 8)) & 1
> - *
> - * - the subslice mask for each slice with one bit per subslice telling
> - *   whether a subslice is available. Gen12 has dual-subslices, which are
> - *   similar to two gen11 subslices. For gen12, this array represents dual-
> - *   subslices. The availability of subslice Y in slice X can be queried
> - *   with the following formula :
> - *
> - *   (data[subslice_offset +
> - * X * subslice_stride +
> - * Y / 8] >> (Y % 8)) & 1
> - *
> - * - the EU mask for each subslice in each slice with one bit per EU telling
> - *   whether an EU is available. The availability of EU Z in subslice Y in
> - *   slice X can be queried with the following formula :
> +/**
> + * struct drm_i915_query_topology_info
>   *
> - *   (data[eu_offset +
> - * (X * max_subslices + Y) * eu_stride +
> - * Z / 8] >> (Z % 8)) & 1
> + * Describes slice/subslice/EU information queried by
> + * %DRM_I915_QUERY_TOPOLOGY_INFO
>   */
>  struct drm_i915_query_topology_info {
> - /*
> + /**
> +  * @flags:
> +  *
>* Unused for now. Must be cleared to zero.
>*/
>   __u16 flags;
>  
> + /**
> +  * @max_slices:
> +  *
> +  * The number of bits used to express the slice mask.
> +  */
>   __u16 max_slices;
> +
> + /**
> +  * @max_subslices:
> +  *
> +  * The number of bits used to express the subslice mask.
> +  */
>   __u16 max_subslices;
> +
> + /**
> +  * @max_eus_per_subslice:
> +  *
> +  * The number of bits in the EU mask that correspond to a single
> +  * subslice's EUs.
> +  */
>   __u16 max_eus_per_subslice;
>  
> - /*
> + /**
> +  * @subslice_offset:
> +  *
>* Offset in data[] at which the subslice masks are stored.
>*/
>   __u16 subslice_offset;
>  
> - /*
> + /**
> +  * @subslice_stride:
> +  *
>* Stride at which each of the subslice masks for each slice are
>* stored.
>*/
>   __u16 subslice_stride;
>  
> - /*
> + /**
> +  * @eu_offset:
> +  *
>* Offset in data[] at which the EU masks are stored.
>*/
>   __u16 eu_offset;
>  
> - /*
> + /**
> +  * @eu_stride:
> +  *
>* Stride at which each of the EU masks for each subslice are stored.
>*/
>   __u16 eu_stride;
>  
> + /**
> +  * @data:
> +  *
> +  * Contains 3 pieces of information :
> +  *
> +  * - The slice mask with one bit per slice telling whether a slice is
> +  *   available. The availability of slice X can be queried with the
> +  *   following formula :
> +  *
> +  *   .. code:: c
> +  *
> +  *  (data[X / 8] >> (X % 8)) & 1
> +  *
> +  *   Starting with Xe_HP platforms, Intel hardware no longer has
> +  *   traditional slices so i915 will always report a single slice
> +  *   (hardcoded slicemask = 0x1) which contains all of the platform's
> +  *   subslices.  I.e., the mask here does not reflect any of the newer
> +  *   hardware concepts such as "gslices" or "cslices" since userspace
> +  *   is capable of inferring those from the subslice mask.
> +  *
> +  * - T

Re: [PATCH v4 RFC] drm/i915/uapi: Add DRM_I915_QUERY_GEOMETRY_SUBSLICES

[Francisco Jerez]

Daniel Vetter  writes:

> On Wed, Mar 30, 2022 at 02:53:11PM -0700, Matt Atwood wrote:
>> Newer platforms have DSS that aren't necessarily available for both
>> geometry and compute, two queries will need to exist. This introduces
>> the first, when passing a valid engine class and engine instance in the
>> flags returns a topology describing geometry.
>> 
>> v2: fix white space errors
>> v3: change flags from hosting 2 8 bit numbers to holding a
>> i915_engine_class_instance struct
>> v4: add error if non rcs engine passed.
>> 
>> Cc: Ashutosh Dixit 
>> Cc: Matt Roper 
>> Cc: Joonas Lahtinen 
>> UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14143
>> 
>> Signed-off-by: Matt Atwood 
>> ---
>>  drivers/gpu/drm/i915/i915_query.c | 71 ++-
>>  include/uapi/drm/i915_drm.h   | 26 +++
>>  2 files changed, 69 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index b5ca00cb6cf6..32be84c95956 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -9,6 +9,7 @@
>>  #include "i915_drv.h"
>>  #include "i915_perf.h"
>>  #include "i915_query.h"
>> +#include "gt/intel_engine_user.h"
>>  #include 
>>  
>>  static int copy_query_item(void *query_hdr, size_t query_sz,
>> @@ -28,36 +29,30 @@ static int copy_query_item(void *query_hdr, size_t 
>> query_sz,
>>  return 0;
>>  }
>>  
>> -static int query_topology_info(struct drm_i915_private *dev_priv,
>> -   struct drm_i915_query_item *query_item)
>> +static int fill_topology_info(const struct sseu_dev_info *sseu,
>> +  struct drm_i915_query_item *query_item,
>> +  const u8 *subslice_mask)
>>  {
>> -const struct sseu_dev_info *sseu = _gt(dev_priv)->info.sseu;
>>  struct drm_i915_query_topology_info topo;
>>  u32 slice_length, subslice_length, eu_length, total_length;
>>  int ret;
>>  
>> -if (query_item->flags != 0)
>> -return -EINVAL;
>> +BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
>>  
>>  if (sseu->max_slices == 0)
>>  return -ENODEV;
>>  
>> -BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
>> -
>>  slice_length = sizeof(sseu->slice_mask);
>>  subslice_length = sseu->max_slices * sseu->ss_stride;
>>  eu_length = sseu->max_slices * sseu->max_subslices * sseu->eu_stride;
>>  total_length = sizeof(topo) + slice_length + subslice_length +
>> eu_length;
>>  
>> -ret = copy_query_item(, sizeof(topo), total_length,
>> -  query_item);
>> +ret = copy_query_item(, sizeof(topo), total_length, query_item);
>> +
>>  if (ret != 0)
>>  return ret;
>>  
>> -if (topo.flags != 0)
>> -return -EINVAL;
>> -
>>  memset(, 0, sizeof(topo));
>>  topo.max_slices = sseu->max_slices;
>>  topo.max_subslices = sseu->max_subslices;
>> @@ -69,27 +64,64 @@ static int query_topology_info(struct drm_i915_private 
>> *dev_priv,
>>  topo.eu_stride = sseu->eu_stride;
>>  
>>  if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
>> -   , sizeof(topo)))
>> + , sizeof(topo)))
>>  return -EFAULT;
>>  
>>  if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)),
>> -   >slice_mask, slice_length))
>> + >slice_mask, slice_length))
>>  return -EFAULT;
>>  
>>  if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>> -   sizeof(topo) + slice_length),
>> -   sseu->subslice_mask, subslice_length))
>> + sizeof(topo) + slice_length),
>> + subslice_mask, subslice_length))
>>  return -EFAULT;
>>  
>>  if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>> -   sizeof(topo) +
>> -   slice_length + subslice_length),
>> -   sseu->eu_mask, eu_length))
>> + sizeof(topo) +
>> + slice_length + subslice_length),
>> + sseu->eu_mask, eu_length))
>>  return -EFAULT;
>>  
>>  return total_length;
>>  }
>>  
>> +static int query_topology_info(struct drm_i915_private *dev_priv,
>> +   struct drm_i915_query_item *query_item)
>> +{
>> +const struct sseu_dev_info *sseu = _gt(dev_priv)->info.sseu;
>> +
>> +if (query_item->flags != 0)
>> +return -EINVAL;
>> +
>> +return fill_topology_info(sseu, query_item, sseu->subslice_mask);
>> +}
>> +
>> +static int query_geometry_subslices(struct drm_i915_private *i915,
>> +struct drm_i915_query_item *query_item)
>> +{
>> +const 

Re: [Intel-gfx] [PATCH] drm/i915/uapi: Add DRM_I915_QUERY_GEOMETRY_SUBSLICES

[Francisco Jerez]

Matt Atwood  writes:

> Newer platforms have DSS that aren't necessarily available for both
> geometry and compute, two queries will need to exist. This introduces
> the first, when passing a valid engine class and engine instance in the
> flags returns a topology describing geometry.
>
> v2: fix white space errors
> v3: change flags from hosting 2 8 bit numbers to holding a
> i915_engine_class_instance struct
>
> Cc: Ashutosh Dixit 
> Cc: Matt Roper 
> Cc: Joonas Lahtinen 
> UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14143
> Signed-off-by: Matt Atwood 
> ---
>  drivers/gpu/drm/i915/i915_query.c | 68 ++-
>  include/uapi/drm/i915_drm.h   | 24 +++
>  2 files changed, 65 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c 
> b/drivers/gpu/drm/i915/i915_query.c
> index 2dfbc22857a3..fcb374201edb 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -9,6 +9,7 @@
>  #include "i915_drv.h"
>  #include "i915_perf.h"
>  #include "i915_query.h"
> +#include "gt/intel_engine_user.h"
>  #include 
>  
>  static int copy_query_item(void *query_hdr, size_t query_sz,
> @@ -28,36 +29,30 @@ static int copy_query_item(void *query_hdr, size_t 
> query_sz,
>   return 0;
>  }
>  
> -static int query_topology_info(struct drm_i915_private *dev_priv,
> -struct drm_i915_query_item *query_item)
> +static int fill_topology_info(const struct sseu_dev_info *sseu,
> +   struct drm_i915_query_item *query_item,
> +   const u8 *subslice_mask)
>  {
> - const struct sseu_dev_info *sseu = _gt(dev_priv)->info.sseu;
>   struct drm_i915_query_topology_info topo;
>   u32 slice_length, subslice_length, eu_length, total_length;
>   int ret;
>  
> - if (query_item->flags != 0)
> - return -EINVAL;
> + BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
>  
>   if (sseu->max_slices == 0)
>   return -ENODEV;
>  
> - BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
> -
>   slice_length = sizeof(sseu->slice_mask);
>   subslice_length = sseu->max_slices * sseu->ss_stride;
>   eu_length = sseu->max_slices * sseu->max_subslices * sseu->eu_stride;
>   total_length = sizeof(topo) + slice_length + subslice_length +
>  eu_length;
>  
> - ret = copy_query_item(, sizeof(topo), total_length,
> -   query_item);
> + ret = copy_query_item(, sizeof(topo), total_length, query_item);
> +
>   if (ret != 0)
>   return ret;
>  
> - if (topo.flags != 0)
> - return -EINVAL;
> -
>   memset(, 0, sizeof(topo));
>   topo.max_slices = sseu->max_slices;
>   topo.max_subslices = sseu->max_subslices;
> @@ -69,27 +64,61 @@ static int query_topology_info(struct drm_i915_private 
> *dev_priv,
>   topo.eu_stride = sseu->eu_stride;
>  
>   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
> -, sizeof(topo)))
> +  , sizeof(topo)))
>   return -EFAULT;
>  
>   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)),
> ->slice_mask, slice_length))
> +  >slice_mask, slice_length))
>   return -EFAULT;
>  
>   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
> -sizeof(topo) + slice_length),
> -sseu->subslice_mask, subslice_length))
> +  sizeof(topo) + slice_length),
> +  subslice_mask, subslice_length))
>   return -EFAULT;
>  
>   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
> -sizeof(topo) +
> -slice_length + subslice_length),
> -sseu->eu_mask, eu_length))
> +  sizeof(topo) +
> +  slice_length + subslice_length),
> +  sseu->eu_mask, eu_length))
>   return -EFAULT;
>  
>   return total_length;
>  }
>  
> +static int query_topology_info(struct drm_i915_private *dev_priv,
> +struct drm_i915_query_item *query_item)
> +{
> + const struct sseu_dev_info *sseu = _gt(dev_priv)->info.sseu;
> +
> + if (query_item->flags != 0)
> + return -EINVAL;
> +
> + return fill_topology_info(sseu, query_item, sseu->subslice_mask);
> +}
> +
> +static int query_geometry_subslices(struct drm_i915_private *i915,
> + struct drm_i915_query_item *query_item)
> +{
> + const struct sseu_dev_info *sseu;
> + struct intel_engine_cs *engine;
> + struct i915_engine_class_instance classinstance;
> +
> + if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 

Re: [Intel-gfx] [RFC PATCH 00/97] Basic GuC submission support in the i915

[Francisco Jerez]

Daniel Vetter  writes:

> On Mon, May 10, 2021 at 3:55 PM Martin Peres  wrote:
>>
>> On 10/05/2021 02:11, Jason Ekstrand wrote:
>> > On May 9, 2021 12:12:36 Martin Peres  wrote:
>> >
>> >> Hi,
>> >>
>> >> On 06/05/2021 22:13, Matthew Brost wrote:
>> >>> Basic GuC submission support. This is the first bullet point in the
>> >>> upstreaming plan covered in the following RFC [1].
>> >>>
>> >>> At a very high level the GuC is a piece of firmware which sits between
>> >>> the i915 and the GPU. It offloads some of the scheduling of contexts
>> >>> from the i915 and programs the GPU to submit contexts. The i915
>> >>> communicates with the GuC and the GuC communicates with the GPU.
>> >>
>> >> May I ask what will GuC command submission do that execlist won't/can't
>> >> do? And what would be the impact on users? Even forgetting the troubled
>> >> history of GuC (instability, performance regression, poor level of user
>> >> support, 6+ years of trying to upstream it...), adding this much code
>> >> and doubling the amount of validation needed should come with a
>> >> rationale making it feel worth it... and I am not seeing here. Would you
>> >> mind providing the rationale behind this work?
>> >>
>> >>>
>> >>> GuC submission will be disabled by default on all current upstream
>> >>> platforms behind a module parameter - enable_guc. A value of 3 will
>> >>> enable submission and HuC loading via the GuC. GuC submission should
>> >>> work on all gen11+ platforms assuming the GuC firmware is present.
>> >>
>> >> What is the plan here when it comes to keeping support for execlist? I
>> >> am afraid that landing GuC support in Linux is the first step towards
>> >> killing the execlist, which would force users to use proprietary
>> >> firmwares that even most Intel engineers have little influence over.
>> >> Indeed, if "drm/i915/guc: Disable semaphores when using GuC scheduling"
>> >> which states "Disable semaphores when using GuC scheduling as semaphores
>> >> are broken in the current GuC firmware." is anything to go by, it means
>> >> that even Intel developers seem to prefer working around the GuC
>> >> firmware, rather than fixing it.
>> >
>> > Yes, landing GuC support may be the first step in removing execlist
>> > support. The inevitable reality is that GPU scheduling is coming and
>> > likely to be there only path in the not-too-distant future. (See also
>> > the ongoing thread with AMD about fences.) I'm not going to pass
>> > judgement on whether or not this is a good thing.  I'm just reading the
>> > winds and, in my view, this is where things are headed for good or ill.
>> >
>> > In answer to the question above, the answer to "what do we gain from
>> > GuC?" may soon be, "you get to use your GPU."  We're not there yet and,
>> > again, I'm not necessarily advocating for it, but that is likely where
>> > things are headed.
>>
>> This will be a sad day, especially since it seems fundamentally opposed
>> with any long-term support, on top of taking away user freedom to
>> fix/tweak their system when Intel won't.
>>
>> > A firmware-based submission model isn't a bad design IMO and, aside from
>> > the firmware freedom issues, I think there are actual advantages to the
>> > model. Immediately, it'll unlock a few features like parallel submission
>> > (more on that in a bit) and long-running compute because they're
>> > implemented in GuC and the work to implement them properly in the
>> > execlist scheduler is highly non-trivial. Longer term, it may (no
>> > guarantees) unlock some performance by getting the kernel out of the way.
>>
>> Oh, I definitely agree with firmware-based submission model not being a
>> bad design. I was even cheering for it in 2015. Experience with it made
>> me regret that deeply since :s
>>
>> But with the DRM scheduler being responsible for most things, I fail to
>> see what we could offload in the GuC except context switching (like
>> every other manufacturer). The problem is, the GuC does way more than
>> just switching registers in bulk, and if the number of revisions of the
>> GuC is anything to go by, it is way too complex for me to feel
>> comfortable with it.
>
> We need to flesh out that part of the plan more, but we're not going
> to use drm scheduler for everything. It's only to handle the dma-fence
> legacy side of things, which means:
> - timeout handling for batches that take too long
> - dma_fence dependency sorting/handling
> - boosting of context from display flips (currently missing, needs to
> be ported from drm/i915)
>
> The actual round-robin/preempt/priority handling is still left to the
> backend, in this case here the fw. So there's large chunks of
> code/functionality where drm/scheduler wont be involved in, and like
> Jason says: The hw direction winds definitely blow in the direction
> that this is all handled in hw.
>

I agree with Martin on this.  Given that using GuC currently involves
making your open-source graphics stack rely on a closed-source

Re: [PATCH v2 4/9] drm/i2c/sil164: use drm_debug_enabled() to check for debug categories

[Francisco Jerez]

Jani Nikula  writes:

> Allow better abstraction of the drm_debug global variable in the
> future. No functional changes.
>
> Cc: Francisco Jerez 
> Signed-off-by: Jani Nikula 

Reviewed-by: Francisco Jerez 

> ---
>  drivers/gpu/drm/i2c/sil164_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/sil164_drv.c 
> b/drivers/gpu/drm/i2c/sil164_drv.c
> index 8bcf0d199145..a839f78a4c8a 100644
> --- a/drivers/gpu/drm/i2c/sil164_drv.c
> +++ b/drivers/gpu/drm/i2c/sil164_drv.c
> @@ -44,7 +44,7 @@ struct sil164_priv {
>   ((struct sil164_priv *)to_encoder_slave(x)->slave_priv)
>  
>  #define sil164_dbg(client, format, ...) do { \
> - if (drm_debug & DRM_UT_KMS) \
> + if (drm_debug_enabled(DRM_UT_KMS))  \
>   dev_printk(KERN_DEBUG, >dev,\
>  "%s: " format, __func__, ## __VA_ARGS__); \
>   } while (0)
> -- 
> 2.20.1


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 14/15] drm/i2c/ch7006: Use helper to turn off CRTC

[Francisco Jerez]

Lukas Wunner  writes:

> Use shiny new drm_crtc_force_disable() instead of open coding the same.
> No functional change intended.
>
> Cc: Francisco Jerez 
> Signed-off-by: Lukas Wunner 

Reviewed-by: Francisco Jerez 

> ---
>  drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c 
> b/drivers/gpu/drm/i2c/ch7006_drv.c
> index 0594c45..e9e8ae2 100644
> --- a/drivers/gpu/drm/i2c/ch7006_drv.c
> +++ b/drivers/gpu/drm/i2c/ch7006_drv.c
> @@ -361,13 +361,8 @@ static int ch7006_encoder_set_property(struct 
> drm_encoder *encoder,
>  
>   /* Disable the crtc to ensure a full modeset is
>* performed whenever it's turned on again. */
> - if (crtc) {
> - struct drm_mode_set modeset = {
> - .crtc = crtc,
> - };
> -
> - drm_mode_set_config_internal();
> - }
> + if (crtc)
> + drm_crtc_force_disable(crtc);
>   }
>  
>   return 0;
> -- 
> 2.8.1
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160611/8843899b/attachment.sig>

[PATCH] drm: encoder_slave: respect of_node on i2c encoder init

[Francisco Jerez]

Hi,

Sebastian Hesselbarth  writes:

>> - I think we could also drop the call to ->set_config since presumably an
>>of-enabled driver grabbed any required info already from the dt.
>[...]
>> I think this way we could still share encoder slaves across tons of
>> platforms, only the init sequence (and specifically how they get at their

The "set_config" hook is just the way a DRM driver communicates those
board-specific differences in the init sequence to the slave encoder
driver.  I don't think it would make sense to remove it unless we make
sure it's being called elsewhere.

>
> IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave
> driver is even really using its probe(). Everything is packed somewhere
> because it just worked.. this is at least a start.
>
Why is that?  What do you mean by the probe hooks not being used?
ch7006 and sil164 rely on it being called on initialization.

> I suggest this to get merged to at least allow to have DT slaves,
> then start with improving tda998x as an example, then maybe rethink
> drm_slave_encoder completely, e.g.
>
> - generalize the concept to SPI attached encoders, "internal" encoders..

drm_encoder_slave is bus-agnostic.  drm_i2c_encoder_init() is just a
helper function taking care of bus-specific details like the creation of
the underlying I2C device object, which cannot be made bus-agnostic for
obvious reasons.  You're welcome to implement SPI and internal
counterparts of drm_i2c_encoder_init().

> - find a way to setup .encoder_type and .connector_type correctly

I guess encoder_type could be initialized correctly from the slave
encoder_init() hook -- that hasn't been necessary until now because the
DRM driver making use of the slave encoder has been expected to have
some other means to find out encoder types [e.g. device-specific BIOS
tables].  OTOH, I don't think that setting connector types is the slave
encoder's business.

> - have more common of_drm_blabla helpers
>
>> config data) would be different. That would also be extensible quite
>> easily (*cough* intel platforms could setup encoder slaves from
>> information out of the vbt *cough*).
>>
>[...]
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: [PATCH] drm: encoder_slave: respect of_node on i2c encoder init

[Francisco Jerez]

Hi,

Sebastian Hesselbarth sebastian.hesselba...@gmail.com writes:

 - I think we could also drop the call to -set_config since presumably an
of-enabled driver grabbed any required info already from the dt.
[...]
 I think this way we could still share encoder slaves across tons of
 platforms, only the init sequence (and specifically how they get at their

The set_config hook is just the way a DRM driver communicates those
board-specific differences in the init sequence to the slave encoder
driver.  I don't think it would make sense to remove it unless we make
sure it's being called elsewhere.


 IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave
 driver is even really using its probe(). Everything is packed somewhere
 because it just worked.. this is at least a start.

Why is that?  What do you mean by the probe hooks not being used?
ch7006 and sil164 rely on it being called on initialization.

 I suggest this to get merged to at least allow to have DT slaves,
 then start with improving tda998x as an example, then maybe rethink
 drm_slave_encoder completely, e.g.

 - generalize the concept to SPI attached encoders, internal encoders..

drm_encoder_slave is bus-agnostic.  drm_i2c_encoder_init() is just a
helper function taking care of bus-specific details like the creation of
the underlying I2C device object, which cannot be made bus-agnostic for
obvious reasons.  You're welcome to implement SPI and internal
counterparts of drm_i2c_encoder_init().

 - find a way to setup .encoder_type and .connector_type correctly

I guess encoder_type could be initialized correctly from the slave
encoder_init() hook -- that hasn't been necessary until now because the
DRM driver making use of the slave encoder has been expected to have
some other means to find out encoder types [e.g. device-specific BIOS
tables].  OTOH, I don't think that setting connector types is the slave
encoder's business.

 - have more common of_drm_blabla helpers

 config data) would be different. That would also be extensible quite
 easily (*cough* intel platforms could setup encoder slaves from
 information out of the vbt *cough*).

[...]


pgpPys4RF08O3.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i2c/ch7006: Convert to dev_pm_ops

[Francisco Jerez]

Mark Brown  writes:

> The I2C specific suspend and resume functions have been deprecated and
> printing a warning on boot for over a year, dev_pm_ops should be used
> instead so convert to that.
>
> Also remove the suspend function since all it does is log.
>
> Signed-off-by: Mark Brown 
> ---
>  drivers/gpu/drm/i2c/ch7006_drv.c |   16 +++-
>  1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c 
> b/drivers/gpu/drm/i2c/ch7006_drv.c
> index d3f2e87..ca4435d 100644
> --- a/drivers/gpu/drm/i2c/ch7006_drv.c
> +++ b/drivers/gpu/drm/i2c/ch7006_drv.c
> @@ -427,15 +427,10 @@ static int ch7006_remove(struct i2c_client *client)
>   return 0;
>  }
>  
> -static int ch7006_suspend(struct i2c_client *client, pm_message_t mesg)
> +static int ch7006_resume(struct device *dev)
>  {
> - ch7006_dbg(client, "\n");
> -
> - return 0;
> -}
> + struct i2c_client *client = to_i2c_device(dev);
>  
> -static int ch7006_resume(struct i2c_client *client)
> -{
>   ch7006_dbg(client, "\n");
>  
>   ch7006_write(client, 0x3d, 0x0);
> @@ -499,15 +494,18 @@ static struct i2c_device_id ch7006_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ch7006_ids);
>  
> +static const struct dev_pm_ops ch7006_pm_ops = {
> + .resume = ch7006_resume,
> +};
> +
>  static struct drm_i2c_encoder_driver ch7006_driver = {
>   .i2c_driver = {
>   .probe = ch7006_probe,
>   .remove = ch7006_remove,
> - .suspend = ch7006_suspend,
> - .resume = ch7006_resume,
>  
>   .driver = {
>   .name = "ch7006",
> + .pm = ch7006_pm_ops,
>   },
>  
>   .id_table = ch7006_ids,

Thanks for fixing this,

Reviewed-by: Francisco Jerez 
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20120323/ea17e8a8/attachment.pgp>

Re: [PATCH] drm/i2c/ch7006: Convert to dev_pm_ops

[Francisco Jerez]

Mark Brown broo...@opensource.wolfsonmicro.com writes:

 The I2C specific suspend and resume functions have been deprecated and
 printing a warning on boot for over a year, dev_pm_ops should be used
 instead so convert to that.

 Also remove the suspend function since all it does is log.

 Signed-off-by: Mark Brown broo...@opensource.wolfsonmicro.com
 ---
  drivers/gpu/drm/i2c/ch7006_drv.c |   16 +++-
  1 files changed, 7 insertions(+), 9 deletions(-)

 diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c 
 b/drivers/gpu/drm/i2c/ch7006_drv.c
 index d3f2e87..ca4435d 100644
 --- a/drivers/gpu/drm/i2c/ch7006_drv.c
 +++ b/drivers/gpu/drm/i2c/ch7006_drv.c
 @@ -427,15 +427,10 @@ static int ch7006_remove(struct i2c_client *client)
   return 0;
  }
  
 -static int ch7006_suspend(struct i2c_client *client, pm_message_t mesg)
 +static int ch7006_resume(struct device *dev)
  {
 - ch7006_dbg(client, \n);
 -
 - return 0;
 -}
 + struct i2c_client *client = to_i2c_device(dev);
  
 -static int ch7006_resume(struct i2c_client *client)
 -{
   ch7006_dbg(client, \n);
  
   ch7006_write(client, 0x3d, 0x0);
 @@ -499,15 +494,18 @@ static struct i2c_device_id ch7006_ids[] = {
  };
  MODULE_DEVICE_TABLE(i2c, ch7006_ids);
  
 +static const struct dev_pm_ops ch7006_pm_ops = {
 + .resume = ch7006_resume,
 +};
 +
  static struct drm_i2c_encoder_driver ch7006_driver = {
   .i2c_driver = {
   .probe = ch7006_probe,
   .remove = ch7006_remove,
 - .suspend = ch7006_suspend,
 - .resume = ch7006_resume,
  
   .driver = {
   .name = ch7006,
 + .pm = ch7006_pm_ops,
   },
  
   .id_table = ch7006_ids,

Thanks for fixing this,

Reviewed-by: Francisco Jerez curroje...@riseup.net


pgpqYbaUbei8x.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

nouveau page_flip function implement not wait vblank, which cause screen garbage

[Francisco Jerez]

Thomas Hellstrom  writes:

> FWIW, there was a quite long discussion / argument when the page flip
> ioctl was designed, and at that time
> I pointed out that there are hardware capable of pageflipping using
> the fifo/pipe with optional VSYNC barriers, and that it is actually
> possible to queue up a number of pageflips in the fifo. Not just one.
>
That's the case of the nouveau driver, and it's the reason that we don't
respect the API returning -EBUSY when there's an already scheduled flip
request. IMHO that should be up to the driver, or even better, the IOCTL
could be specified to block in case userspace is requesting more
simultaneous page-flips than the kernel driver can handle, in order to
make the resulting behavior consistent for userspace no matter which
implementation is being used.

> The interface description in drm_mode.h is somewhat different to what
> was agreed upon, namely:
>
> 1) The command submission mechanism should block if a user tries to
> render to a not yet flipped frontbuffer, and that would cause
> rendering problems. For hardware that flips using a fifo / pipe,
> that's not really a problem. Thus, any rendering errors due to
> rendering to a not-yet-flipped frontbuffer is a kernel driver error.
> The user-space app can avoid being blocked waiting using events.
>
Yeah, it would be good to relax this restriction -- the nouveau driver
has never respected it because we'd end up lock-stepping the GPU (we
wouldn't be sending the next batch of commands until the one sent before
the flip had been completely processed), and it's just not necessary
because we take additional measures to make sure that flips and commands
sent to other hardware queues are properly ordered with respect to one
another.

> 2) The interface in itself doesn't require flips to be synced to
> vblanks, as I understand it.
>  However, it should be possible to add a new flag
> DRM_MODE_PAGE_FLIP_SYNC that tries to sync if at all possible.
>
Yes, so the fact that the nouveau pageflip implementation doesn't sync
to vblank before flipping isn't even a bug as it stands.

> /Thomas
>
>
> On 10/27/2011 10:00 AM, chris wrote:
>> I think page_flip ioctl need to realize a synchronous mechanism to control 
>> fresh rate...!!!
>> At 2011-10-25 20:30:39,"Ben Skeggs"  wrote:
>>
>>> On Tue, 2011-10-25 at 14:15 +0200, Francisco Jerez wrote:
>>>  
>>>> Maarten Maathuis  writes:
>>>>
>>>>
>>>>> 2011/10/25 chris:
>>>>>  
>>>>>> Can anyone give a suggestion, is wait-vblank fully implemented in
>>>>>> page_flip() for nouveau drm driver?
>>>>>>
>>>>>>
>>>> It's intentionally not implemented.  The reason is that I wanted to
>>>> support non-vsync'ed vblank as well, and for vsync'ed blits we had to
>>>> think about a different mechanism for vblank synchronization anyway, so
>>>> I figured it didn't make that much sense to force vblank synchronization
>>>> directly from the pageflip ioctl.
>>>>
>>> +1 I deliberately didn't flip 1 bit in the NV50/NVC0 page flipping code
>>> for this as well.  The interface IMO is flawed.  Though, that said, we
>>> really should look at doing something properly for this, a lot of people
>>> do want tear-free goodness.
>>>
>>> Ben.
>>>  
>>>>
>>>>>> At 2011-10-24 14:30:55,chris  wrote:
>>>>>>
>>>>>> Dear,
>>>>>>
>>>>>> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 
>>>>>> kernel
>>>>>> code, git drm 2.4.36.
>>>>>>When I run the vbltest program, it prints  "60HZ"  which indicated the
>>>>>> implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
>>>>>>But when I run modetest with option " -v -s 12:1280x1024" , it prints 
>>>>>> high
>>>>>> fresh rate up to "150 HZ" .  I examing the code , and found that no 
>>>>>> waiting
>>>>>> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>>>>>> screen  produced lots of garbage and blink very much.
>>>>>>
>>>> That's fine if by "garbage" you just mean it's tearing like crazy.
>>>>
>>>>
>>>>>> [...]
>>>>>>
>>>>> It seems to be, the actual page flipping is done by software method
>>>>> (

Re: nouveau page_flip function implement not wait vblank, which cause screen garbage

[Francisco Jerez]

Thomas Hellstrom tho...@shipmail.org writes:

 FWIW, there was a quite long discussion / argument when the page flip
 ioctl was designed, and at that time
 I pointed out that there are hardware capable of pageflipping using
 the fifo/pipe with optional VSYNC barriers, and that it is actually
 possible to queue up a number of pageflips in the fifo. Not just one.

That's the case of the nouveau driver, and it's the reason that we don't
respect the API returning -EBUSY when there's an already scheduled flip
request. IMHO that should be up to the driver, or even better, the IOCTL
could be specified to block in case userspace is requesting more
simultaneous page-flips than the kernel driver can handle, in order to
make the resulting behavior consistent for userspace no matter which
implementation is being used.

 The interface description in drm_mode.h is somewhat different to what
 was agreed upon, namely:

 1) The command submission mechanism should block if a user tries to
 render to a not yet flipped frontbuffer, and that would cause
 rendering problems. For hardware that flips using a fifo / pipe,
 that's not really a problem. Thus, any rendering errors due to
 rendering to a not-yet-flipped frontbuffer is a kernel driver error.
 The user-space app can avoid being blocked waiting using events.

Yeah, it would be good to relax this restriction -- the nouveau driver
has never respected it because we'd end up lock-stepping the GPU (we
wouldn't be sending the next batch of commands until the one sent before
the flip had been completely processed), and it's just not necessary
because we take additional measures to make sure that flips and commands
sent to other hardware queues are properly ordered with respect to one
another.

 2) The interface in itself doesn't require flips to be synced to
 vblanks, as I understand it.
  However, it should be possible to add a new flag
 DRM_MODE_PAGE_FLIP_SYNC that tries to sync if at all possible.

Yes, so the fact that the nouveau pageflip implementation doesn't sync
to vblank before flipping isn't even a bug as it stands.

 /Thomas


 On 10/27/2011 10:00 AM, chris wrote:
 I think page_flip ioctl need to realize a synchronous mechanism to control 
 fresh rate...!!!
 At 2011-10-25 20:30:39,Ben Skeggsskeg...@gmail.com  wrote:

 On Tue, 2011-10-25 at 14:15 +0200, Francisco Jerez wrote:
  
 Maarten Maathuismadman2...@gmail.com  writes:


 2011/10/25 chriswwzbw...@163.com:
  
 Can anyone give a suggestion, is wait-vblank fully implemented in
 page_flip() for nouveau drm driver?


 It's intentionally not implemented.  The reason is that I wanted to
 support non-vsync'ed vblank as well, and for vsync'ed blits we had to
 think about a different mechanism for vblank synchronization anyway, so
 I figured it didn't make that much sense to force vblank synchronization
 directly from the pageflip ioctl.

 +1 I deliberately didn't flip 1 bit in the NV50/NVC0 page flipping code
 for this as well.  The interface IMO is flawed.  Though, that said, we
 really should look at doing something properly for this, a lot of people
 do want tear-free goodness.

 Ben.
  

 At 2011-10-24 14:30:55,chriswwzbw...@163.com  wrote:

 Dear,

 I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 
 kernel
 code, git drm 2.4.36.
When I run the vbltest program, it prints  60HZ  which indicated the
 implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
But when I run modetest with option  -v -s 12:1280x1024 , it prints 
 high
 fresh rate up to 150 HZ .  I examing the code , and found that no 
 waiting
 vblank operation is processed in nouveau_crtc_ page_flip() function. The
 screen  produced lots of garbage and blink very much.

 That's fine if by garbage you just mean it's tearing like crazy.


 [...]

 It seems to be, the actual page flipping is done by software method
 (see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
 and that is that we wait for the rendering to be done to the current
 frontbuffer and not the current backbuffer (this is only done if the
 page flip channel is different than the rendering channel). Maybe
 someone else can comment on that.
  
 There's no need to wait for the backbuffer rendering to end because the
 pageflip is always pushed through the last channel that has queued
 rendering to it, so, the waiting is actually done by the GPU. The
 waiting to the current frontbuffer (which in most cases is going to be a
 cross-channel barrier instead of actual CPU waiting) is necessary for
 the (rare) case where you have several channels trying to render to the
 same pageflipped drawable, to make sure that the flips are properly
 synchronized with respect each other.
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

nouveau page_flip function implement not wait vblank, which cause screen garbage

[Francisco Jerez]

Maarten Maathuis  writes:

> 2011/10/25 chris :
>> Can anyone give a suggestion, is wait-vblank fully implemented in
>> page_flip() for nouveau drm driver?
>>

It's intentionally not implemented.  The reason is that I wanted to
support non-vsync'ed vblank as well, and for vsync'ed blits we had to
think about a different mechanism for vblank synchronization anyway, so
I figured it didn't make that much sense to force vblank synchronization
directly from the pageflip ioctl.

>>
>> At 2011-10-24 14:30:55,chris? wrote:
>>
>> Dear,
>>
>> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel
>> code, git drm 2.4.36.
>> ? When I run the vbltest program, it prints? "60HZ"? which indicated the
>> implementation of drmWaitVBlank() and drm_vblank_wait()? is correct.
>> ? But when I run modetest with option " -v -s 12:1280x1024" , it prints high
>> fresh rate up to "150 HZ" .? I examing the code , and found that no waiting
>> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>> screen? produced lots of garbage and blink very much.

That's fine if by "garbage" you just mean it's tearing like crazy.

>>[...]
>
> It seems to be, the actual page flipping is done by software method
> (see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
> and that is that we wait for the rendering to be done to the current
> frontbuffer and not the current backbuffer (this is only done if the
> page flip channel is different than the rendering channel). Maybe
> someone else can comment on that.

There's no need to wait for the backbuffer rendering to end because the
pageflip is always pushed through the last channel that has queued
rendering to it, so, the "waiting" is actually done by the GPU. The
waiting to the current frontbuffer (which in most cases is going to be a
cross-channel barrier instead of actual CPU waiting) is necessary for
the (rare) case where you have several channels trying to render to the
same pageflipped drawable, to make sure that the flips are properly
synchronized with respect each other.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: nouveau page_flip function implement not wait vblank, which cause screen garbage

[Francisco Jerez]

Maarten Maathuis madman2...@gmail.com writes:

 2011/10/25 chris wwzbw...@163.com:
 Can anyone give a suggestion, is wait-vblank fully implemented in
 page_flip() for nouveau drm driver?


It's intentionally not implemented.  The reason is that I wanted to
support non-vsync'ed vblank as well, and for vsync'ed blits we had to
think about a different mechanism for vblank synchronization anyway, so
I figured it didn't make that much sense to force vblank synchronization
directly from the pageflip ioctl.


 At 2011-10-24 14:30:55,chris wwzbw...@163.com wrote:

 Dear,

 I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel
 code, git drm 2.4.36.
   When I run the vbltest program, it prints  60HZ  which indicated the
 implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
   But when I run modetest with option  -v -s 12:1280x1024 , it prints high
 fresh rate up to 150 HZ .  I examing the code , and found that no waiting
 vblank operation is processed in nouveau_crtc_ page_flip() function. The
 screen  produced lots of garbage and blink very much.

That's fine if by garbage you just mean it's tearing like crazy.

[...]

 It seems to be, the actual page flipping is done by software method
 (see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
 and that is that we wait for the rendering to be done to the current
 frontbuffer and not the current backbuffer (this is only done if the
 page flip channel is different than the rendering channel). Maybe
 someone else can comment on that.

There's no need to wait for the backbuffer rendering to end because the
pageflip is always pushed through the last channel that has queued
rendering to it, so, the waiting is actually done by the GPU. The
waiting to the current frontbuffer (which in most cases is going to be a
cross-channel barrier instead of actual CPU waiting) is necessary for
the (rare) case where you have several channels trying to render to the
same pageflipped drawable, to make sure that the flips are properly
synchronized with respect each other.


pgpGCtyUWX8Et.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/nouveau: Only select ACPI_VIDEO if its dependencies are met

[Francisco Jerez]

Ben Hutchings  writes:

> CONFIG_ACPI_VIDEO depends on more than just CONFIG_ACPI, so add those
> dependencies to the Kconfig select condition and make the code
> conditional on CONFIG_ACPI_VIDEO.
>
> Signed-off-by: Ben Hutchings 
> ---
> On Sat, 2010-12-25 at 16:21 +0100, Francisco Jerez wrote:
> [...]
>> > --- a/drivers/gpu/drm/nouveau/Makefile
>> > +++ b/drivers/gpu/drm/nouveau/Makefile
>> > @@ -30,6 +30,6 @@ nouveau-y := nouveau_drv.o nouveau_state.o 
>> > nouveau_channel.o nouveau_mem.o \
>> >  nouveau-$(CONFIG_DRM_NOUVEAU_DEBUG) += nouveau_debugfs.o
>> >  nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
>> >  nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
>> > -nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
>> > +nouveau-$(CONFIG_ACPI_VIDEO) += nouveau_acpi.o
>> >  
>> Not sure this makes sense, most of the code in nouveau_acpi.c doesn't
>> depend on ACPI_VIDEO at all. Do you really need to do it? Apparently all
>> the ACPI_VIDEO functions will be turned into stubs (see "acpi/video.h")
>> on kernels without ACPI_VIDEO support.
> [...]
>
> You're right; there's no need for the changes outside of Kconfig.
>
Thanks, I've pushed this to the Nouveau kernel tree, with a small
clarification to the commit message.

> Ben.
>
>  drivers/gpu/drm/nouveau/Kconfig |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 72730e9..21d6c29 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -10,7 +10,7 @@ config DRM_NOUVEAU
>   select FB
>   select FRAMEBUFFER_CONSOLE if !EMBEDDED
>   select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
> - select ACPI_VIDEO if ACPI
> + select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && 
> VIDEO_OUTPUT_CONTROL && INPUT
>   help
> Choose this option for open-source nVidia support.
>  
> -- 
> 1.7.2.3
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20101225/f21741f7/attachment-0001.pgp>

Re: [PATCH] drm/nouveau: Only select ACPI_VIDEO if its dependencies are met

[Francisco Jerez]

Ben Hutchings b...@decadent.org.uk writes:

 CONFIG_ACPI_VIDEO depends on more than just CONFIG_ACPI, so add those
 dependencies to the Kconfig select condition and make the code
 conditional on CONFIG_ACPI_VIDEO.

 Fixes building for ia64 (ACPI  !X86).

 Signed-off-by: Ben Hutchings b...@decadent.org.uk

First, I'm sorry for the late reply.
 ---
 Please send this up to Linus for 2.6.37.

 Ben.

  drivers/gpu/drm/nouveau/Kconfig |2 +-
  drivers/gpu/drm/nouveau/Makefile|2 +-
  drivers/gpu/drm/nouveau/nouveau_backlight.c |2 +-
  drivers/gpu/drm/nouveau/nouveau_drv.h   |2 +-
  4 files changed, 4 insertions(+), 4 deletions(-)

 diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
 index 72730e9..21d6c29 100644
 --- a/drivers/gpu/drm/nouveau/Kconfig
 +++ b/drivers/gpu/drm/nouveau/Kconfig
 @@ -10,7 +10,7 @@ config DRM_NOUVEAU
   select FB
   select FRAMEBUFFER_CONSOLE if !EMBEDDED
   select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
 - select ACPI_VIDEO if ACPI
 + select ACPI_VIDEO if ACPI  X86  BACKLIGHT_CLASS_DEVICE  
 VIDEO_OUTPUT_CONTROL  INPUT
   help
 Choose this option for open-source nVidia support.
  
This looks OK to me.

 diff --git a/drivers/gpu/drm/nouveau/Makefile 
 b/drivers/gpu/drm/nouveau/Makefile
 index 23fa82d..101a0f6 100644
 --- a/drivers/gpu/drm/nouveau/Makefile
 +++ b/drivers/gpu/drm/nouveau/Makefile
 @@ -30,6 +30,6 @@ nouveau-y := nouveau_drv.o nouveau_state.o 
 nouveau_channel.o nouveau_mem.o \
  nouveau-$(CONFIG_DRM_NOUVEAU_DEBUG) += nouveau_debugfs.o
  nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
  nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
 -nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
 +nouveau-$(CONFIG_ACPI_VIDEO) += nouveau_acpi.o
  
Not sure this makes sense, most of the code in nouveau_acpi.c doesn't
depend on ACPI_VIDEO at all. Do you really need to do it? Apparently all
the ACPI_VIDEO functions will be turned into stubs (see acpi/video.h)
on kernels without ACPI_VIDEO support.

  obj-$(CONFIG_DRM_NOUVEAU)+= nouveau.o
 diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c 
 b/drivers/gpu/drm/nouveau/nouveau_backlight.c
 index b14c811..6f3f463 100644
 --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
 +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
 @@ -137,7 +137,7 @@ int nouveau_backlight_init(struct drm_device *dev)
  {
   struct drm_nouveau_private *dev_priv = dev-dev_private;
  
 -#ifdef CONFIG_ACPI
 +#ifdef CONFIG_ACPI_VIDEO
   if (acpi_video_backlight_support()) {
   NV_INFO(dev, ACPI backlight interface available, 
not registering our own\n);
 diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
 b/drivers/gpu/drm/nouveau/nouveau_drv.h
 index 1c7db64..a18d0ed 100644
 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
 +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
 @@ -894,7 +894,7 @@ extern int  nouveau_dma_wait(struct nouveau_channel *, 
 int slots, int size);
  
  /* nouveau_acpi.c */
  #define ROM_BIOS_PAGE 4096
 -#if defined(CONFIG_ACPI)
 +#if defined(CONFIG_ACPI_VIDEO)
  void nouveau_register_dsm_handler(void);
  void nouveau_unregister_dsm_handler(void);
  int nouveau_acpi_get_bios_chunk(uint8_t *bios, int offset, int len);


pgpgxlxxS9r8a.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/nouveau: Only select ACPI_VIDEO if its dependencies are met

[Francisco Jerez]

Ben Hutchings b...@decadent.org.uk writes:

 CONFIG_ACPI_VIDEO depends on more than just CONFIG_ACPI, so add those
 dependencies to the Kconfig select condition and make the code
 conditional on CONFIG_ACPI_VIDEO.

 Signed-off-by: Ben Hutchings b...@decadent.org.uk
 ---
 On Sat, 2010-12-25 at 16:21 +0100, Francisco Jerez wrote:
 [...]
  --- a/drivers/gpu/drm/nouveau/Makefile
  +++ b/drivers/gpu/drm/nouveau/Makefile
  @@ -30,6 +30,6 @@ nouveau-y := nouveau_drv.o nouveau_state.o 
  nouveau_channel.o nouveau_mem.o \
   nouveau-$(CONFIG_DRM_NOUVEAU_DEBUG) += nouveau_debugfs.o
   nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
   nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
  -nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
  +nouveau-$(CONFIG_ACPI_VIDEO) += nouveau_acpi.o
   
 Not sure this makes sense, most of the code in nouveau_acpi.c doesn't
 depend on ACPI_VIDEO at all. Do you really need to do it? Apparently all
 the ACPI_VIDEO functions will be turned into stubs (see acpi/video.h)
 on kernels without ACPI_VIDEO support.
 [...]

 You're right; there's no need for the changes outside of Kconfig.

Thanks, I've pushed this to the Nouveau kernel tree, with a small
clarification to the commit message.

 Ben.

  drivers/gpu/drm/nouveau/Kconfig |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
 index 72730e9..21d6c29 100644
 --- a/drivers/gpu/drm/nouveau/Kconfig
 +++ b/drivers/gpu/drm/nouveau/Kconfig
 @@ -10,7 +10,7 @@ config DRM_NOUVEAU
   select FB
   select FRAMEBUFFER_CONSOLE if !EMBEDDED
   select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
 - select ACPI_VIDEO if ACPI
 + select ACPI_VIDEO if ACPI  X86  BACKLIGHT_CLASS_DEVICE  
 VIDEO_OUTPUT_CONTROL  INPUT
   help
 Choose this option for open-source nVidia support.
  
 -- 
 1.7.2.3


pgpKSp5sgy9cw.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Nouveau fences?

[Francisco Jerez]

Thomas Hellstrom tho...@shipmail.org writes:

 Ben,

 I'm looking at a way to make TTM memory management asynchronous with
 the CPU. The idea is that you should basically be able to DMA data to
 and from memory regions without waiting for idle, as long as the GPU
 has a means to provide operation ordering.

Sounds good. I guess you're mainly dealing with BO eviction
synchronization? The only problem I see on our side is that calls to our
move() hook aren't guaranteed to be carried out in order (because of the
multiple hardware channels). I'm thinking that move() could be extended
with an optional sync_obj argument, that way move() would be able to
make sure that evictions are strictly ordered with respect to the fence
specified.

 While doing that I looked a bit at the Nouveau fencing. It appears
 like waiting for fences is polling only (no irq to signal fences)? Is
 that correct?

That's right, nvidia hardware has no nice way to schedule a fence-like
interrupt we could selectively turn on and off around the sync_obj_wait
hook. There's a bunch of (more or less) chipset-specific hacks that
could be used to get an equivalent effect, but polling has seemed good
enough so far (in the typical case we only take the lazy path so CPU
usage is still OK).

Unconditional PFIFO CACHE interrupts might be an option too, but, I'm a
bit afraid of the PFIFO stalls and useless IRQ storms some applications
could trigger.

 /Thomas

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


pgp9lLB49LUVh.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Nouveau fences?

[Francisco Jerez]

Thomas Hellstrom  writes:

> On 11/28/2010 05:11 PM, Francisco Jerez wrote:
>> Francisco Jerez  writes:
>>
>>
>>> Thomas Hellstrom  writes:
>>>
>>>  
>>>> Ben,
>>>>
>>>> I'm looking at a way to make TTM memory management asynchronous with
>>>> the CPU. The idea is that you should basically be able to DMA data to
>>>> and from memory regions without waiting for idle, as long as the GPU
>>>> has a means to provide operation ordering.
>>>>
>>>>
>>> Sounds good. I guess you're mainly dealing with BO eviction
>>> synchronization? The only problem I see on our side is that calls to our
>>> move() hook aren't guaranteed to be carried out in order (because of the
>>> multiple hardware channels). I'm thinking that move() could be extended
>>> with an optional sync_obj argument, that way move() would be able to
>>> make sure that evictions are strictly ordered with respect to the fence
>>> specified.
>>>  
> The way evictions will work is that they appear to take place
> "instantly", but are scheduled on a channel, and there will be a data
> structure that keeps track about what fences need to be signaled
> before a managed area can be reused.
>
> The driver will need to provide a function that, given a list of
> fences, returns a fence that when it signals, guarantees that all
> other fences in the list have signaled.

Ah, so, evictions made in response to ttm_bo_mem_force_space() are still
going to be synchronous after the changes you have in mind (because in
that case you need to reuse the freed memory immediately), right?

In other cases (e.g. evictions triggered by BO validation), what exactly
would we gain from this function? I mean, why can't we just push waiting
down to ttm_bo_move_ttm/memcpy?

> Single-channel hardware will just return the fence with the highest
> sequence. Multi-channel hardware may need to insert command stream
> barriers if available and create a new sync object to return or resort
> to simply waiting to determine which fence signals last.
>
> I guess Nouveau can do command stream barriers, (waiting for other
> channels to reach a certain command before progressing?)
>
Yep, that's what nouveau_fence_sync() does.

> Needless to say, drivers need not activate async operation if they
> don't want to, but for single-channel hardware it will hopefully be
> very simple.
>
>
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20101128/7f5e5869/attachment.pgp>

Nouveau fences?

[Francisco Jerez]

Francisco Jerez  writes:

> Thomas Hellstrom  writes:
>
>> Ben,
>>
>> I'm looking at a way to make TTM memory management asynchronous with
>> the CPU. The idea is that you should basically be able to DMA data to
>> and from memory regions without waiting for idle, as long as the GPU
>> has a means to provide operation ordering.
>>
> Sounds good. I guess you're mainly dealing with BO eviction
> synchronization? The only problem I see on our side is that calls to our
> move() hook aren't guaranteed to be carried out in order (because of the
> multiple hardware channels). I'm thinking that move() could be extended
> with an optional sync_obj argument, that way move() would be able to
> make sure that evictions are strictly ordered with respect to the fence
> specified.
>
>> While doing that I looked a bit at the Nouveau fencing. It appears
>> like waiting for fences is polling only (no irq to signal fences)? Is
>> that correct?
>>
> That's right, nvidia hardware has no nice way to schedule a fence-like
> interrupt we could selectively turn on and off around the sync_obj_wait
> hook. There's a bunch of (more or less) chipset-specific hacks that
> could be used to get an equivalent effect, but polling has seemed good
> enough so far (in the typical case we only take the "lazy" path so CPU
> usage is still OK).
>
> Unconditional PFIFO CACHE interrupts might be an option too, but, I'm a
> bit afraid of the PFIFO stalls and useless IRQ storms some applications
> could trigger.
>
Meh, apparently this one couldn't make it through, some spam filter has
decided I'm a spammer for some reason...

>> /Thomas
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20101128/d42bc1be/attachment.pgp>

Nouveau fences?

[Francisco Jerez]

Thomas Hellstrom  writes:

> Ben,
>
> I'm looking at a way to make TTM memory management asynchronous with
> the CPU. The idea is that you should basically be able to DMA data to
> and from memory regions without waiting for idle, as long as the GPU
> has a means to provide operation ordering.
>
Sounds good. I guess you're mainly dealing with BO eviction
synchronization? The only problem I see on our side is that calls to our
move() hook aren't guaranteed to be carried out in order (because of the
multiple hardware channels). I'm thinking that move() could be extended
with an optional sync_obj argument, that way move() would be able to
make sure that evictions are strictly ordered with respect to the fence
specified.

> While doing that I looked a bit at the Nouveau fencing. It appears
> like waiting for fences is polling only (no irq to signal fences)? Is
> that correct?
>
That's right, nvidia hardware has no nice way to schedule a fence-like
interrupt we could selectively turn on and off around the sync_obj_wait
hook. There's a bunch of (more or less) chipset-specific hacks that
could be used to get an equivalent effect, but polling has seemed good
enough so far (in the typical case we only take the "lazy" path so CPU
usage is still OK).

Unconditional PFIFO CACHE interrupts might be an option too, but, I'm a
bit afraid of the PFIFO stalls and useless IRQ storms some applications
could trigger.

> /Thomas
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: Nouveau fences?

[Francisco Jerez]

Francisco Jerez curroje...@riseup.net writes:

 Thomas Hellstrom tho...@shipmail.org writes:

 Ben,

 I'm looking at a way to make TTM memory management asynchronous with
 the CPU. The idea is that you should basically be able to DMA data to
 and from memory regions without waiting for idle, as long as the GPU
 has a means to provide operation ordering.

 Sounds good. I guess you're mainly dealing with BO eviction
 synchronization? The only problem I see on our side is that calls to our
 move() hook aren't guaranteed to be carried out in order (because of the
 multiple hardware channels). I'm thinking that move() could be extended
 with an optional sync_obj argument, that way move() would be able to
 make sure that evictions are strictly ordered with respect to the fence
 specified.

 While doing that I looked a bit at the Nouveau fencing. It appears
 like waiting for fences is polling only (no irq to signal fences)? Is
 that correct?

 That's right, nvidia hardware has no nice way to schedule a fence-like
 interrupt we could selectively turn on and off around the sync_obj_wait
 hook. There's a bunch of (more or less) chipset-specific hacks that
 could be used to get an equivalent effect, but polling has seemed good
 enough so far (in the typical case we only take the lazy path so CPU
 usage is still OK).

 Unconditional PFIFO CACHE interrupts might be an option too, but, I'm a
 bit afraid of the PFIFO stalls and useless IRQ storms some applications
 could trigger.

Meh, apparently this one couldn't make it through, some spam filter has
decided I'm a spammer for some reason...

 /Thomas

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


pgpXpqdjknr8j.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Nouveau fences?

[Francisco Jerez]

Thomas Hellstrom tho...@shipmail.org writes:

 On 11/28/2010 05:11 PM, Francisco Jerez wrote:
 Francisco Jerezcurroje...@riseup.net  writes:


 Thomas Hellstromtho...@shipmail.org  writes:

  
 Ben,

 I'm looking at a way to make TTM memory management asynchronous with
 the CPU. The idea is that you should basically be able to DMA data to
 and from memory regions without waiting for idle, as long as the GPU
 has a means to provide operation ordering.


 Sounds good. I guess you're mainly dealing with BO eviction
 synchronization? The only problem I see on our side is that calls to our
 move() hook aren't guaranteed to be carried out in order (because of the
 multiple hardware channels). I'm thinking that move() could be extended
 with an optional sync_obj argument, that way move() would be able to
 make sure that evictions are strictly ordered with respect to the fence
 specified.
  
 The way evictions will work is that they appear to take place
 instantly, but are scheduled on a channel, and there will be a data
 structure that keeps track about what fences need to be signaled
 before a managed area can be reused.

 The driver will need to provide a function that, given a list of
 fences, returns a fence that when it signals, guarantees that all
 other fences in the list have signaled.

Ah, so, evictions made in response to ttm_bo_mem_force_space() are still
going to be synchronous after the changes you have in mind (because in
that case you need to reuse the freed memory immediately), right?

In other cases (e.g. evictions triggered by BO validation), what exactly
would we gain from this function? I mean, why can't we just push waiting
down to ttm_bo_move_ttm/memcpy?

 Single-channel hardware will just return the fence with the highest
 sequence. Multi-channel hardware may need to insert command stream
 barriers if available and create a new sync object to return or resort
 to simply waiting to determine which fence signals last.

 I guess Nouveau can do command stream barriers, (waiting for other
 channels to reach a certain command before progressing?)

Yep, that's what nouveau_fence_sync() does.

 Needless to say, drivers need not activate async operation if they
 don't want to, but for single-channel hardware it will hopefully be
 very simple.




pgprxFT6f4uZl.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[BUG?] [Nouveau] INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected

[Francisco Jerez]

Arun Bhanu  writes:

> I am seeing the following in kernel log messages every time I reboot.
> I am running 2.6.37-rc2.
> (commit 589136bfa784a4558b397f017ca2f06f0ca9080e).
>
> Please let me know if you need more info or want me to test any
> patches.
>
> [ 1043.994049] ==
> [ 1043.995596] [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
> [ 1043.996203] 2.6.37-rc2-ab3-589136bfa784a4558b397f017ca2f06f0ca9080e+ #2
> [ 1043.996817] --
> [ 1043.997432] Xorg/2097 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> [ 1043.998038]  (&(_priv->ramin_lock)->rlock){+.+...}, at: [] 
> nouveau_gpuobj_del+0xb8/0xe2 [nouveau]
> [ 1043.998658] 
> [ 1043.998658] and this task is already holding:
> [ 1043.999891]  (&(>lock)->rlock){-.}, at: [] 
> nouveau_ramht_remove+0x23/0x3e [nouveau]
> [ 1043.06] which would create a new lock dependency:
> [ 1043.07]  (&(>lock)->rlock){-.} -> 
> (&(_priv->ramin_lock)->rlock){+.+...}
> [ 1043.13] 
> [ 1043.13] but this new dependency connects a HARDIRQ-irq-safe lock:
> [ 1043.14]  (&(>lock)->rlock){-.}
> [ 1043.16] ... which became HARDIRQ-irq-safe at:
> [ 1043.17]   [] __lock_acquire+0x27a/0xc06
> [ 1043.21]   [] lock_acquire+0xbc/0xdc
> [ 1043.23]   [] _raw_spin_lock_irqsave+0x48/0x78
> [ 1043.27]   [] nouveau_ramht_find+0x29/0x6d [nouveau]
> [ 1043.41]   [] nouveau_irq_handler+0x16a/0x17aa [nouveau]
> [ 1043.53]   [] handle_IRQ_event+0x51/0x10f
> [ 1043.56]   [] handle_fasteoi_irq+0x95/0xcc
> [ 1043.59] 
> [ 1043.60] to a HARDIRQ-irq-unsafe lock:
> [ 1043.61]  (&(_priv->ramin_lock)->rlock){+.+...}
> [ 1043.62] ... which became HARDIRQ-irq-unsafe at:
> [ 1043.63] ...  [] __lock_acquire+0x2ee/0xc06
> [ 1043.66]   [] lock_acquire+0xbc/0xdc
> [ 1043.68]   [] _raw_spin_lock+0x3b/0x68
> [ 1043.70]   [] nouveau_gpuobj_new+0x129/0x39b [nouveau]
> [ 1043.81]   [] nv50_instmem_init+0x11a/0x7db [nouveau]
> [ 1043.96]   [] nouveau_card_init+0xeda/0x1214 [nouveau]
> [ 1044.07]   [] nouveau_load+0x6e7/0x73e [nouveau]
> [ 1044.17]   [] drm_get_pci_dev+0x165/0x257 [drm]
> [ 1044.29]   [] nouveau_pci_probe+0x12/0x14 [nouveau]
> [ 1044.43]   [] local_pci_probe+0x34/0x5f
> [ 1044.46]   [] pci_device_probe+0x4d/0x70
> [ 1044.48]   [] driver_probe_device+0x119/0x1e9
> [ 1044.52]   [] __driver_attach+0x44/0x60
> [ 1044.53]   [] bus_for_each_dev+0x42/0x65
> [ 1044.55]   [] driver_attach+0x1e/0x20
> [ 1044.57]   [] bus_add_driver+0xc0/0x218
> [ 1044.59]   [] driver_register+0x84/0xe3
> [ 1044.61]   [] __pci_register_driver+0x51/0xae
> [ 1044.63]   [] drm_pci_init+0x37/0x96 [drm]
> [ 1044.74]   [] drm_init+0x5c/0x5e [drm]
> [ 1044.83]   [] 0xf08e4042
> [ 1044.86]   [] do_one_initcall+0x8c/0x146
> [ 1044.88]   [] sys_init_module+0x12e9/0x1486
> [ 1044.91]   [] sysenter_do_call+0x12/0x38
> [ 1044.94] 
> [ 1044.95] other info that might help us debug this:
> [ 1044.96] 
> [ 1044.97] 2 locks held by Xorg/2097:
> [ 1044.98]  #0:  (drm_global_mutex){+.+.+.}, at: [] 
> drm_ioctl+0x2f1/0x3c5 [drm]
> [ 1044.000107]  #1:  (&(>lock)->rlock){-.}, at: [] 
> nouveau_ramht_remove+0x23/0x3e [nouveau]
> [ 1044.000123] 
> [ 1044.000123] the dependencies between HARDIRQ-irq-safe lock and the holding 
> lock:
> [ 1044.000147] -> (&(>lock)->rlock){-.} ops: 61 {
> [ 1044.000149]IN-HARDIRQ-W at:
> [ 1044.000150][] 
> __lock_acquire+0x27a/0xc06
> [ 1044.000154][] 
> lock_acquire+0xbc/0xdc
> [ 1044.000156][] 
> _raw_spin_lock_irqsave+0x48/0x78
> [ 1044.000159][] 
> nouveau_ramht_find+0x29/0x6d [nouveau]
> [ 1044.000173][] 
> nouveau_irq_handler+0x16a/0x17aa [nouveau]
> [ 1044.000186][] 
> handle_IRQ_event+0x51/0x10f
> [ 1044.000189][] 
> handle_fasteoi_irq+0x95/0xcc
> [ 1044.000192]INITIAL USE at:
> [ 1044.000193]   [] 
> __lock_acquire+0x362/0xc06
> [ 1044.000196]   [] 
> lock_acquire+0xbc/0xdc
> [ 1044.000199]   [] 
> _raw_spin_lock_irqsave+0x48/0x78
> [ 1044.000201]   [] 
> nouveau_ramht_find+0x29/0x6d [nouveau]
> [ 1044.000216]   [] 
> nouveau_ramht_insert+0x39/0x2dd [nouveau]
> [ 1044.000230]   [] 
> nv50_evo_dmaobj_new.clone.4+0xe7/0xfd [nouveau]
> [ 1044.000246]   [] 
> nv50_display_create+0x289/0x5bd [nouveau]
> [ 1044.000262]   [] 
> 

Re: [BUG?] [Nouveau] INFO: HARDIRQ-safe - HARDIRQ-unsafe lock order detected

[Francisco Jerez]

Arun Bhanu a...@arunbhanu.com writes:

 I am seeing the following in kernel log messages every time I reboot.
 I am running 2.6.37-rc2.
 (commit 589136bfa784a4558b397f017ca2f06f0ca9080e).

 Please let me know if you need more info or want me to test any
 patches.

 [ 1043.994049] ==
 [ 1043.995596] [ INFO: HARDIRQ-safe - HARDIRQ-unsafe lock order detected ]
 [ 1043.996203] 2.6.37-rc2-ab3-589136bfa784a4558b397f017ca2f06f0ca9080e+ #2
 [ 1043.996817] --
 [ 1043.997432] Xorg/2097 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
 [ 1043.998038]  ((dev_priv-ramin_lock)-rlock){+.+...}, at: [f0887014] 
 nouveau_gpuobj_del+0xb8/0xe2 [nouveau]
 [ 1043.998658] 
 [ 1043.998658] and this task is already holding:
 [ 1043.999891]  ((ramht-lock)-rlock){-.}, at: [f089fe4f] 
 nouveau_ramht_remove+0x23/0x3e [nouveau]
 [ 1043.06] which would create a new lock dependency:
 [ 1043.07]  ((ramht-lock)-rlock){-.} - 
 ((dev_priv-ramin_lock)-rlock){+.+...}
 [ 1043.13] 
 [ 1043.13] but this new dependency connects a HARDIRQ-irq-safe lock:
 [ 1043.14]  ((ramht-lock)-rlock){-.}
 [ 1043.16] ... which became HARDIRQ-irq-safe at:
 [ 1043.17]   [c0472027] __lock_acquire+0x27a/0xc06
 [ 1043.21]   [c0472e10] lock_acquire+0xbc/0xdc
 [ 1043.23]   [c0824efb] _raw_spin_lock_irqsave+0x48/0x78
 [ 1043.27]   [f089fe93] nouveau_ramht_find+0x29/0x6d [nouveau]
 [ 1043.41]   [f0888e74] nouveau_irq_handler+0x16a/0x17aa [nouveau]
 [ 1043.53]   [c049da82] handle_IRQ_event+0x51/0x10f
 [ 1043.56]   [c049f7bb] handle_fasteoi_irq+0x95/0xcc
 [ 1043.59] 
 [ 1043.60] to a HARDIRQ-irq-unsafe lock:
 [ 1043.61]  ((dev_priv-ramin_lock)-rlock){+.+...}
 [ 1043.62] ... which became HARDIRQ-irq-unsafe at:
 [ 1043.63] ...  [c047209b] __lock_acquire+0x2ee/0xc06
 [ 1043.66]   [c0472e10] lock_acquire+0xbc/0xdc
 [ 1043.68]   [c0824d98] _raw_spin_lock+0x3b/0x68
 [ 1043.70]   [f0887167] nouveau_gpuobj_new+0x129/0x39b [nouveau]
 [ 1043.81]   [f08b583f] nv50_instmem_init+0x11a/0x7db [nouveau]
 [ 1043.96]   [f088371c] nouveau_card_init+0xeda/0x1214 [nouveau]
 [ 1044.07]   [f0884158] nouveau_load+0x6e7/0x73e [nouveau]
 [ 1044.17]   [ef819b8f] drm_get_pci_dev+0x165/0x257 [drm]
 [ 1044.29]   [f08c5c12] nouveau_pci_probe+0x12/0x14 [nouveau]
 [ 1044.43]   [c060c794] local_pci_probe+0x34/0x5f
 [ 1044.46]   [c060cc70] pci_device_probe+0x4d/0x70
 [ 1044.48]   [c06b1a9a] driver_probe_device+0x119/0x1e9
 [ 1044.52]   [c06b1bae] __driver_attach+0x44/0x60
 [ 1044.53]   [c06b0c44] bus_for_each_dev+0x42/0x65
 [ 1044.55]   [c06b1707] driver_attach+0x1e/0x20
 [ 1044.57]   [c06b1369] bus_add_driver+0xc0/0x218
 [ 1044.59]   [c06b1d9d] driver_register+0x84/0xe3
 [ 1044.61]   [c060ce6d] __pci_register_driver+0x51/0xae
 [ 1044.63]   [ef819f11] drm_pci_init+0x37/0x96 [drm]
 [ 1044.74]   [ef8135ae] drm_init+0x5c/0x5e [drm]
 [ 1044.83]   [f08e4042] 0xf08e4042
 [ 1044.86]   [c0403192] do_one_initcall+0x8c/0x146
 [ 1044.88]   [c047c610] sys_init_module+0x12e9/0x1486
 [ 1044.91]   [c040971f] sysenter_do_call+0x12/0x38
 [ 1044.94] 
 [ 1044.95] other info that might help us debug this:
 [ 1044.96] 
 [ 1044.97] 2 locks held by Xorg/2097:
 [ 1044.98]  #0:  (drm_global_mutex){+.+.+.}, at: [ef8138f8] 
 drm_ioctl+0x2f1/0x3c5 [drm]
 [ 1044.000107]  #1:  ((ramht-lock)-rlock){-.}, at: [f089fe4f] 
 nouveau_ramht_remove+0x23/0x3e [nouveau]
 [ 1044.000123] 
 [ 1044.000123] the dependencies between HARDIRQ-irq-safe lock and the holding 
 lock:
 [ 1044.000147] - ((ramht-lock)-rlock){-.} ops: 61 {
 [ 1044.000149]IN-HARDIRQ-W at:
 [ 1044.000150][c0472027] 
 __lock_acquire+0x27a/0xc06
 [ 1044.000154][c0472e10] 
 lock_acquire+0xbc/0xdc
 [ 1044.000156][c0824efb] 
 _raw_spin_lock_irqsave+0x48/0x78
 [ 1044.000159][f089fe93] 
 nouveau_ramht_find+0x29/0x6d [nouveau]
 [ 1044.000173][f0888e74] 
 nouveau_irq_handler+0x16a/0x17aa [nouveau]
 [ 1044.000186][c049da82] 
 handle_IRQ_event+0x51/0x10f
 [ 1044.000189][c049f7bb] 
 handle_fasteoi_irq+0x95/0xcc
 [ 1044.000192]INITIAL USE at:
 [ 1044.000193]   [c047210f] 
 __lock_acquire+0x362/0xc06
 [ 1044.000196]   [c0472e10] 
 lock_acquire+0xbc/0xdc
 [ 1044.000199]   [c0824efb] 
 _raw_spin_lock_irqsave+0x48/0x78
 [ 1044.000201]   [f089fe93] 
 nouveau_ramht_find+0x29/0x6d [nouveau]
 [ 1044.000216]   [f089ff10] 
 

[PATCH 2/2] drm/kms: Fix missing locking in some fb helpers.

[Francisco Jerez]

Signed-off-by: Francisco Jerez 
---
 drivers/gpu/drm/drm_fb_helper.c |   14 --
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 6a5e403..4d608b7 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1449,6 +1449,8 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
struct drm_device *dev = fb_helper->dev;
int count = 0;

+   mutex_lock(>mode_config.mutex);
+
/* disable all the possible outputs/crtcs before entering KMS mode */
drm_helper_disable_unused_functions(fb_helper->dev);

@@ -1465,19 +1467,23 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
}
drm_setup_crtcs(fb_helper);

+   mutex_unlock(>mode_config.mutex);
+
return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
 }
 EXPORT_SYMBOL(drm_fb_helper_initial_config);

 bool drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
+   struct drm_device *dev = fb_helper->dev;
int count = 0;
u32 max_width, max_height, bpp_sel;
bool bound = false, crtcs_bound = false;
struct drm_crtc *crtc;

+   mutex_lock(>mode_config.mutex);
if (!fb_helper->fb)
-   return false;
+   goto fail;

list_for_each_entry(crtc, _helper->dev->mode_config.crtc_list, head) 
{
if (crtc->fb)
@@ -1488,7 +1494,7 @@ bool drm_fb_helper_hotplug_event(struct drm_fb_helper 
*fb_helper)

if (!bound && crtcs_bound) {
fb_helper->delayed_hotplug = true;
-   return false;
+   goto fail;
}
DRM_DEBUG_KMS("\n");

@@ -1500,7 +1506,11 @@ bool drm_fb_helper_hotplug_event(struct drm_fb_helper 
*fb_helper)
max_height);
drm_setup_crtcs(fb_helper);

+   mutex_unlock(>mode_config.mutex);
return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+fail:
+   mutex_unlock(>mode_config.mutex);
+   return false;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);

-- 
1.6.4.4

[PATCH 1/2] drm/kms: Lock mode_config.mutex consistently from the sysfs handling code.

[Francisco Jerez]

Among other potential issues, this fixes a race between output polling
and status_show() that could cause a load detection false positive
with the nouveau driver.

Signed-off-by: Francisco Jerez 
---
 drivers/gpu/drm/drm_sysfs.c |   71 ---
 1 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 85da4c4..25a9d9e 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -157,9 +157,13 @@ static ssize_t status_show(struct device *device,
   char *buf)
 {
struct drm_connector *connector = to_drm_connector(device);
+   struct drm_device *dev = connector->dev;
enum drm_connector_status status;

+   mutex_lock(>mode_config.mutex);
status = connector->funcs->detect(connector, true);
+   mutex_unlock(>mode_config.mutex);
+
return snprintf(buf, PAGE_SIZE, "%s\n",
drm_get_connector_status_name(status));
 }
@@ -173,9 +177,11 @@ static ssize_t dpms_show(struct device *device,
uint64_t dpms_status;
int ret;

+   mutex_lock(>mode_config.mutex);
ret = drm_connector_property_get_value(connector,
dev->mode_config.dpms_property,
_status);
+   mutex_unlock(>mode_config.mutex);
if (ret)
return 0;

@@ -199,25 +205,27 @@ static ssize_t edid_show(struct file *filp, struct 
kobject *kobj,
 {
struct device *connector_dev = container_of(kobj, struct device, kobj);
struct drm_connector *connector = to_drm_connector(connector_dev);
+   struct drm_device *dev = connector->dev;
unsigned char *edid;
-   size_t size;
+   size_t size, n = 0;

+   mutex_lock(>mode_config.mutex);
if (!connector->edid_blob_ptr)
-   return 0;
+   goto out;

edid = connector->edid_blob_ptr->data;
size = connector->edid_blob_ptr->length;
if (!edid)
-   return 0;
+   goto out;

if (off >= size)
-   return 0;
-
-   if (off + count > size)
-   count = size - off;
-   memcpy(buf, edid + off, count);
+   goto out;

-   return count;
+   n = min(count, size - (size_t)off);
+   memcpy(buf, edid + off, n);
+out:
+   mutex_unlock(>mode_config.mutex);
+   return n;
 }

 static ssize_t modes_show(struct device *device,
@@ -225,13 +233,16 @@ static ssize_t modes_show(struct device *device,
   char *buf)
 {
struct drm_connector *connector = to_drm_connector(device);
+   struct drm_device *dev = connector->dev;
struct drm_display_mode *mode;
int written = 0;

+   mutex_lock(>mode_config.mutex);
list_for_each_entry(mode, >modes, head) {
written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
mode->name);
}
+   mutex_unlock(>mode_config.mutex);

return written;
 }
@@ -245,7 +256,9 @@ static ssize_t subconnector_show(struct device *device,
struct drm_property *prop = NULL;
uint64_t subconnector;
int is_tv = 0;
-   int ret;
+   int ret = 0;
+
+   mutex_lock(>mode_config.mutex);

switch (connector->connector_type) {
case DRM_MODE_CONNECTOR_DVII:
@@ -260,21 +273,24 @@ static ssize_t subconnector_show(struct device *device,
break;
default:
DRM_ERROR("Wrong connector type for this property\n");
-   return 0;
+   goto out;
}

if (!prop) {
DRM_ERROR("Unable to find subconnector property\n");
-   return 0;
+   goto out;
}

ret = drm_connector_property_get_value(connector, prop, );
if (ret)
-   return 0;
+   goto out;

-   return snprintf(buf, PAGE_SIZE, "%s", is_tv ?
-   drm_get_tv_subconnector_name((int)subconnector) :
-   drm_get_dvi_i_subconnector_name((int)subconnector));
+   ret = snprintf(buf, PAGE_SIZE, "%s", is_tv ?
+  drm_get_tv_subconnector_name((int)subconnector) :
+  drm_get_dvi_i_subconnector_name((int)subconnector));
+out:
+   mutex_unlock(>mode_config.mutex);
+   return ret;
 }

 static ssize_t select_subconnector_show(struct device *device,
@@ -286,7 +302,9 @@ static ssize_t select_subconnector_show(struct device 
*device,
struct drm_property *prop = NULL;
uint64_t subconnector;
int is_tv = 0;
-   int ret;
+   int ret = 0;
+
+   mutex_lock(>

[PATCH 1/2] drm/kms: Lock mode_config.mutex consistently from the sysfs handling code.

[Francisco Jerez]

Among other potential issues, this fixes a race between output polling
and status_show() that could cause a load detection false positive
with the nouveau driver.

Signed-off-by: Francisco Jerez curroje...@riseup.net
---
 drivers/gpu/drm/drm_sysfs.c |   71 ---
 1 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 85da4c4..25a9d9e 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -157,9 +157,13 @@ static ssize_t status_show(struct device *device,
   char *buf)
 {
struct drm_connector *connector = to_drm_connector(device);
+   struct drm_device *dev = connector-dev;
enum drm_connector_status status;
 
+   mutex_lock(dev-mode_config.mutex);
status = connector-funcs-detect(connector, true);
+   mutex_unlock(dev-mode_config.mutex);
+
return snprintf(buf, PAGE_SIZE, %s\n,
drm_get_connector_status_name(status));
 }
@@ -173,9 +177,11 @@ static ssize_t dpms_show(struct device *device,
uint64_t dpms_status;
int ret;
 
+   mutex_lock(dev-mode_config.mutex);
ret = drm_connector_property_get_value(connector,
dev-mode_config.dpms_property,
dpms_status);
+   mutex_unlock(dev-mode_config.mutex);
if (ret)
return 0;
 
@@ -199,25 +205,27 @@ static ssize_t edid_show(struct file *filp, struct 
kobject *kobj,
 {
struct device *connector_dev = container_of(kobj, struct device, kobj);
struct drm_connector *connector = to_drm_connector(connector_dev);
+   struct drm_device *dev = connector-dev;
unsigned char *edid;
-   size_t size;
+   size_t size, n = 0;
 
+   mutex_lock(dev-mode_config.mutex);
if (!connector-edid_blob_ptr)
-   return 0;
+   goto out;
 
edid = connector-edid_blob_ptr-data;
size = connector-edid_blob_ptr-length;
if (!edid)
-   return 0;
+   goto out;
 
if (off = size)
-   return 0;
-
-   if (off + count  size)
-   count = size - off;
-   memcpy(buf, edid + off, count);
+   goto out;
 
-   return count;
+   n = min(count, size - (size_t)off);
+   memcpy(buf, edid + off, n);
+out:
+   mutex_unlock(dev-mode_config.mutex);
+   return n;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -225,13 +233,16 @@ static ssize_t modes_show(struct device *device,
   char *buf)
 {
struct drm_connector *connector = to_drm_connector(device);
+   struct drm_device *dev = connector-dev;
struct drm_display_mode *mode;
int written = 0;
 
+   mutex_lock(dev-mode_config.mutex);
list_for_each_entry(mode, connector-modes, head) {
written += snprintf(buf + written, PAGE_SIZE - written, %s\n,
mode-name);
}
+   mutex_unlock(dev-mode_config.mutex);
 
return written;
 }
@@ -245,7 +256,9 @@ static ssize_t subconnector_show(struct device *device,
struct drm_property *prop = NULL;
uint64_t subconnector;
int is_tv = 0;
-   int ret;
+   int ret = 0;
+
+   mutex_lock(dev-mode_config.mutex);
 
switch (connector-connector_type) {
case DRM_MODE_CONNECTOR_DVII:
@@ -260,21 +273,24 @@ static ssize_t subconnector_show(struct device *device,
break;
default:
DRM_ERROR(Wrong connector type for this property\n);
-   return 0;
+   goto out;
}
 
if (!prop) {
DRM_ERROR(Unable to find subconnector property\n);
-   return 0;
+   goto out;
}
 
ret = drm_connector_property_get_value(connector, prop, subconnector);
if (ret)
-   return 0;
+   goto out;
 
-   return snprintf(buf, PAGE_SIZE, %s, is_tv ?
-   drm_get_tv_subconnector_name((int)subconnector) :
-   drm_get_dvi_i_subconnector_name((int)subconnector));
+   ret = snprintf(buf, PAGE_SIZE, %s, is_tv ?
+  drm_get_tv_subconnector_name((int)subconnector) :
+  drm_get_dvi_i_subconnector_name((int)subconnector));
+out:
+   mutex_unlock(dev-mode_config.mutex);
+   return ret;
 }
 
 static ssize_t select_subconnector_show(struct device *device,
@@ -286,7 +302,9 @@ static ssize_t select_subconnector_show(struct device 
*device,
struct drm_property *prop = NULL;
uint64_t subconnector;
int is_tv = 0;
-   int ret;
+   int ret = 0;
+
+   mutex_lock(dev-mode_config.mutex);
 
switch (connector-connector_type

[PATCH 2/2] drm/kms: Fix missing locking in some fb helpers.

[Francisco Jerez]

Signed-off-by: Francisco Jerez curroje...@riseup.net
---
 drivers/gpu/drm/drm_fb_helper.c |   14 --
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 6a5e403..4d608b7 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1449,6 +1449,8 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
struct drm_device *dev = fb_helper-dev;
int count = 0;
 
+   mutex_lock(dev-mode_config.mutex);
+
/* disable all the possible outputs/crtcs before entering KMS mode */
drm_helper_disable_unused_functions(fb_helper-dev);
 
@@ -1465,19 +1467,23 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
}
drm_setup_crtcs(fb_helper);
 
+   mutex_unlock(dev-mode_config.mutex);
+
return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
 }
 EXPORT_SYMBOL(drm_fb_helper_initial_config);
 
 bool drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
+   struct drm_device *dev = fb_helper-dev;
int count = 0;
u32 max_width, max_height, bpp_sel;
bool bound = false, crtcs_bound = false;
struct drm_crtc *crtc;
 
+   mutex_lock(dev-mode_config.mutex);
if (!fb_helper-fb)
-   return false;
+   goto fail;
 
list_for_each_entry(crtc, fb_helper-dev-mode_config.crtc_list, head) 
{
if (crtc-fb)
@@ -1488,7 +1494,7 @@ bool drm_fb_helper_hotplug_event(struct drm_fb_helper 
*fb_helper)
 
if (!bound  crtcs_bound) {
fb_helper-delayed_hotplug = true;
-   return false;
+   goto fail;
}
DRM_DEBUG_KMS(\n);
 
@@ -1500,7 +1506,11 @@ bool drm_fb_helper_hotplug_event(struct drm_fb_helper 
*fb_helper)
max_height);
drm_setup_crtcs(fb_helper);
 
+   mutex_unlock(dev-mode_config.mutex);
return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+fail:
+   mutex_unlock(dev-mode_config.mutex);
+   return false;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
-- 
1.6.4.4

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

[PATCHv2] agp/amd-k7: Allow binding user memory to the AGP GART.

[Francisco Jerez]

TTM-based DRM drivers need to be able to bind user memory to the AGP
aperture. This patch fixes the "[TTM] AGP Bind memory failed." errors
and the subsequent fallout seen with the nouveau driver.

Signed-off-by: Francisco Jerez 
Tested-by: Grzesiek S?jka 
---
v2: "amd_remove_memory" bails out when it's given user memory, fix it too.

 drivers/char/agp/amd-k7-agp.c |6 --
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index b6b1568..b1b4362 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -309,7 +309,8 @@ static int amd_insert_memory(struct agp_memory *mem, off_t 
pg_start, int type)

num_entries = A_SIZE_LVL2(agp_bridge->current_size)->num_entries;

-   if (type != 0 || mem->type != 0)
+   if (type != mem->type ||
+   agp_bridge->driver->agp_type_to_mask_type(agp_bridge, type))
return -EINVAL;

if ((pg_start + mem->page_count) > num_entries)
@@ -348,7 +349,8 @@ static int amd_remove_memory(struct agp_memory *mem, off_t 
pg_start, int type)
unsigned long __iomem *cur_gatt;
unsigned long addr;

-   if (type != 0 || mem->type != 0)
+   if (type != mem->type ||
+   agp_bridge->driver->agp_type_to_mask_type(agp_bridge, type))
return -EINVAL;

for (i = pg_start; i < (mem->page_count + pg_start); i++) {
-- 
1.6.4.4

[PATCHv2] agp/amd-k7: Allow binding user memory to the AGP GART.

[Francisco Jerez]

TTM-based DRM drivers need to be able to bind user memory to the AGP
aperture. This patch fixes the [TTM] AGP Bind memory failed. errors
and the subsequent fallout seen with the nouveau driver.

Signed-off-by: Francisco Jerez curroje...@riseup.net
Tested-by: Grzesiek Sójka p...@pfu.pl
---
v2: amd_remove_memory bails out when it's given user memory, fix it too.

 drivers/char/agp/amd-k7-agp.c |6 --
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index b6b1568..b1b4362 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -309,7 +309,8 @@ static int amd_insert_memory(struct agp_memory *mem, off_t 
pg_start, int type)
 
num_entries = A_SIZE_LVL2(agp_bridge-current_size)-num_entries;
 
-   if (type != 0 || mem-type != 0)
+   if (type != mem-type ||
+   agp_bridge-driver-agp_type_to_mask_type(agp_bridge, type))
return -EINVAL;
 
if ((pg_start + mem-page_count)  num_entries)
@@ -348,7 +349,8 @@ static int amd_remove_memory(struct agp_memory *mem, off_t 
pg_start, int type)
unsigned long __iomem *cur_gatt;
unsigned long addr;
 
-   if (type != 0 || mem-type != 0)
+   if (type != mem-type ||
+   agp_bridge-driver-agp_type_to_mask_type(agp_bridge, type))
return -EINVAL;
 
for (i = pg_start; i  (mem-page_count + pg_start); i++) {
-- 
1.6.4.4

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

[PATCH] agp/amd-k7: Allow binding user memory to the AGP GART.

[Francisco Jerez]

TTM-based DRM drivers need to be able to bind user memory to the AGP
aperture. This patch fixes the "[TTM] AGP Bind memory failed." errors
and the subsequent fallout seen with the nouveau driver.

Reported-by: Grzesiek S?jka 
Signed-off-by: Francisco Jerez 
---
 drivers/char/agp/amd-k7-agp.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index b6b1568..82e5189 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -303,13 +303,15 @@ static void amd_irongate_tlbflush(struct agp_memory *temp)

 static int amd_insert_memory(struct agp_memory *mem, off_t pg_start, int type)
 {
+   struct agp_bridge_data *bridge = mem->bridge;
int i, j, num_entries;
unsigned long __iomem *cur_gatt;
unsigned long addr;

num_entries = A_SIZE_LVL2(agp_bridge->current_size)->num_entries;

-   if (type != 0 || mem->type != 0)
+   if (!bridge || type != mem->type ||
+   bridge->driver->agp_type_to_mask_type(bridge, type))
return -EINVAL;

if ((pg_start + mem->page_count) > num_entries)
-- 
1.6.4.4

[PATCH] agp/amd-k7: Allow binding user memory to the AGP GART.

[Francisco Jerez]

TTM-based DRM drivers need to be able to bind user memory to the AGP
aperture. This patch fixes the [TTM] AGP Bind memory failed. errors
and the subsequent fallout seen with the nouveau driver.

Reported-by: Grzesiek Sójka p...@pfu.pl
Signed-off-by: Francisco Jerez curroje...@riseup.net
---
 drivers/char/agp/amd-k7-agp.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index b6b1568..82e5189 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -303,13 +303,15 @@ static void amd_irongate_tlbflush(struct agp_memory *temp)
 
 static int amd_insert_memory(struct agp_memory *mem, off_t pg_start, int type)
 {
+   struct agp_bridge_data *bridge = mem-bridge;
int i, j, num_entries;
unsigned long __iomem *cur_gatt;
unsigned long addr;
 
num_entries = A_SIZE_LVL2(agp_bridge-current_size)-num_entries;
 
-   if (type != 0 || mem-type != 0)
+   if (!bridge || type != mem-type ||
+   bridge-driver-agp_type_to_mask_type(bridge, type))
return -EINVAL;
 
if ((pg_start + mem-page_count)  num_entries)
-- 
1.6.4.4

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

[PATCH] drm/ttm: Clear the ghost cpu_writers flag on ttm_buffer_object_transfer.

[Francisco Jerez]

It makes sense for a BO to move after a process has requested
exclusive RW access on it (e.g. because the BO used to be located in
unmappable VRAM and we intercepted the CPU access from the fault
handler).

If we let the ghost object inherit cpu_writers from the original
object, ttm_bo_release_list() will raise a kernel BUG when the ghost
object is destroyed. This can be reproduced with the nouveau driver on
nv5x.

Reported-by: Marcin Slusarz 
Signed-off-by: Francisco Jerez 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 7cffb3e..3451a82 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -351,6 +351,7 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
INIT_LIST_HEAD(>lru);
INIT_LIST_HEAD(>swap);
fbo->vm_node = NULL;
+   atomic_set(>cpu_writers, 0);

fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
kref_init(>list_kref);
-- 
1.6.4.4

[PATCH v2] nouveau build regression, undefined reference to `acpi_video_get_edid'

[Francisco Jerez]

Phil Turmel  writes:

> Build breakage:
>
> drivers/built-in.o: In function `nouveau_acpi_edid':
> (.text+0x13404e): undefined reference to `acpi_video_get_edid'
> make: *** [.tmp_vmlinux1] Error 1
>
> Introduced by:
>
> a6ed76d7ffc62ffa474b41d31b011b6853c5de32 is the first bad commit
> commit a6ed76d7ffc62ffa474b41d31b011b6853c5de32
> Author: Ben Skeggs 
> Date:   Mon Jul 12 15:33:07 2010 +1000
>
> drm/nouveau: support fetching LVDS EDID from ACPI
>
> Based on a patch from Matthew Garrett.
>
> Signed-off-by: Ben Skeggs 
> Acked-by: Matthew Garrett 
>
> :04 04 2fbe9b4d9778329908107e72c11b100c2f5a460b 
> 97dcf06923bb576298746584c45d17d3be9edcf8 M  drivers
>
> It doesn't seem to revert cleanly, but the problem lies in these
> two config entries:
>
> CONFIG_ACPI=y
> CONFIG_ACPI_VIDEO=m
>
> Adding a select for ACPI_VIDEO appears to be the best solution, and
> is comparable to what is done in DRM_I915.  Builds, boots, and appears to
> work correctly.
>
> Signed-off-by: Philip J. Turmel 
> ---
>
> The first version disabled all ACPI functions in the nouveau driver if
> ACPI_VIDEO wasn't set.  Francisco Jerez  pointed out
> that this didn't make much sense.
>
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index d2d2804..72730e9 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -10,6 +10,7 @@ config DRM_NOUVEAU
>   select FB
>   select FRAMEBUFFER_CONSOLE if !EMBEDDED
>   select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
> + select ACPI_VIDEO if ACPI
>   help
> Choose this option for open-source nVidia support.

Thanks, pushed to the nouveau tree.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20100917/9056e4ec/attachment.pgp>

Re: [PATCH v2] nouveau build regression, undefined reference to `acpi_video_get_edid'

[Francisco Jerez]

Phil Turmel phi...@turmel.org writes:

 Build breakage:

 drivers/built-in.o: In function `nouveau_acpi_edid':
 (.text+0x13404e): undefined reference to `acpi_video_get_edid'
 make: *** [.tmp_vmlinux1] Error 1

 Introduced by:

 a6ed76d7ffc62ffa474b41d31b011b6853c5de32 is the first bad commit
 commit a6ed76d7ffc62ffa474b41d31b011b6853c5de32
 Author: Ben Skeggs bske...@redhat.com
 Date:   Mon Jul 12 15:33:07 2010 +1000

 drm/nouveau: support fetching LVDS EDID from ACPI

 Based on a patch from Matthew Garrett.

 Signed-off-by: Ben Skeggs bske...@redhat.com
 Acked-by: Matthew Garrett m...@redhat.com

 :04 04 2fbe9b4d9778329908107e72c11b100c2f5a460b 
 97dcf06923bb576298746584c45d17d3be9edcf8 M  drivers

 It doesn't seem to revert cleanly, but the problem lies in these
 two config entries:

 CONFIG_ACPI=y
 CONFIG_ACPI_VIDEO=m

 Adding a select for ACPI_VIDEO appears to be the best solution, and
 is comparable to what is done in DRM_I915.  Builds, boots, and appears to
 work correctly.

 Signed-off-by: Philip J. Turmel phi...@turmel.org
 ---

 The first version disabled all ACPI functions in the nouveau driver if
 ACPI_VIDEO wasn't set.  Francisco Jerez curroje...@riseup.net pointed out
 that this didn't make much sense.

 diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
 index d2d2804..72730e9 100644
 --- a/drivers/gpu/drm/nouveau/Kconfig
 +++ b/drivers/gpu/drm/nouveau/Kconfig
 @@ -10,6 +10,7 @@ config DRM_NOUVEAU
   select FB
   select FRAMEBUFFER_CONSOLE if !EMBEDDED
   select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
 + select ACPI_VIDEO if ACPI
   help
 Choose this option for open-source nVidia support.

Thanks, pushed to the nouveau tree.


pgpvaePFCFov9.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] nouveau build regression, undefined reference to `acpi_video_get_edid'

[Francisco Jerez]

Phil Turmel  writes:

> Hi all,
>
> I've got a build breakage on my laptop:
>
> drivers/built-in.o: In function `nouveau_acpi_edid':
> (.text+0x13404e): undefined reference to `acpi_video_get_edid'
> make: *** [.tmp_vmlinux1] Error 1
>
> Introduced by:
>
> a6ed76d7ffc62ffa474b41d31b011b6853c5de32 is the first bad commit
> commit a6ed76d7ffc62ffa474b41d31b011b6853c5de32
> Author: Ben Skeggs 
> Date:   Mon Jul 12 15:33:07 2010 +1000
>
> drm/nouveau: support fetching LVDS EDID from ACPI
>
> Based on a patch from Matthew Garrett.
>
> Signed-off-by: Ben Skeggs 
> Acked-by: Matthew Garrett 
>
> :04 04 2fbe9b4d9778329908107e72c11b100c2f5a460b 
> 97dcf06923bb576298746584c45d17d3be9edcf8 M  drivers
>
> It doesn't seem to revert cleanly, but I believe the problem lies in these
> two config entries:
>
> CONFIG_ACPI=y
> CONFIG_ACPI_VIDEO=m
>
> Having the nouveau ACPI features depend on CONFIG_ACPI_VIDEO instead of
> bare CONFIG_ACPI builds, boots, and works for me.
>
nouveau_acpi_edid() is the only function that depends on ACPI_VIDEO,
ifdef'ing out the rest of the ACPI stuff in that case doesn't make much
sense to me.

> Signed-off-by: Philip J. Turmel 
> ---
>
> Presumably, this could also be solved by adding
> "select ACPI_VIDEO if ACPI" to the nouveau Kconfig.
> I'd be happy to try that, if this approach is flawed.
>
> diff --git a/drivers/gpu/drm/nouveau/Makefile 
> b/drivers/gpu/drm/nouveau/Makefile
> index e9b06e4..4e295b5 100644
> --- a/drivers/gpu/drm/nouveau/Makefile
> +++ b/drivers/gpu/drm/nouveau/Makefile
> @@ -28,6 +28,6 @@ nouveau-y := nouveau_drv.o nouveau_state.o 
> nouveau_channel.o nouveau_mem.o \
>  nouveau-$(CONFIG_DRM_NOUVEAU_DEBUG) += nouveau_debugfs.o
>  nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
>  nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
> -nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
> +nouveau-$(CONFIG_ACPI_VIDEO) += nouveau_acpi.o
>  
>  obj-$(CONFIG_DRM_NOUVEAU)+= nouveau.o
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
> b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index b1be617..bd995b4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -842,7 +842,7 @@ extern int  nouveau_dma_wait(struct nouveau_channel *, 
> int slots, int size);
>  
>  /* nouveau_acpi.c */
>  #define ROM_BIOS_PAGE 4096
> -#if defined(CONFIG_ACPI)
> +#if defined(CONFIG_ACPI_VIDEO)
>  void nouveau_register_dsm_handler(void);
>  void nouveau_unregister_dsm_handler(void);
>  int nouveau_acpi_get_bios_chunk(uint8_t *bios, int offset, int len);
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: [PATCH] nouveau build regression, undefined reference to `acpi_video_get_edid'

[Francisco Jerez]

Phil Turmel phi...@turmel.org writes:

 Hi all,

 I've got a build breakage on my laptop:

 drivers/built-in.o: In function `nouveau_acpi_edid':
 (.text+0x13404e): undefined reference to `acpi_video_get_edid'
 make: *** [.tmp_vmlinux1] Error 1

 Introduced by:

 a6ed76d7ffc62ffa474b41d31b011b6853c5de32 is the first bad commit
 commit a6ed76d7ffc62ffa474b41d31b011b6853c5de32
 Author: Ben Skeggs bske...@redhat.com
 Date:   Mon Jul 12 15:33:07 2010 +1000

 drm/nouveau: support fetching LVDS EDID from ACPI

 Based on a patch from Matthew Garrett.

 Signed-off-by: Ben Skeggs bske...@redhat.com
 Acked-by: Matthew Garrett m...@redhat.com

 :04 04 2fbe9b4d9778329908107e72c11b100c2f5a460b 
 97dcf06923bb576298746584c45d17d3be9edcf8 M  drivers

 It doesn't seem to revert cleanly, but I believe the problem lies in these
 two config entries:

 CONFIG_ACPI=y
 CONFIG_ACPI_VIDEO=m

 Having the nouveau ACPI features depend on CONFIG_ACPI_VIDEO instead of
 bare CONFIG_ACPI builds, boots, and works for me.

nouveau_acpi_edid() is the only function that depends on ACPI_VIDEO,
ifdef'ing out the rest of the ACPI stuff in that case doesn't make much
sense to me.

 Signed-off-by: Philip J. Turmel phi...@turmel.org
 ---

 Presumably, this could also be solved by adding
 select ACPI_VIDEO if ACPI to the nouveau Kconfig.
 I'd be happy to try that, if this approach is flawed.

 diff --git a/drivers/gpu/drm/nouveau/Makefile 
 b/drivers/gpu/drm/nouveau/Makefile
 index e9b06e4..4e295b5 100644
 --- a/drivers/gpu/drm/nouveau/Makefile
 +++ b/drivers/gpu/drm/nouveau/Makefile
 @@ -28,6 +28,6 @@ nouveau-y := nouveau_drv.o nouveau_state.o 
 nouveau_channel.o nouveau_mem.o \
  nouveau-$(CONFIG_DRM_NOUVEAU_DEBUG) += nouveau_debugfs.o
  nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o
  nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
 -nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
 +nouveau-$(CONFIG_ACPI_VIDEO) += nouveau_acpi.o
  
  obj-$(CONFIG_DRM_NOUVEAU)+= nouveau.o
 diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h 
 b/drivers/gpu/drm/nouveau/nouveau_drv.h
 index b1be617..bd995b4 100644
 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
 +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
 @@ -842,7 +842,7 @@ extern int  nouveau_dma_wait(struct nouveau_channel *, 
 int slots, int size);
  
  /* nouveau_acpi.c */
  #define ROM_BIOS_PAGE 4096
 -#if defined(CONFIG_ACPI)
 +#if defined(CONFIG_ACPI_VIDEO)
  void nouveau_register_dsm_handler(void);
  void nouveau_unregister_dsm_handler(void);
  int nouveau_acpi_get_bios_chunk(uint8_t *bios, int offset, int len);


pgp2gL5zVIhFB.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

nouveau lockdep warning

[Francisco Jerez]

Johannes Berg  writes:

> Francisco,
>
> The patch you pointed me works, but now, although it's probably not due
> to that patch, I get a lockdep warning:
>
> [   75.428119] [drm] nouveau :02:00.0: nouveau_channel_free: freeing fifo 
> 2
> [   75.430015] 
> [   75.430015] ==
> [   75.430015] [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
> [   75.430015] 2.6.36-rc3-wl-47417-g8164729-dirty #183
> [   75.430015] --
> [   75.430015] Xorg/3109 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> [   75.430015]  (&(>unused_lock)->rlock){+.+...}, at: 
> [] drm_mm_put_block+0x93/0x190 [drm]
> [   75.430015] 
> [   75.430015] and this task is already holding:
> [   75.430015]  (&(_priv->context_switch_lock)->rlock){-.}, at: 
> [] nouveau_channel_free+0xf9/0x2b0 [nouveau]
> [   75.430015] which would create a new lock dependency:
> [   75.430015]  (&(_priv->context_switch_lock)->rlock){-.} -> 
> (&(>unused_lock)->rlock){+.+...}
> [   75.430015] 
> [   75.430015] but this new dependency connects a HARDIRQ-irq-safe lock:
> [   75.430015] [drm] nouveau :02:00.0: GPU lockup - switching to software 
> fbcon
> [   75.430015]  (&(_priv->context_switch_lock)->rlock){-.}
> [   75.430015] ... which became HARDIRQ-irq-safe at:
> [   75.430015]   [] mark_irqflags+0x17d/0x190
> [   75.430015]   [] __lock_acquire+0x57c/0x9d0
> [   75.430015]   [] lock_acquire+0xa2/0x1d0
> [   75.430015]   [] _raw_spin_lock_irqsave+0x52/0x90
> [   75.430015]   [] nouveau_irq_handler+0x6f/0x1b0 [nouveau]
> [   75.430015]   [] handle_IRQ_event+0x81/0x2e0
> [   75.430015]   [] handle_fasteoi_irq+0x7c/0x100
> [   75.430015]   [] handle_irq+0x22/0x30
> [   75.430015]   [] do_IRQ+0x73/0xf0
> [   75.430015]   [] ret_from_intr+0x0/0xf
> [   75.430015]   [] dev_set_name+0x41/0x50
> [   75.430015]   [] backlight_device_register+0xcc/0x2d0
> [   75.430015]   [] nouveau_nv50_backlight_init+0x87/0xf0 
> [nouveau]
> [   75.430015]   [] nouveau_backlight_init+0x2d/0x50 
> [nouveau]
> [   75.430015]   [] nouveau_card_init+0x263/0x300 [nouveau]
> [   75.430015]   [] nouveau_load+0x351/0x660 [nouveau]
> [   75.430015]   [] drm_get_pci_dev+0x183/0x3a0 [drm]
> [   75.430015]   [] nouveau_pci_probe+0x15/0x17 [nouveau]
> [   75.430015]   [] local_pci_probe+0x5f/0xd0
> [   75.430015]   [] pci_device_probe+0x88/0xb0
> [   75.430015]   [] really_probe+0x68/0x190
> [   75.430015]   [] driver_probe_device+0x45/0x70
> [   75.430015]   [] __driver_attach+0x9b/0xa0
> [   75.430015]   [] bus_for_each_dev+0x6c/0xa0
> [   75.430015]   [] driver_attach+0x1e/0x20
> [   75.430015]   [] bus_add_driver+0xd5/0x370
> [   75.430015]   [] driver_register+0x78/0x140
> [   75.430015]   [] __pci_register_driver+0x66/0xe0
> [   75.430015]   [] drm_pci_init+0xdf/0xf0 [drm]
> [   75.430015]   [] drm_init+0x58/0x70 [drm]
> [   75.430015]   [] 0xa085b048
> [   75.430015]   [] do_one_initcall+0x43/0x180
> [   75.430015]   [] sys_init_module+0xba/0x200
> [   75.430015]   [] system_call_fastpath+0x16/0x1b
> [   75.430015] 
> [   75.430015] to a HARDIRQ-irq-unsafe lock:
> [   75.430015]  (&(>unused_lock)->rlock){+.+...}
> [   75.430015] ... which became HARDIRQ-irq-unsafe at:
> [   75.430015] ...  [] mark_irqflags+0x120/0x190
> [   75.430015]   [] __lock_acquire+0x57c/0x9d0
> [   75.430015]   [] lock_acquire+0xa2/0x1d0
> [   75.430015]   [] _raw_spin_lock+0x40/0x80
> [   75.430015]   [] drm_mm_pre_get+0x25/0x1e0 [drm]
> [   75.430015]   [] ttm_bo_setup_vm+0x2b/0x140 [ttm]
> [   75.430015]   [] ttm_bo_init+0x24d/0x290 [ttm]
> [   75.430015]   [] nouveau_bo_new+0x161/0x2d0 [nouveau]
> [   75.430015]   [] nouveau_mem_init+0x200/0x580 [nouveau]
> [   75.430015]   [] nouveau_card_init+0xf6/0x300 [nouveau]
> [   75.430015]   [] nouveau_load+0x351/0x660 [nouveau]
> [   75.430015]   [] drm_get_pci_dev+0x183/0x3a0 [drm]
> [   75.430015]   [] nouveau_pci_probe+0x15/0x17 [nouveau]
> [   75.430015]   [] local_pci_probe+0x5f/0xd0
> [   75.430015]   [] pci_device_probe+0x88/0xb0
> [   75.430015]   [] really_probe+0x68/0x190
> [   75.430015]   [] driver_probe_device+0x45/0x70
> [   75.430015]   [] __driver_attach+0x9b/0xa0
> [   75.430015]   [] bus_for_each_dev+0x6c/0xa0
> [   75.430015]   [] driver_attach+0x1e/0x20
> [   75.430015]   [] bus_add_driver+0xd5/0x370
> [   75.430015]   [] driver_register+0x78/0x140
> [   75.430015]   [] __pci_register_driver+0x66/0xe0
> [   75.430015]   [] drm_pci_init+0xdf/0xf0 [drm]
> [   75.430015]   [] drm_init+0x58/0x70 [drm]
> [   75.430015]   [] 0xa085b048
> [   75.430015]   [] do_one_initcall+0x43/0x180
> [   75.430015]   [] sys_init_module+0xba/0x200
> [   75.430015]   [] system_call_fastpath+0x16/0x1b
> [   75.430015] 
> [   75.430015] other info that might help us debug this:
> [   75.430015] 
> [   75.430015] 2 locks held by Xorg/3109:
> [   75.430015]  #0:  (drm_global_mutex){+.+.+.}, at: [] 
> drm_ioctl+0x361/0x4d0 [drm]
> 

nouveau X lockup on 36-rc (regression from 2.6.35)

[Francisco Jerez]

Johannes Berg  writes:

> On Mon, 2010-08-30 at 21:33 +0200, Francisco Jerez wrote:
>> Johannes Berg  writes:
>> 
>> > On Mon, 2010-08-30 at 21:08 +0200, Francisco Jerez wrote:
>> >
>> >> > This problem persists in -rc3, where I'd hoped the nouveau changes would
>> >> > actually fix it :-( In fact, it seems I was either "lucky" or it was
>> >> > made easier to trigger, this time it already happened during my login.
>> >
>> >> That's already fixed in the nouveau kernel tree.
>> >
>> > Can you identify a fix that I can cherry-pick until (hopefully) -rc4?
>> >
>> Sure, try 5a5608adff833bad37c56278b6194c5b0a5b36bf.
>
> An actual url would help ... What is "the nouveau kernel tree"?
>
Ah sorry, git://anongit.freedesktop.org/nouveau/linux-2.6.

> johannes
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20100830/0e0efa5d/attachment.pgp>

nouveau X lockup on 36-rc (regression from 2.6.35)

[Francisco Jerez]

Johannes Berg  writes:

> On Mon, 2010-08-30 at 21:08 +0200, Francisco Jerez wrote:
>
>> > This problem persists in -rc3, where I'd hoped the nouveau changes would
>> > actually fix it :-( In fact, it seems I was either "lucky" or it was
>> > made easier to trigger, this time it already happened during my login.
>
>> That's already fixed in the nouveau kernel tree.
>
> Can you identify a fix that I can cherry-pick until (hopefully) -rc4?
>
Sure, try 5a5608adff833bad37c56278b6194c5b0a5b36bf.

> johannes
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20100830/7b890524/attachment.pgp>

nouveau X lockup on 36-rc (regression from 2.6.35)

[Francisco Jerez]

Johannes Berg  writes:

> On Wed, 2010-08-25 at 15:55 +0200, Johannes Berg wrote:
>> Since 2.6.36-rc kernels (both rc1 and rc2) I've had X lockups where X
>> sits in a loop just using CPU.
>> 
>> The only kernel message I got this time was this one:
>> 
>> [22290.792075] [drm] nouveau :02:00.0: PFIFO_DMA_PUSHER - Ch 2
>> 
>> stracing X just repeats, in a loop,
>> 
>> ioctl(9, 0x40086482, 0x7fff6a873af0)= ? ERESTARTSYS (To be restarted)
>> --- SIGALRM (Alarm clock) @ 0 (0) ---
>> rt_sigreturn(0xe)   = -1 EINTR (Interrupted system call)
>> 
>> 
>> so apparently that ioctl never returns.
>> 
>> This has never happened on 2.6.35, and I've gone back now. Large screen
>> updates seem to trigger it more likely, but I haven't found a way to
>> really reproduce it. FWIW, I'm running with two screens connected.
>
> This problem persists in -rc3, where I'd hoped the nouveau changes would
> actually fix it :-( In fact, it seems I was either "lucky" or it was
> made easier to trigger, this time it already happened during my login.
>
That's already fixed in the nouveau kernel tree.

> johannes
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: nouveau X lockup on 36-rc (regression from 2.6.35)

[Francisco Jerez]

Johannes Berg johan...@sipsolutions.net writes:

 On Mon, 2010-08-30 at 21:08 +0200, Francisco Jerez wrote:

  This problem persists in -rc3, where I'd hoped the nouveau changes would
  actually fix it :-( In fact, it seems I was either lucky or it was
  made easier to trigger, this time it already happened during my login.

 That's already fixed in the nouveau kernel tree.

 Can you identify a fix that I can cherry-pick until (hopefully) -rc4?

Sure, try 5a5608adff833bad37c56278b6194c5b0a5b36bf.

 johannes


pgpSNkTDFqsi7.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: nouveau X lockup on 36-rc (regression from 2.6.35)

[Francisco Jerez]

Johannes Berg johan...@sipsolutions.net writes:

 On Mon, 2010-08-30 at 21:33 +0200, Francisco Jerez wrote:
 Johannes Berg johan...@sipsolutions.net writes:
 
  On Mon, 2010-08-30 at 21:08 +0200, Francisco Jerez wrote:
 
   This problem persists in -rc3, where I'd hoped the nouveau changes would
   actually fix it :-( In fact, it seems I was either lucky or it was
   made easier to trigger, this time it already happened during my login.
 
  That's already fixed in the nouveau kernel tree.
 
  Can you identify a fix that I can cherry-pick until (hopefully) -rc4?
 
 Sure, try 5a5608adff833bad37c56278b6194c5b0a5b36bf.

 An actual url would help ... What is the nouveau kernel tree?

Ah sorry, git://anongit.freedesktop.org/nouveau/linux-2.6.

 johannes


pgpHjpcKx58L5.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[patch v2] nouveau: unwind on load errors

[Francisco Jerez]

Marcin Slusarz  writes:

> On Fri, Jul 30, 2010 at 05:04:32PM +0200, Dan Carpenter wrote:
>> nuveau_load() just returned directly if there was an error instead of
>>
>   ^^ typo
>
>> releasing resources. 
>> 
>> 
>> Signed-off-by: Dan Carpenter 
>> ---
>> V2: updated to the nouveau git tree.
>
> Thanks.
> Reviewed-by: Marcin Slusarz 

Thanks, pushed to the nouveau kernel tree.

>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c 
>> b/drivers/gpu/drm/nouveau/nouveau_state.c
>> index ee3729e..cf16bfb 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_state.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_state.c
>> @@ -739,8 +739,10 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  int ret;
>>  
>>  dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
>> -if (!dev_priv)
>> -return -ENOMEM;
>> +if (!dev_priv) {
>> +ret = -ENOMEM;
>> +goto err_out;
>> +}
>>  dev->dev_private = dev_priv;
>>  dev_priv->dev = dev;
>>  
>> @@ -750,8 +752,10 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>   dev->pci_vendor, dev->pci_device, dev->pdev->class);
>>  
>>  dev_priv->wq = create_workqueue("nouveau");
>> -if (!dev_priv->wq)
>> -return -EINVAL;
>> +if (!dev_priv->wq) {
>> +ret = -EINVAL;
>> +goto err_priv;
>> +}
>>  
>>  /* resource 0 is mmio regs */
>>  /* resource 1 is linear FB */
>> @@ -764,7 +768,8 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  if (!dev_priv->mmio) {
>>  NV_ERROR(dev, "Unable to initialize the mmio mapping. "
>>   "Please report your setup to " DRIVER_EMAIL "\n");
>> -return -EINVAL;
>> +ret = -EINVAL;
>> +goto err_wq;
>>  }
>>  NV_DEBUG(dev, "regs mapped ok at 0x%llx\n",
>>  (unsigned long long)mmio_start_offs);
>> @@ -812,7 +817,8 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  break;
>>  default:
>>  NV_INFO(dev, "Unsupported chipset 0x%08x\n", reg0);
>> -return -EINVAL;
>> +ret = -EINVAL;
>> +goto err_mmio;
>>  }
>>  
>>  NV_INFO(dev, "Detected an NV%2x generation card (0x%08x)\n",
>> @@ -820,7 +826,7 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  
>>  ret = nouveau_remove_conflicting_drivers(dev);
>>  if (ret)
>> -return ret;
>> +goto err_mmio;
>>  
>>  /* Map PRAMIN BAR, or on older cards, the aperture withing BAR0 */
>>  if (dev_priv->card_type >= NV_40) {
>> @@ -834,7 +840,8 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  dev_priv->ramin_size);
>>  if (!dev_priv->ramin) {
>>  NV_ERROR(dev, "Failed to PRAMIN BAR");
>> -return -ENOMEM;
>> +ret = -ENOMEM;
>> +goto err_mmio;
>>  }
>>  } else {
>>  dev_priv->ramin_size = 1 * 1024 * 1024;
>> @@ -842,7 +849,8 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>dev_priv->ramin_size);
>>  if (!dev_priv->ramin) {
>>  NV_ERROR(dev, "Failed to map BAR0 PRAMIN.\n");
>> -return -ENOMEM;
>> +ret = -ENOMEM;
>> +goto err_mmio;
>>  }
>>  }
>>  
>> @@ -857,9 +865,21 @@ int nouveau_load(struct drm_device *dev, unsigned long 
>> flags)
>>  /* For kernel modesetting, init card now and bring up fbcon */
>>  ret = nouveau_card_init(dev);
>>  if (ret)
>> -return ret;
>> +goto err_ramin;
>>  
>>  return 0;
>> +
>> +err_ramin:
>> +iounmap(dev_priv->ramin);
>> +err_mmio:
>> +iounmap(dev_priv->mmio);
>> +err_wq:
>> +destroy_workqueue(dev_priv->wq);
>> +err_priv:
>> +kfree(dev_priv);
>> +dev->dev_private = NULL;
>> +err_out:
>> +return ret;
>>  }
>>  
>>  void nouveau_lastclose(struct drm_device *dev)
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

[PATCH] drm: nouveau: set TASK_(UN)INTERRUPTIBLE before schedule_timeout()

[Francisco Jerez]

Kulikov Vasiliy  writes:

> set_current_state() is called only once before the first iteration.
> After return from schedule_timeout() current state is TASK_RUNNING. If
> we are going to wait again, set_current_state() must be called.
>
> Signed-off-by: Kulikov Vasiliy 
> ---
>  drivers/gpu/drm/nouveau/nouveau_fence.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
Thanks, I've pushed this patch to the Nouveau kernel tree.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c 
> b/drivers/gpu/drm/nouveau/nouveau_fence.c
> index 813d853..bae 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
> @@ -186,7 +186,6 @@ nouveau_fence_wait(void *sync_obj, void *sync_arg, bool 
> lazy, bool intr)
>   unsigned long timeout = jiffies + (3 * DRM_HZ);
>   int ret = 0;
>  
> - __set_current_state(intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
>  
>   while (1) {
>   if (nouveau_fence_signalled(sync_obj, sync_arg))
> @@ -197,6 +196,8 @@ nouveau_fence_wait(void *sync_obj, void *sync_arg, bool 
> lazy, bool intr)
>   break;
>   }
>  
> + __set_current_state(intr ? TASK_INTERRUPTIBLE
> + : TASK_UNINTERRUPTIBLE);
>   if (lazy)
>   schedule_timeout(1);
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

deadlock possiblity introduced by "drm/nouveau: use drm_mm in preference to custom code doing the same thing"

[Francisco Jerez]

Marcin Slusarz  writes:

> On Sun, Jul 11, 2010 at 11:02:12AM +1000, Ben Skeggs wrote:
>> On Sun, 2010-07-11 at 01:24 +0200, Marcin Slusarz wrote:
>> > Hi
>> > 
>> > Patch "drm/nouveau: use drm_mm in preference to custom code doing the same 
>> > thing"
>> > in nouveau tree introduced new deadlock possibility, for which lockdep 
>> > complains loudly:
>> > 
>> > (...)
>> > 
>> Hey,
>> 
>> Thanks for the report, I'll look at this more during the week.
>> 
>> > Deadlock scenario looks like this:
>> > CPU1CPU2
>> > nouveau code calls some drm_mm.c
>> > function which takes mm->unused_lock
>> > 
>> > nouveau_channel_free disables 
>> > irqs and takes dev_priv->context_switch_lock
>> > calls 
>> > nv50_graph_destroy_context which
>> > (... backtrace 
>> > above)
>> > calls 
>> > drm_mm_put_block which tries to take mm->unused_lock (spins)
>> > nouveau interrupt raises
>> > 
>> > nouveau_irq_handler tries to take
>> > dev_priv->context_switch_lock (spins)
>> > 
>> > deadlock
>> It's important to note that the drm_mm referenced eventually by
>> nv50_graph_destroy_context is per-channel on the card, so for the
>> deadlock to happen it'd have to be multiple threads from a single
>> process, one thread creating/destroying and object on the channel while
>> another was trying to destroy the channel.
>> 

Yeah, and that situation is impossible ATM because those functions are
called with the BKL held.

>> > 
>> > Possible solutions:
>> > - reverting "drm/nouveau: use drm_mm in preference to custom code doing 
>> > the same thing"
>> > - disabling interrupts before calling drm_mm functions - unmaintainable 
>> > and still
>> >   deadlockable in multicard setups (nouveau and eg radeon)
>> Agreed it's unmaintainable, but, as mentioned above, the relevant locks
>> can't be touched by radeon.
>> 
>> > - making mm->unused_lock HARDIRQ-safe (patch below) - simple but with 
>> > slight overhead
>> I'll look more during the week, there's other solutions to be explored.
>
> So, did you find other solution?

Some random ideas:

 - Make context_switch_lock HARDIRQ-unsafe. To avoid racing with the IRQ
   handler we'd have to disable interrupt dispatch on the card before
   taking context_switch_lock (i.e. at channel creation and destruction
   time), and the interrupt control registers would have to be protected
   with a IRQ safe spinlock.

 - Split the current destroy_context() hooks in two halves, the first
   one would be in charge of the PFIFO/PGRAPH-poking tasks (e.g.
   disable_context()), and the second one would take care of releasing
   the allocated resources (and it wouldn't need locking).

>
> Marcin
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: deadlock possiblity introduced by drm/nouveau: use drm_mm in preference to custom code doing the same thing

[Francisco Jerez]

Marcin Slusarz marcin.slus...@gmail.com writes:

 On Sun, Jul 11, 2010 at 11:02:12AM +1000, Ben Skeggs wrote:
 On Sun, 2010-07-11 at 01:24 +0200, Marcin Slusarz wrote:
  Hi
  
  Patch drm/nouveau: use drm_mm in preference to custom code doing the same 
  thing
  in nouveau tree introduced new deadlock possibility, for which lockdep 
  complains loudly:
  
  (...)
  
 Hey,
 
 Thanks for the report, I'll look at this more during the week.
 
  Deadlock scenario looks like this:
  CPU1CPU2
  nouveau code calls some drm_mm.c
  function which takes mm-unused_lock
  
  nouveau_channel_free disables 
  irqs and takes dev_priv-context_switch_lock
  calls 
  nv50_graph_destroy_context which
  (... backtrace 
  above)
  calls 
  drm_mm_put_block which tries to take mm-unused_lock (spins)
  nouveau interrupt raises
  
  nouveau_irq_handler tries to take
  dev_priv-context_switch_lock (spins)
  
  deadlock
 It's important to note that the drm_mm referenced eventually by
 nv50_graph_destroy_context is per-channel on the card, so for the
 deadlock to happen it'd have to be multiple threads from a single
 process, one thread creating/destroying and object on the channel while
 another was trying to destroy the channel.
 

Yeah, and that situation is impossible ATM because those functions are
called with the BKL held.

  
  Possible solutions:
  - reverting drm/nouveau: use drm_mm in preference to custom code doing 
  the same thing
  - disabling interrupts before calling drm_mm functions - unmaintainable 
  and still
deadlockable in multicard setups (nouveau and eg radeon)
 Agreed it's unmaintainable, but, as mentioned above, the relevant locks
 can't be touched by radeon.
 
  - making mm-unused_lock HARDIRQ-safe (patch below) - simple but with 
  slight overhead
 I'll look more during the week, there's other solutions to be explored.

 So, did you find other solution?

Some random ideas:

 - Make context_switch_lock HARDIRQ-unsafe. To avoid racing with the IRQ
   handler we'd have to disable interrupt dispatch on the card before
   taking context_switch_lock (i.e. at channel creation and destruction
   time), and the interrupt control registers would have to be protected
   with a IRQ safe spinlock.

 - Split the current destroy_context() hooks in two halves, the first
   one would be in charge of the PFIFO/PGRAPH-poking tasks (e.g.
   disable_context()), and the second one would take care of releasing
   the allocated resources (and it wouldn't need locking).


 Marcin

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


pgpMPxuqUCMXj.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/ttm: Allocate the page pool manager in the heap.

[Francisco Jerez]

Repeated ttm_page_alloc_init/fini fails noisily because the pool
manager kobj isn't zeroed out between uses (we could do just that but
statically allocated kobjects are generally considered a bad thing).
Move it to kzalloc'ed memory.

Note that this patch drops the refcounting behavior of the pool
allocator init/fini functions: it would have led to a race condition
in its current form, and anyway it was never exploited.

Signed-off-by: Francisco Jerez 
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c |   68 -
 include/drm/ttm/ttm_page_alloc.h |4 --
 2 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index ef91069..fc68a66 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -104,7 +104,6 @@ struct ttm_pool_opts {
 struct ttm_pool_manager {
struct kobject  kobj;
struct shrinker mm_shrink;
-   atomic_tpage_alloc_inited;
struct ttm_pool_optsoptions;

union {
@@ -142,7 +141,7 @@ static void ttm_pool_kobj_release(struct kobject *kobj)
 {
struct ttm_pool_manager *m =
container_of(kobj, struct ttm_pool_manager, kobj);
-   (void)m;
+   kfree(m);
 }

 static ssize_t ttm_pool_store(struct kobject *kobj,
@@ -214,9 +213,7 @@ static struct kobj_type ttm_pool_kobj_type = {
.default_attrs = ttm_pool_attrs,
 };

-static struct ttm_pool_manager _manager = {
-   .page_alloc_inited  = ATOMIC_INIT(0)
-};
+static struct ttm_pool_manager *_manager;

 #ifndef CONFIG_X86
 static int set_pages_array_wb(struct page **pages, int addrinarray)
@@ -271,7 +268,7 @@ static struct ttm_page_pool *ttm_get_pool(int flags,
if (flags & TTM_PAGE_FLAG_DMA32)
pool_index |= 0x2;

-   return &_manager.pools[pool_index];
+   return &_manager->pools[pool_index];
 }

 /* set memory back to wb and free the pages. */
@@ -387,7 +384,7 @@ static int ttm_pool_get_num_unused_pages(void)
unsigned i;
int total = 0;
for (i = 0; i < NUM_POOLS; ++i)
-   total += _manager.pools[i].npages;
+   total += _manager->pools[i].npages;

return total;
 }
@@ -408,7 +405,7 @@ static int ttm_pool_mm_shrink(int shrink_pages, gfp_t 
gfp_mask)
unsigned nr_free = shrink_pages;
if (shrink_pages == 0)
break;
-   pool = &_manager.pools[(i + pool_offset)%NUM_POOLS];
+   pool = &_manager->pools[(i + pool_offset)%NUM_POOLS];
shrink_pages = ttm_page_pool_free(pool, nr_free);
}
/* return estimated number of unused pages in pool */
@@ -576,10 +573,10 @@ static void ttm_page_pool_fill_locked(struct 
ttm_page_pool *pool,

/* If allocation request is small and there is not enough
 * pages in pool we fill the pool first */
-   if (count < _manager.options.small
+   if (count < _manager->options.small
&& count > pool->npages) {
struct list_head new_pages;
-   unsigned alloc_size = _manager.options.alloc_size;
+   unsigned alloc_size = _manager->options.alloc_size;

/**
 * Can't change page caching if in irqsave context. We have to
@@ -759,8 +756,8 @@ void ttm_put_pages(struct list_head *pages, unsigned 
page_count, int flags,
pool->npages += page_count;
/* Check that we don't go over the pool limit */
page_count = 0;
-   if (pool->npages > _manager.options.max_size) {
-   page_count = pool->npages - _manager.options.max_size;
+   if (pool->npages > _manager->options.max_size) {
+   page_count = pool->npages - _manager->options.max_size;
/* free at least NUM_PAGES_TO_ALLOC number of pages
 * to reduce calls to set_memory_wb */
if (page_count < NUM_PAGES_TO_ALLOC)
@@ -785,33 +782,36 @@ static void ttm_page_pool_init_locked(struct 
ttm_page_pool *pool, int flags,
 int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
 {
int ret;
-   if (atomic_add_return(1, &_manager.page_alloc_inited) > 1)
-   return 0;
+
+   WARN_ON(_manager);

printk(KERN_INFO TTM_PFX "Initializing pool allocator.\n");

-   ttm_page_pool_init_locked(&_manager.wc_pool, GFP_HIGHUSER, "wc");
+   _manager = kzalloc(sizeof(*_manager), GFP_KERNEL);

-   ttm_page_pool_init_locked(&_manager.uc_pool, GFP_HIGHUSER, "uc");
+   ttm_page_pool_init_locked(&_manager->wc_pool, GFP_HIGHUSER, "wc");

-   ttm_page_pool_init_locked(&_manager.wc_pool_dma32, GFP_USER | GFP_DMA32,
-   "wc dma

Radeon 3650HD laptop LVDS lid open/closed detection problem

[Francisco Jerez]

Alex Deucher  writes:

> On Mon, Jun 21, 2010 at 10:53 AM, Francisco Jerez  
> wrote:
>> Jerome Glisse  writes:
>>
>>> On Mon, Jun 21, 2010 at 03:31:19PM +0300, Pasi K?rkk?inen wrote:
>>>> Hello,
>>>>
>>>> After fixing the dvi/hdmi detection problem I now have another problem
>>>> with the HP EliteBook 8530p, which has Radeon 3650HD adapter.
>>>>
>>>> Here's a summary of the environment:
>>>>
>>>> ? ? ?- laptop connected to a docking station.
>>>> ? ? ?- external display in use, connected with DVI to the dock.
>>>> ? ? ?- laptop lid closed, so internal LVDS display is not used.
>>>>
>>>> Now, when I start the laptop, I can see the BIOS and grub on the external 
>>>> DVI display.
>>>> All fine so far. I select the Fedora 13 kernel, and Linux starts. I see 
>>>> the Fedora
>>>> graphical boot on the external DVI display, just like it should be. GDM 
>>>> login prompt
>>>> appears on the external DVI display, still everything fine.
>>>>
>>>> And then it goes wrong. After I login to X, the external display only 
>>>> shows the background
>>>> picture.. it turns out the desktop stuff has been started to the internal 
>>>> LVDS display,
>>>> which shouldn't be used at all since the laptop lid is closed!!
>>>>
>>>> When the laptop lid is closed, and external display is connected, I want 
>>>> to use only the external display..
>>>>
>>>> Any ideas how to troubleshoot this one?
>
> Does it work ok if you boot up without the external display connected
> and then connect and enable the secondary display after you've logged
> in?  I have a similar issue here;  I think it's an issue with rhgb and
> starting X.  If I boot with an external display, the entire plymouth
> load sequence works fine, but then when X starts the external display
> goes blank and the internal display hangs on the plymouth screen.  On
> a related note, if I close the lid and turn off the LVDS output, gpm
> never blanks the external monitor.  I have to manually force dpms with
> xset.
>
>
>>>>
>>>> -- Pasi
>>>>
>>>
>>> It's better to open bug when you face issue rather than mail, as it's
>>> harder to track information in mail thread than in a bug. Your issue
>>> is not easily fixed because there is many laptop with broken acpi which
>>> report wrong lid status (some of them always report lid closed what ever
>>> is the lid status, other always report lid open, ... i am not expert on
>>> how broken this is but from what i have been told i should rather consider
>>> drinking than trying to look into it and then go to the drinking step).
>>>
>>> Bottom line is that lid detection is unreliable thus so far we ignore
>>> it silently. I think the plan is to monitor lid status change and if
>>> we detect change from either open to close or close to open then we
>>> can start assuming that acpi lid status is reliable and act accordingly.
>>>
>> In Nouveau we report connector_status_unknown for closed lids (On the
>> kernel side unknown outputs are left disabled unless there's nothing
>> else definitely connected: if lid detection doesn't work at all the
>> system will still be usable). This would solve your problem if we made
>> the X server set the first output known to be connected as RandR
>> primary.
>>
>> In short, I see two different "bugs" here:
>> ?* radeon reports connector_status_connected when the lid is closed.
>
> Do we really want to report disconnected when the lid is closed?

Definitely not, my point was that you could report connector_status_unknown.

> The monitor is still connected.  Considering how unreliable lid events
> are I don't think that makes sense.  We probably actually want dpms
> off, but connected which is more of a policy thing and should be
> handled by userspace.
>
> Alex
>
>> ?* the X server doesn't select a primary output among the definitely
>> ? connected ones.
>>
>>> Cheers,
>>> Jerome
>>> ___
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20100621/d27ec200/attachment.pgp>

Radeon 3650HD laptop LVDS lid open/closed detection problem

[Francisco Jerez]

Jerome Glisse  writes:

> On Mon, Jun 21, 2010 at 03:31:19PM +0300, Pasi K?rkk?inen wrote:
>> Hello,
>> 
>> After fixing the dvi/hdmi detection problem I now have another problem
>> with the HP EliteBook 8530p, which has Radeon 3650HD adapter.
>> 
>> Here's a summary of the environment:
>> 
>>  - laptop connected to a docking station.
>>  - external display in use, connected with DVI to the dock.
>>  - laptop lid closed, so internal LVDS display is not used.
>> 
>> Now, when I start the laptop, I can see the BIOS and grub on the external 
>> DVI display.
>> All fine so far. I select the Fedora 13 kernel, and Linux starts. I see the 
>> Fedora
>> graphical boot on the external DVI display, just like it should be. GDM 
>> login prompt
>> appears on the external DVI display, still everything fine.
>> 
>> And then it goes wrong. After I login to X, the external display only shows 
>> the background
>> picture.. it turns out the desktop stuff has been started to the internal 
>> LVDS display,
>> which shouldn't be used at all since the laptop lid is closed!! 
>> 
>> When the laptop lid is closed, and external display is connected, I want to 
>> use only the external display..
>> 
>> Any ideas how to troubleshoot this one? 
>> 
>> -- Pasi
>> 
>
> It's better to open bug when you face issue rather than mail, as it's
> harder to track information in mail thread than in a bug. Your issue
> is not easily fixed because there is many laptop with broken acpi which
> report wrong lid status (some of them always report lid closed what ever
> is the lid status, other always report lid open, ... i am not expert on
> how broken this is but from what i have been told i should rather consider
> drinking than trying to look into it and then go to the drinking step).
>
> Bottom line is that lid detection is unreliable thus so far we ignore
> it silently. I think the plan is to monitor lid status change and if
> we detect change from either open to close or close to open then we
> can start assuming that acpi lid status is reliable and act accordingly.
>
In Nouveau we report connector_status_unknown for closed lids (On the
kernel side unknown outputs are left disabled unless there's nothing
else definitely connected: if lid detection doesn't work at all the
system will still be usable). This would solve your problem if we made
the X server set the first output known to be connected as RandR
primary.

In short, I see two different "bugs" here:
 * radeon reports connector_status_connected when the lid is closed.
 * the X server doesn't select a primary output among the definitely
   connected ones.

> Cheers,
> Jerome
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: 

Re: Radeon 3650HD laptop LVDS lid open/closed detection problem

[Francisco Jerez]

Jerome Glisse gli...@freedesktop.org writes:

 On Mon, Jun 21, 2010 at 03:31:19PM +0300, Pasi Kärkkäinen wrote:
 Hello,
 
 After fixing the dvi/hdmi detection problem I now have another problem
 with the HP EliteBook 8530p, which has Radeon 3650HD adapter.
 
 Here's a summary of the environment:
 
  - laptop connected to a docking station.
  - external display in use, connected with DVI to the dock.
  - laptop lid closed, so internal LVDS display is not used.
 
 Now, when I start the laptop, I can see the BIOS and grub on the external 
 DVI display.
 All fine so far. I select the Fedora 13 kernel, and Linux starts. I see the 
 Fedora
 graphical boot on the external DVI display, just like it should be. GDM 
 login prompt
 appears on the external DVI display, still everything fine.
 
 And then it goes wrong. After I login to X, the external display only shows 
 the background
 picture.. it turns out the desktop stuff has been started to the internal 
 LVDS display,
 which shouldn't be used at all since the laptop lid is closed!! 
 
 When the laptop lid is closed, and external display is connected, I want to 
 use only the external display..
 
 Any ideas how to troubleshoot this one? 
 
 -- Pasi
 

 It's better to open bug when you face issue rather than mail, as it's
 harder to track information in mail thread than in a bug. Your issue
 is not easily fixed because there is many laptop with broken acpi which
 report wrong lid status (some of them always report lid closed what ever
 is the lid status, other always report lid open, ... i am not expert on
 how broken this is but from what i have been told i should rather consider
 drinking than trying to look into it and then go to the drinking step).

 Bottom line is that lid detection is unreliable thus so far we ignore
 it silently. I think the plan is to monitor lid status change and if
 we detect change from either open to close or close to open then we
 can start assuming that acpi lid status is reliable and act accordingly.

In Nouveau we report connector_status_unknown for closed lids (On the
kernel side unknown outputs are left disabled unless there's nothing
else definitely connected: if lid detection doesn't work at all the
system will still be usable). This would solve your problem if we made
the X server set the first output known to be connected as RandR
primary.

In short, I see two different bugs here:
 * radeon reports connector_status_connected when the lid is closed.
 * the X server doesn't select a primary output among the definitely
   connected ones.

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


pgpvHORAyjJzk.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel