Re: [PATCH] dma-buf: Update docs for SYNC ioctl

2016-03-21 Thread David Herrmann
Hi

On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter  wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> v2: Fix them typos (Hans).
>
> Cc: Chris Wilson 
> Cc: Tiago Vignatti 
> Cc: Stéphane Marchesin 
> Cc: David Herrmann 
> Cc: Sumit Semwal 
> Cc: Daniel Vetter 
> CC: linux-me...@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: intel-...@lists.freedesktop.org
> Cc: de...@driverdev.osuosl.org
> Cc: Hans Verkuil 
> Acked-by: Sumit Semwal 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++-
>  drivers/dma-buf/dma-buf.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> making
> sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.

What is "restart on EAGAIN" supposed to mean? Or more generally, what
does EAGAIN tell the caller?

Thanks
David

> Some systems might need some sort of cache coherency management e.g. when
> CPU and GPU domains are being accessed through dma-buf at the same time. 
> To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 
> 2 main use-cases:
> want (with the new data being consumed by the GPU or say scanout 
> device)
>   - munmap once you don't need the buffer any more
>
> -Therefore, for correctness and optimal performance, systems with the 
> memory
> -cache shared by the GPU and CPU i.e. the "coherent" and also the
> -"incoherent" are always required to use SYNC_START and SYNC_END before 
> and
> -after, respectively, when accessing the mapped address.
> +For correctness and optimal performance, it is always required to use
> +SYNC_START and SYNC_END before and after, respectively, when accessing 
> the
> +mapped address. Userspace cannot rely on coherent access, even when there
> +are systems where it just works without calling these ioctls.
>
>  2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:[in]buffer to complete cpu access for.
>   * @direction: [in]length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>enum dma_data_direction direction)
> --
> 2.8.0.rc3
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] dma-buf: Update docs for SYNC ioctl

2016-03-23 Thread David Herrmann
Hey

On Mon, Mar 21, 2016 at 6:14 PM, Daniel Vetter  wrote:
> On Mon, Mar 21, 2016 at 01:26:58PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter  
>> wrote:
>> > Just a bit of wording polish plus mentioning that it can fail and must
>> > be restarted.
>> >
>> > Requested by Sumit.
>> >
>> > v2: Fix them typos (Hans).
>> >
>> > Cc: Chris Wilson 
>> > Cc: Tiago Vignatti 
>> > Cc: Stéphane Marchesin 
>> > Cc: David Herrmann 
>> > Cc: Sumit Semwal 
>> > Cc: Daniel Vetter 
>> > CC: linux-me...@vger.kernel.org
>> > Cc: dri-de...@lists.freedesktop.org
>> > Cc: linaro-mm-...@lists.linaro.org
>> > Cc: intel-...@lists.freedesktop.org
>> > Cc: de...@driverdev.osuosl.org
>> > Cc: Hans Verkuil 
>> > Acked-by: Sumit Semwal 
>> > Signed-off-by: Daniel Vetter 
>> > ---
>> >  Documentation/dma-buf-sharing.txt | 11 ++-
>> >  drivers/dma-buf/dma-buf.c |  2 +-
>> >  2 files changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/Documentation/dma-buf-sharing.txt 
>> > b/Documentation/dma-buf-sharing.txt
>> > index 32ac32e773e1..ca44c5820585 100644
>> > --- a/Documentation/dma-buf-sharing.txt
>> > +++ b/Documentation/dma-buf-sharing.txt
>> > @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 
>> > 2 main use-cases:
>> >
>> > No special interfaces, userspace simply calls mmap on the dma-buf fd, 
>> > making
>> > sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is 
>> > *always*
>> > -   used when the access happens. This is discussed next paragraphs.
>> > +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail 
>> > with
>> > +   -EAGAIN or -EINTR, in which case it must be restarted.
>>
>> What is "restart on EAGAIN" supposed to mean? Or more generally, what
>> does EAGAIN tell the caller?
>
> Do what drmIoctl does essentially.
>
> while (ret == -1 && (errno == EAGAIN || errno == EINTR)
> ret = ioctl();
>
> Typed from memery, too lazy to look it up in the source ;-) I'm trying to
> sell the idea of a real dma-buf manpage to Sumit, we should clarify this
> in detail there.

My question was rather about why we do this? Semantics for EINTR are
well defined, and with SA_RESTART (default on linux) user-space can
ignore it. However, looping on EAGAIN is very uncommon, and it is not
at all clear why it is needed?

Returning an error to user-space makes sense if user-space has a
reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
operation helps user-space at all? As someone without insight into the
driver implementation, it is hard to tell why.. Any hints?

Thanks
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] dma-buf: Update docs for SYNC ioctl

2016-03-23 Thread David Herrmann
Hi

On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson  wrote:
> On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
>> My question was rather about why we do this? Semantics for EINTR are
>> well defined, and with SA_RESTART (default on linux) user-space can
>> ignore it. However, looping on EAGAIN is very uncommon, and it is not
>> at all clear why it is needed?
>>
>> Returning an error to user-space makes sense if user-space has a
>> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
>> operation helps user-space at all? As someone without insight into the
>> driver implementation, it is hard to tell why.. Any hints?
>
> The reason we return EAGAIN is to workaround a deadlock we face when
> blocking on the GPU holding the struct_mutex (inside the client's
> process), but the GPU is dead. As our locking is very, very coarse we
> cannot restart the GPU without acquiring the struct_mutex being held by
> the client so we wake the client up and tell them the resource they are
> waiting on (the flush of the object from the GPU into the CPU domain) is
> temporarily unavailable. If they try to immediately wait upon the ioctl
> again, they are blocked waiting for the reset to occur before they may
> complete their flush. There are a few other possible deadlocks that are
> also avoided with EAGAIN (again, the issue is more or less the lack of
> fine grained locking).

...so you hijacked EAGAIN for all DRM ioctls just for a driver
workaround? EAGAIN is universally used to signal the caller about a
blocking resource. It is very much linked to O_NONBLOCK. Why not use
EBUSY, ECANCELED, ECOMM, EDEADLOCK, EIO, EL3RST, ...

Anyhow, I guess that ship has sailed. But just mentioning EAGAIN in a
kernel-doc is way to vague for user-space to figure out they should
loop on it.

Thanks
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH] dma-buf: Update docs for SYNC ioctl

2016-03-29 Thread David Herrmann
Hi

On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
 wrote:
> Do we have an agreement here after all? David? I need to know whether this
> fixup is okay to go cause I'll need to submit to Chrome OS then.

Sure it is fine. The code is already there, we cannot change it.

Thanks
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

2013-12-19 Thread David Herrmann
Hi

On Mon, Dec 16, 2013 at 2:01 PM, Jiri Kosina  wrote:
> On Fri, 27 Sep 2013, Joseph Salisbury wrote:
>
>> >> commit b1a1442a23776756b254b69786848a94d92445ba
>> >> Author: Jiri Kosina 
>> >> Date: Mon Jun 3 11:27:48 2013 +0200
>> >>
>> >> HID: core: fix reporting of raw events
>> >>
>> >> Reverting this commit in v3.12-rc2 prevents the system from locking up,
>> >> which happens when connecting a bluetooth trackpad.
>> >>
>> >> Jiri, do you think we should revert this patch, or is there some further
>> >> debugging/data collecting you would like to do?
>> > Hi Joseph,
>> >
>> > in this mail:
>> >
>> > Message-ID: <5241992e.3090...@canonical.com>
>> > Date: Tue, 24 Sep 2013 09:52:46 -0400
>> >
>> > you said that reverting this commit doesn't prevent the lockups, so I am
>> > rather confused ... ?
>> >
>> > Thanks,
>> >
>> The testing was invalid.  Reverting commit b1a1442 does resolve the bug
>> and stop the lockups.
>
> Okay, I finally got some sense of this, sorry for the delay.
>
> Could you please test with the patch below, instead of reverting
> b1a1442a23? Thanks a lot.
>
>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 253fe23..81eacd3 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int 
> type, u8 *data, int size,
> csize--;
> }
>
> -   rsize = ((report->size - 1) >> 3) + 1;
> +   rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;

Isn't "report->id" already covered by "if (report_enum->numbered)"
above? The test for "id > 0" won't work here as in this case
"report_enum->numbered" must already be set to true by the hid-desc
parser, doesn't it?

Where exactly did you get the +7 from?

What worries me here is that we write over the @data buffer from the
hid-ll-driver if the report is too short. I don't think the BT driver
accounts for that, mhh.

David

> if (rsize > HID_MAX_BUFFER_SIZE)
> rsize = HID_MAX_BUFFER_SIZE;
>
> --
> Jiri Kosina
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

2013-12-19 Thread David Herrmann
Hi

On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina  wrote:
> On Thu, 19 Dec 2013, David Herrmann wrote:
>
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 253fe23..81eacd3 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int 
>> > type, u8 *data, int size,
>> > csize--;
>> > }
>> >
>> > -   rsize = ((report->size - 1) >> 3) + 1;
>> > +   rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>> above? The test for "id > 0" won't work here as in this case
>> "report_enum->numbered" must already be set to true by the hid-desc
>> parser, doesn't it?
>
> Right, that one is not correct here, thanks.
>
>> Where exactly did you get the +7 from?
>
> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.

Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
really breaks BT drivers:

commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina 
Date:   Mon Jun 3 11:27:48 2013 +0200

HID: core: fix reporting of raw events

Problem is, if the raw_event() callback returned 0 earlier, we just
skipped raw input reports. However, we now always call the
hid_report_raw_event() helper. Which is normally fine, but the helper
expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
*not* true for HIDP. So the memset() writes over some random memory.

I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
be allocated on the stack, but we're in atomic-context here so a
kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
look into HIDP to make the skb big enough, but I'm not sure how we can
achieve that.

So off the top of my head, the best idea is to add "char
inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
copy every input-report into the buffer before passing to
hid_input_report().

Ideas?
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

2013-12-19 Thread David Herrmann
Hi

On Thu, Dec 19, 2013 at 11:08 AM, David Herrmann  wrote:
> Hi
>
> On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina  wrote:
>> On Thu, 19 Dec 2013, David Herrmann wrote:
>>
>>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> > index 253fe23..81eacd3 100644
>>> > --- a/drivers/hid/hid-core.c
>>> > +++ b/drivers/hid/hid-core.c
>>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, 
>>> > int type, u8 *data, int size,
>>> > csize--;
>>> > }
>>> >
>>> > -   rsize = ((report->size - 1) >> 3) + 1;
>>> > +   rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>>
>>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>>> above? The test for "id > 0" won't work here as in this case
>>> "report_enum->numbered" must already be set to true by the hid-desc
>>> parser, doesn't it?
>>
>> Right, that one is not correct here, thanks.
>>
>>> Where exactly did you get the +7 from?
>>
>> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
>
> Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
> really breaks BT drivers:
>
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina 
> Date:   Mon Jun 3 11:27:48 2013 +0200
>
> HID: core: fix reporting of raw events
>
> Problem is, if the raw_event() callback returned 0 earlier, we just
> skipped raw input reports. However, we now always call the
> hid_report_raw_event() helper. Which is normally fine, but the helper
> expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
> *not* true for HIDP. So the memset() writes over some random memory.
>
> I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
> be allocated on the stack, but we're in atomic-context here so a
> kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
> look into HIDP to make the skb big enough, but I'm not sure how we can
> achieve that.
>
> So off the top of my head, the best idea is to add "char
> inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
> copy every input-report into the buffer before passing to
> hid_input_report().

As this thread doesn't really contain any oops message nor the exact
driver name (except mentioning hyperv and magicmouse), I fixed a
related bug for HIDP:
  http://thread.gmane.org/gmane.linux.bluez.kernel/41969

A similar patch is *really* required for hid-hyperv.c as it also does
not provide a properly sized buffer to hid_input_report().

David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH RFC 21/46] drm: provide a helper for the encoder possible_crtcs mask

2014-01-03 Thread David Herrmann
Hi

On Thu, Jan 2, 2014 at 10:27 PM, Russell King
 wrote:
> The encoder possible_crtcs mask identifies which CRTCs can be bound to
> a particular encoder.  Each bit from bit 0 defines an index in the list
> of CRTCs held in the DRM mode_config crtc_list.  Rather than having
> drivers trying to track the position of their CRTCs in the list, expose
> the code which already exists for calculating the appropriate mask bit
> for a CRTC.
>
> Signed-off-by: Russell King 
> ---
>  drivers/gpu/drm/drm_crtc_helper.c |   39 
>  include/drm/drm_crtc_helper.h |1 +
>  2 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 01361aba033b..10708c248196 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct 
> drm_device *dev)
>  EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>
>  /**
> + * drm_helper_crtc_possible_mask - find the mask of a registered CRTC
> + * @crtc: crtc to find mask for
> + *
> + * Given a registered CRTC, return the mask bit of that CRTC for an
> + * encoder's possible_crtcs field.

I think this is a nice cleanup which we could pick up right away. Most
drivers can be simplified by using this. Only the name is misleading,
imo, I'd use something like drm_helper_crtc_to_mask(). Anyhow, not my
call so:

Reviewed-by: David Herrmann 

Thanks
David

> + */
> +uint32_t drm_helper_crtc_possible_mask(struct drm_crtc *crtc)
> +{
> +   struct drm_device *dev = crtc->dev;
> +   struct drm_crtc *tmp;
> +   uint32_t crtc_mask = 1;
> +
> +   list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) {
> +   if (tmp == crtc)
> +   return crtc_mask;
> +   crtc_mask <<= 1;
> +   }
> +
> +   return 0;
> +}
> +EXPORT_SYMBOL(drm_helper_crtc_possible_mask);
> +
> +/**
>   * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
>   * @encoder: encoder to test
>   * @crtc: crtc to test
> @@ -334,23 +357,13 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>  static bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
> struct drm_crtc *crtc)
>  {
> -   struct drm_device *dev;
> -   struct drm_crtc *tmp;
> -   int crtc_mask = 1;
> +   uint32_t crtc_mask;
>
> WARN(!crtc, "checking null crtc?\n");
>
> -   dev = crtc->dev;
> -
> -   list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) {
> -   if (tmp == crtc)
> -   break;
> -   crtc_mask <<= 1;
> -   }
> +   crtc_mask = drm_helper_crtc_possible_mask(crtc);
>
> -   if (encoder->possible_crtcs & crtc_mask)
> -   return true;
> -   return false;
> +   return !!(encoder->possible_crtcs & crtc_mask);
>  }
>
>  /*
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index ef6ad3a8e58e..21b9f47dd88c 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -127,6 +127,7 @@ struct drm_connector_helper_funcs {
>
>  extern int drm_helper_probe_single_connector_modes(struct drm_connector 
> *connector, uint32_t maxX, uint32_t maxY);
>  extern void drm_helper_disable_unused_functions(struct drm_device *dev);
> +extern uint32_t drm_helper_crtc_possible_mask(struct drm_crtc *crtc);
>  extern int drm_crtc_helper_set_config(struct drm_mode_set *set);
>  extern bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  struct drm_display_mode *mode,
> --
> 1.7.4.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: [PATCH RFC 21/46] drm: provide a helper for the encoder possible_crtcs mask

2014-01-03 Thread David Herrmann
Hi

On Fri, Jan 3, 2014 at 5:13 PM, Russell King - ARM Linux
 wrote:
> On Fri, Jan 03, 2014 at 05:05:46PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Thu, Jan 2, 2014 at 10:27 PM, Russell King
>>  wrote:
>> > The encoder possible_crtcs mask identifies which CRTCs can be bound to
>> > a particular encoder.  Each bit from bit 0 defines an index in the list
>> > of CRTCs held in the DRM mode_config crtc_list.  Rather than having
>> > drivers trying to track the position of their CRTCs in the list, expose
>> > the code which already exists for calculating the appropriate mask bit
>> > for a CRTC.
>> >
>> > Signed-off-by: Russell King 
>> > ---
>> >  drivers/gpu/drm/drm_crtc_helper.c |   39 
>> > 
>> >  include/drm/drm_crtc_helper.h |1 +
>> >  2 files changed, 27 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
>> > b/drivers/gpu/drm/drm_crtc_helper.c
>> > index 01361aba033b..10708c248196 100644
>> > --- a/drivers/gpu/drm/drm_crtc_helper.c
>> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> > @@ -325,6 +325,29 @@ void drm_helper_disable_unused_functions(struct 
>> > drm_device *dev)
>> >  EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>> >
>> >  /**
>> > + * drm_helper_crtc_possible_mask - find the mask of a registered CRTC
>> > + * @crtc: crtc to find mask for
>> > + *
>> > + * Given a registered CRTC, return the mask bit of that CRTC for an
>> > + * encoder's possible_crtcs field.
>>
>> I think this is a nice cleanup which we could pick up right away. Most
>> drivers can be simplified by using this. Only the name is misleading,
>> imo, I'd use something like drm_helper_crtc_to_mask().
>
> I'm not so sure - since the only place this mask gets used is with
> the "possible_crtcs" field.  It's got nothing to do with the
> "possible_clones" field as that isn't a mask of CRTCs.

At least intel's copy of intel_crtc_encoder_ok() can be updated, too.
But they don't depend on the crtc_helpers so we'd have to move your
small helper into drm_crtc.c (which is fine to me as it is not limited
to the DRM-crtc-helpers).

possible_clones is about encoders, you're right, I missed that. So you
can carry it in your series just fine.

Thanks
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: source code file for the generic vga driver?

2013-08-16 Thread David Herrmann
Hi

On Mon, Aug 5, 2013 at 9:12 PM, Haiyang Zhang  wrote:
> Hi folks,
>
> I'm working on an issue of HyperV synthetic frame buffer driver, which seems 
> to have
> a conflict with the generic vga driver (not the vesa driver). I hope to read 
> and trace into
> the source code for the generic vga driver...
>
> Can anyone point me to the source code file for the generic vga driver in the 
> kernel tree?

Everything lives in ./drivers/video/. The drivers you're probably
interested in are "vesafb.c" or "vga16fb.c". There is also the
"vgacon" driver in ./drivers/video/console/vgacon.c. I am not sure
which one you are talking about.

You might also want to have a look at the x86 sysfb infrastructure
which isn't merged, yet:
http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=x86/fb
It provides proper platform-devices so drivers no longer conflict on
the vga/vesa/efi.. framebuffer resources. It's x86 only as all the
relevant drivers only work on x86.

If you give some more information on what you are trying to do, I can
point you to the relevant resources. My guess is that you want to have
a look at remove_conflicting_framebuffers() in
./drivers/video/fbmem.c.

Regards
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: conflict of hyperv_fb and the generic video driver?

2013-08-16 Thread David Herrmann
Hi

(hint: your mail header seems to drop Reference-to/Reply-to headers
and breaks thread-info)

On Fri, Aug 16, 2013 at 8:45 PM, Haiyang Zhang  wrote:
>
>
>> -Original Message-----
>> From: David Herrmann [mailto:dh.herrm...@gmail.com]
>> Sent: Friday, August 16, 2013 9:46 AM
>> To: Haiyang Zhang
>> Cc: linux-fb...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
>> Tomi Valkeinen; Jean-Christophe Plagniol-Villard; akpm@linux-
>> foundation.org; KY Srinivasan
>> Subject: Re: source code file for the generic vga driver?
>>
>> Hi
>>
>> On Mon, Aug 5, 2013 at 9:12 PM, Haiyang Zhang 
>> wrote:
>> > Hi folks,
>> >
>> > I'm working on an issue of HyperV synthetic frame buffer driver, which
>> > seems to have a conflict with the generic vga driver (not the vesa
>> > driver). I hope to read and trace into the source code for the generic vga
>> driver...
>> >
>> > Can anyone point me to the source code file for the generic vga driver in
>> the kernel tree?
>>
>> Everything lives in ./drivers/video/. The drivers you're probably interested 
>> in
>> are "vesafb.c" or "vga16fb.c". There is also the "vgacon" driver
>> in ./drivers/video/console/vgacon.c. I am not sure which one you are talking
>> about.
>>
>> You might also want to have a look at the x86 sysfb infrastructure which 
>> isn't
>> merged, yet:
>> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=x86/fb
>> It provides proper platform-devices so drivers no longer conflict on the
>> vga/vesa/efi.. framebuffer resources. It's x86 only as all the relevant 
>> drivers
>> only work on x86.
>>
>> If you give some more information on what you are trying to do, I can point
>> you to the relevant resources. My guess is that you want to have a look at
>> remove_conflicting_framebuffers() in ./drivers/video/fbmem.c.
>
> Thank you for the detailed reply!
>
> I'm looking at a problem of Hyper-V synthetic fb driver (hyperv_fb) which 
> seems
> to have conflict with some kind of generic video driver. I'm not sure which 
> driver
> is this.
>
> On Suse, the vesafb is removed automatically by 
> remove_conflicting_framebuffers()
> when hyperv_fb is loaded. We don't have any problem here.
>
> On some other Distros, like RHEL, CentOS, Ubuntu, the generic driver seems not
> to be vesafb -- I can't see any /dev/fb* there. And, the generic video driver 
> seems not be
> removed when hyperv_fb is loaded. This generic video driver is not vesafb or 
> vga16fb or
> vgacon, because it supports x-window GUI, and it's still here after I 
> un-configured vesafb
> and vga16fb.
>
> Could point out what is the generic video driver used by RHEL, Ubuntu by 
> default? And,
> how to let it exit automatically when our FB driver (hyperv_fb) is loaded?

I have no idea what RHEL or Ubuntu use, sorry. But your description
sounds odd. The kernel has 3 different places that could use VGA
resources:
 - fbdev drivers (/dev/fbX or /sys/class/graphics)
 - DRM drivers (/dev/dri/cardX or /sys/class/drm)
 - vgacon
Could you check whether these directories are empty?
(/sys/class/graphics/ and /sys/class/drm/ on a running machine)

If these are empty, this doesn't look like a kernel thing. Are you
sure it's a kernel driver that accesses your VGA resources? The
xserver used to have some pretty huge vga/vesa/.. user-space drivers.

Could you tell me whether the linux-console actually works? That is,
do you get some console output if xserver is not running?

Cheers
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


Re: conflict of hyperv_fb and the generic video driver?

2013-08-16 Thread David Herrmann
Hi

On Fri, Aug 16, 2013 at 10:27 PM, Haiyang Zhang  wrote:
>
>
>> -Original Message-----
>> From: David Herrmann [mailto:dh.herrm...@gmail.com]
>> Sent: Friday, August 16, 2013 3:11 PM
>> To: Haiyang Zhang
>> Cc: linux-fb...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
>> Tomi Valkeinen; Jean-Christophe Plagniol-Villard; akpm@linux-
>> foundation.org; KY Srinivasan
>> Subject: Re: conflict of hyperv_fb and the generic video driver?
>>
>> Hi
>>
>> (hint: your mail header seems to drop Reference-to/Reply-to headers
>> and breaks thread-info)
>>
>> On Fri, Aug 16, 2013 at 8:45 PM, Haiyang Zhang 
>> wrote:
>> >
>> >
>> >> -Original Message-
>> >> From: David Herrmann [mailto:dh.herrm...@gmail.com]
>> >> Sent: Friday, August 16, 2013 9:46 AM
>> >> To: Haiyang Zhang
>> >> Cc: linux-fb...@vger.kernel.org; driverdev-devel@linuxdriverproject.org;
>> >> Tomi Valkeinen; Jean-Christophe Plagniol-Villard; akpm@linux-
>> >> foundation.org; KY Srinivasan
>> >> Subject: Re: source code file for the generic vga driver?
>> >>
>> >> Hi
>> >>
>> >> On Mon, Aug 5, 2013 at 9:12 PM, Haiyang Zhang
>> 
>> >> wrote:
>> >> > Hi folks,
>> >> >
>> >> > I'm working on an issue of HyperV synthetic frame buffer driver, which
>> >> > seems to have a conflict with the generic vga driver (not the vesa
>> >> > driver). I hope to read and trace into the source code for the generic 
>> >> > vga
>> >> driver...
>> >> >
>> >> > Can anyone point me to the source code file for the generic vga driver 
>> >> > in
>> >> the kernel tree?
>> >>
>> >> Everything lives in ./drivers/video/. The drivers you're probably 
>> >> interested
>> in
>> >> are "vesafb.c" or "vga16fb.c". There is also the "vgacon" driver
>> >> in ./drivers/video/console/vgacon.c. I am not sure which one you are
>> talking
>> >> about.
>> >>
>> >> You might also want to have a look at the x86 sysfb infrastructure which
>> isn't
>> >> merged, yet:
>> >> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=x86/fb
>> >> It provides proper platform-devices so drivers no longer conflict on the
>> >> vga/vesa/efi.. framebuffer resources. It's x86 only as all the relevant
>> drivers
>> >> only work on x86.
>> >>
>> >> If you give some more information on what you are trying to do, I can
>> point
>> >> you to the relevant resources. My guess is that you want to have a look at
>> >> remove_conflicting_framebuffers() in ./drivers/video/fbmem.c.
>> >
>> > Thank you for the detailed reply!
>> >
>> > I'm looking at a problem of Hyper-V synthetic fb driver (hyperv_fb) which
>> seems
>> > to have conflict with some kind of generic video driver. I'm not sure which
>> driver
>> > is this.
>> >
>> > On Suse, the vesafb is removed automatically by
>> remove_conflicting_framebuffers()
>> > when hyperv_fb is loaded. We don't have any problem here.
>> >
>> > On some other Distros, like RHEL, CentOS, Ubuntu, the generic driver
>> seems not
>> > to be vesafb -- I can't see any /dev/fb* there. And, the generic video 
>> > driver
>> seems not be
>> > removed when hyperv_fb is loaded. This generic video driver is not vesafb
>> or vga16fb or
>> > vgacon, because it supports x-window GUI, and it's still here after I un-
>> configured vesafb
>> > and vga16fb.
>> >
>> > Could point out what is the generic video driver used by RHEL, Ubuntu by
>> default? And,
>> > how to let it exit automatically when our FB driver (hyperv_fb) is loaded?
>>
>> I have no idea what RHEL or Ubuntu use, sorry. But your description
>> sounds odd. The kernel has 3 different places that could use VGA
>> resources:
>>  - fbdev drivers (/dev/fbX or /sys/class/graphics)
>>  - DRM drivers (/dev/dri/cardX or /sys/class/drm)
>>  - vgacon
>> Could you check whether these directories are empty?
>> (/sys/class/graphics/ and /sys/class/drm/ on a running machine)
>>
>> If these are empty, this doesn't look like a kernel thing. Are you
>> sure it's a kernel driver that accesses your

Re: conflict of hyperv_fb and the generic video driver?

2013-08-16 Thread David Herrmann
Hi

On Sat, Aug 17, 2013 at 1:04 AM, Haiyang Zhang  wrote:
>> > But I saw VESA VBE in the x log. Seems it's the default driver:
>> > "/var/log/Xorg.0.log":
>> > [12.340] (II) VESA(0): Primary V_BIOS segment is: 0xc000
>> > [12.341] (II) VESA(0): VESA BIOS detected
>> > [12.341] (II) VESA(0): VESA VBE Version 2.0
>> > [12.341] (II) VESA(0): VESA VBE Total Mem: 4096 kB
>> > [12.341] (II) VESA(0): VESA VBE OEM: IBM SVGA BIOS, (C) 1993
>> International Business Machines
>> > [12.341] (II) VESA(0): VESA VBE OEM Software Rev: 0.0
>> > [12.365] (II) VESA(0): Creating default Display subsection in Screen
>> section
>> > "Default Screen Section" for depth/fbbpp 24/32
>> > [12.365] (==) VESA(0): Depth 24, (--) framebuffer bpp 32
>> > [12.365] (==) VESA(0): RGB weight 888
>> > [12.365] (==) VESA(0): Default visual is TrueColor
>> > [12.365] (==) VESA(0): Using gamma correction (1.0, 1.0, 1.0)
>> >
>> > There is no  /dev/fb*,  /dev/dri/, /sys/class/drm I see
>> > /sys/class/graphics/fbcon is here.  But console output is not working.
>> >
>> > Seems that the VESA VBE is causing conflict with my driver... Is there
>> > any way to disable VESA VBE driver?
>>
>> This is the Xorg vesa driver. There is no way to detect that from the kernel 
>> (at
>> least no sane way). Just uninstall the vesa driver, no one uses that these 
>> days.
>> At least I see no reason why you would use it.
>> Probably named xf86-video-vesa. Or make sure your hyperv fbdev driver is
>> loaded before xorg starts and then load xf86-video-fbdev over xf86-video-
>> vesa.
>
> Removing the xorg vesa driver has solved the conflict.
>
> Also, I found adding a xorg.conf which specifies fbdev can solve the problem 
> too:
> [root@localhost ~]# cat /etc/X11/xorg.conf
> Section "Device"
> Identifier  "HYPER-V Framebuffer"
> Driver  "fbdev"
> EndSection
>
> Thank you SO MUCH for the help!!

You're welcome. Good to hear it's solved now.

Cheers
David
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel