Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Fri, Feb 03, 2017 at 12:25:24PM +, Emil Velikov wrote:
> On 3 February 2017 at 08:00, Daniel Vetter  wrote:
> > On Thu, Feb 02, 2017 at 12:37:21PM +, Emil Velikov wrote:
> >>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
> >> review/commit history in the area and/or the kernel in general ?
> >> In this case, Peter have quite noticeable experience [in kernel
> >> development] with little-to no in DRM.
> >> Should one draw a line in the sand somewhere ?
> >
> > We're fairly lenient with accepting new drivers already, and for drivers
> > in drm-misc we require some checks and peer review to make sure new folks
> > learn faster. But it's an expirement, I fully expect some fallout and then
> > some learnign and fine tuning from that, and maybe we even need to crank
> > requirements back up to a much higher level of experience.
> >
> > But for drivers I'm honestly not too concerned: As long as there's some
> > process in place to foster learning (which I think will be
> > better with this) I really don't see much harm in merging non-perfect
> > driver code.
> >
> Thank you Daniel, this is very well said. Being lenient to begin with
> and adjusting where/if needed is the more welcoming approach, indeed.
> Doubt there'll be (m)any cases of (ab|mis)use but even if so, nobody
> can see the future.

Yeah, the big idea behind drm-misc is also to make drm more welcoming for
new folks, and I think starting with a small driver or a few driver
patches for hw you have lying around for hacking is a great way. And every
once in a while we can "trick" one of these newbies into doing some cool
core refactoring or cleanup, and maybe even to stick around for a while.

All for world domination and other evil purposes ofc :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 3 February 2017 at 08:00, Daniel Vetter  wrote:
> On Thu, Feb 02, 2017 at 12:37:21PM +, Emil Velikov wrote:
>>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
>> review/commit history in the area and/or the kernel in general ?
>> In this case, Peter have quite noticeable experience [in kernel
>> development] with little-to no in DRM.
>> Should one draw a line in the sand somewhere ?
>
> We're fairly lenient with accepting new drivers already, and for drivers
> in drm-misc we require some checks and peer review to make sure new folks
> learn faster. But it's an expirement, I fully expect some fallout and then
> some learnign and fine tuning from that, and maybe we even need to crank
> requirements back up to a much higher level of experience.
>
> But for drivers I'm honestly not too concerned: As long as there's some
> process in place to foster learning (which I think will be
> better with this) I really don't see much harm in merging non-perfect
> driver code.
>
Thank you Daniel, this is very well said. Being lenient to begin with
and adjusting where/if needed is the more welcoming approach, indeed.
Doubt there'll be (m)any cases of (ab|mis)use but even if so, nobody
can see the future.

>>  - You patch has been on a long [bit rocky road] for a while, with
>> devs giving you [what seems like] a partial reviews.
>> Have you considered reviewing others' patches in exchange for a [more
>> complete one] of this one ? According to git log people have poked you
>> a handful of times, but seemingly you were busy.
>
> Just a clarification: When I review drivers I don't do a fairly cursor
> look, hunting for areas where some refactoring and align with drm best
> practices would be good. And I think that's perfectly fine and enough to
> get a driver landed, but we're definitely not 100% consistent on this
> within drm-misc. Probably will take some time to figure this out.
Seems like I've missed "Peter, your patch..." above, silly me.

Thanks for answering my [what may seem silly] questions ;-)
Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Thu, Feb 02, 2017 at 12:37:21PM +, Emil Velikov wrote:
>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
> review/commit history in the area and/or the kernel in general ?
> In this case, Peter have quite noticeable experience [in kernel
> development] with little-to no in DRM.
> Should one draw a line in the sand somewhere ?

We're fairly lenient with accepting new drivers already, and for drivers
in drm-misc we require some checks and peer review to make sure new folks
learn faster. But it's an expirement, I fully expect some fallout and then
some learnign and fine tuning from that, and maybe we even need to crank
requirements back up to a much higher level of experience.

But for drivers I'm honestly not too concerned: As long as there's some
process in place to foster learning (which I think will be
better with this) I really don't see much harm in merging non-perfect
driver code.

>  - You patch has been on a long [bit rocky road] for a while, with
> devs giving you [what seems like] a partial reviews.
> Have you considered reviewing others' patches in exchange for a [more
> complete one] of this one ? According to git log people have poked you
> a handful of times, but seemingly you were busy.

Just a clarification: When I review drivers I don't do a fairly cursor
look, hunting for areas where some refactoring and align with drm best
practices would be good. And I think that's perfectly fine and enough to
get a driver landed, but we're definitely not 100% consistent on this
within drm-misc. Probably will take some time to figure this out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Archit Taneja]

On 02/01/2017 05:51 PM, Peter Senna Tschudin wrote:


On 01 February, 2017 12:35 CET, Daniel Vetter  wrote:


On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:

Hi Archit,

On 01 February, 2017 10:44 CET, Archit Taneja  wrote:




On 01/30/2017 10:35 PM, Jani Nikula wrote:

On Sat, 28 Jan 2017, Peter Senna Tschudin  wrote:

On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
Hi Archit,

Thank you for the comments!

[...]

+   total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
+   if (total_size > EDID_LENGTH) {
+   kfree(block);
+   block = kmalloc(total_size, GFP_KERNEL);
+   if (!block)
+   return NULL;
+
+   /* Yes, read the entire buffer, and do not skip the first
+* EDID_LENGTH bytes.
+*/


Is this the reason why you aren't using drm_do_get_edid()?




Yes, for some hw specific reason, it is necessary to read the entire
EDID buffer starting from 0, not block by block.


Hrmh, I'm planning on moving the edid override and firmware edid
mechanisms at the drm_do_get_edid() level to be able to truly and
transparently use a different edid. Currently, they're only used for
modes, really, and lead to some info retrieved from overrides, some from
the real edid. This kind of hacks will bypass the override/firmware edid
mechanisms then too. :(


It seems like there is a HW issue which prevents them from reading EDID
from an offset. So, I'm not sure if it is a hack or a HW limitation.




One way around this would be to hide the HW requirement in the
get_edid_block func pointer passed to drm_do_get_edid(). This
would, however, result in more i2c reads (equal to # of extension
blocks) than what the patch currently does.

Peter, if you think doing extra EDID reads isn't too costly on your
platform, you could consider using drm_do_get_edid(). If not, I guess
you'll miss out on the additional functionality Jani is going to add



in the future.


My concern is that for almost one year now, every time I fix something
one or two new requests are made. I'm happy to fix the driver, but I
want a list of the changes that are required to get it upstream, before
I make more changes. Can we agree on exactly what is preventing this
driver to get upstream? Then I'll fix it.


I think addressing this edid reading question post-merge is perfectly
fine. Aside, want to keep maintaing your stuff as part of the drm-misc
group, with the drivers-in-misc experiment?


The edid thing was only a suggestion. As Daniel said, it's okay to work on
it post merge.

Please do fix the minor comments I mentioned in the latest patch set. I'll
pull in the first 3 patches once Rob H gives an Ack on the DT bindings
patch. The 4th patch needs to go through the SoC maintainer.

Thanks,
Archit



Yes, sure!


-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch









--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

 
On 02 February, 2017 02:46 CET, Emil Velikov  wrote: 
 
> On 1 February 2017 at 11:35, Daniel Vetter  wrote:
> > On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:
> >> Hi Archit,
> >>
> >> On 01 February, 2017 10:44 CET, Archit Taneja  
> >> wrote:
> >>
> >> >
> >> >
> >> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> >> > > On Sat, 28 Jan 2017, Peter Senna Tschudin  
> >> > > wrote:
> >> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> >> > >> Hi Archit,
> >> > >>
> >> > >> Thank you for the comments!
> >> > >>
> >> > >> [...]
> >> >  +  total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> >> >  +  if (total_size > EDID_LENGTH) {
> >> >  +  kfree(block);
> >> >  +  block = kmalloc(total_size, GFP_KERNEL);
> >> >  +  if (!block)
> >> >  +  return NULL;
> >> >  +
> >> >  +  /* Yes, read the entire buffer, and do not skip the 
> >> >  first
> >> >  +   * EDID_LENGTH bytes.
> >> >  +   */
> >> > >>>
> >> > >>> Is this the reason why you aren't using drm_do_get_edid()?
> >>
> >> > >>
> >> > >> Yes, for some hw specific reason, it is necessary to read the entire
> >> > >> EDID buffer starting from 0, not block by block.
> >> > >
> >> > > Hrmh, I'm planning on moving the edid override and firmware edid
> >> > > mechanisms at the drm_do_get_edid() level to be able to truly and
> >> > > transparently use a different edid. Currently, they're only used for
> >> > > modes, really, and lead to some info retrieved from overrides, some 
> >> > > from
> >> > > the real edid. This kind of hacks will bypass the override/firmware 
> >> > > edid
> >> > > mechanisms then too. :(
> >> >
> >> > It seems like there is a HW issue which prevents them from reading EDID
> >> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> >>
> >> >
> >> > One way around this would be to hide the HW requirement in the
> >> > get_edid_block func pointer passed to drm_do_get_edid(). This
> >> > would, however, result in more i2c reads (equal to # of extension
> >> > blocks) than what the patch currently does.
> >> >
> >> > Peter, if you think doing extra EDID reads isn't too costly on your
> >> > platform, you could consider using drm_do_get_edid(). If not, I guess
> >> > you'll miss out on the additional functionality Jani is going to add
> >>
> >> > in the future.
> >>
> >> My concern is that for almost one year now, every time I fix something
> >> one or two new requests are made. I'm happy to fix the driver, but I
> >> want a list of the changes that are required to get it upstream, before
> >> I make more changes. Can we agree on exactly what is preventing this
> >> driver to get upstream? Then I'll fix it.
> >
> > I think addressing this edid reading question post-merge is perfectly
> > fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> > group, with the drivers-in-misc experiment?
> 
> Wasn't there some entry level for people ? It's not like Peter is
> thick or anything, just that he has not reviewed or replied (afaict)
> to any patches, even on ones where he was explicitly cc'd.
> Although he's got dozens of contributions elsewhere, there's only a
> single patch of his in DRM.

"Any" is an exaggeration. See:

c6c1f9bc798bee7cf
...
Tested-by: Peter Senna Tschudin 

Even if my involvement with drm started with my driver last year, I took my 
personal time to report this issue, and to test multiple iterations of the fix. 
Then my "single patch in DRM" involved some work to get it right, and it is 
something, that made imx-ldb better by adding a feature it did not support 
before.

You made me curious. Can you point to the explicit cases where Peter "has not 
reviewed or replied to any patches, even on ones where he was explicitly cc'd"? 
And can you list case by case your take on why I should have done differently?

Also it is not clear to me what the problem really is here. I'm assuming is not 
me, but if I was qualified from your perspective then would be no problem, 
right?






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

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 1 February 2017 at 11:35, Daniel Vetter  wrote:
> On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:
>> Hi Archit,
>>
>> On 01 February, 2017 10:44 CET, Archit Taneja  wrote:
>>
>> >
>> >
>> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
>> > > On Sat, 28 Jan 2017, Peter Senna Tschudin  
>> > > wrote:
>> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>> > >> Hi Archit,
>> > >>
>> > >> Thank you for the comments!
>> > >>
>> > >> [...]
>> >  +  total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> >  +  if (total_size > EDID_LENGTH) {
>> >  +  kfree(block);
>> >  +  block = kmalloc(total_size, GFP_KERNEL);
>> >  +  if (!block)
>> >  +  return NULL;
>> >  +
>> >  +  /* Yes, read the entire buffer, and do not skip the 
>> >  first
>> >  +   * EDID_LENGTH bytes.
>> >  +   */
>> > >>>
>> > >>> Is this the reason why you aren't using drm_do_get_edid()?
>>
>> > >>
>> > >> Yes, for some hw specific reason, it is necessary to read the entire
>> > >> EDID buffer starting from 0, not block by block.
>> > >
>> > > Hrmh, I'm planning on moving the edid override and firmware edid
>> > > mechanisms at the drm_do_get_edid() level to be able to truly and
>> > > transparently use a different edid. Currently, they're only used for
>> > > modes, really, and lead to some info retrieved from overrides, some from
>> > > the real edid. This kind of hacks will bypass the override/firmware edid
>> > > mechanisms then too. :(
>> >
>> > It seems like there is a HW issue which prevents them from reading EDID
>> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
>>
>> >
>> > One way around this would be to hide the HW requirement in the
>> > get_edid_block func pointer passed to drm_do_get_edid(). This
>> > would, however, result in more i2c reads (equal to # of extension
>> > blocks) than what the patch currently does.
>> >
>> > Peter, if you think doing extra EDID reads isn't too costly on your
>> > platform, you could consider using drm_do_get_edid(). If not, I guess
>> > you'll miss out on the additional functionality Jani is going to add
>>
>> > in the future.
>>
>> My concern is that for almost one year now, every time I fix something
>> one or two new requests are made. I'm happy to fix the driver, but I
>> want a list of the changes that are required to get it upstream, before
>> I make more changes. Can we agree on exactly what is preventing this
>> driver to get upstream? Then I'll fix it.
>
> I think addressing this edid reading question post-merge is perfectly
> fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> group, with the drivers-in-misc experiment?

Wasn't there some entry level for people ? It's not like Peter is
thick or anything, just that he has not reviewed or replied (afaict)
to any patches, even on ones where he was explicitly cc'd.
Although he's got dozens of contributions elsewhere, there's only a
single patch of his in DRM.

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

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 2 February 2017 at 11:53, Peter Senna Tschudin
 wrote:
>
> On 02 February, 2017 02:46 CET, Emil Velikov  wrote:
>
>> On 1 February 2017 at 11:35, Daniel Vetter  wrote:
>> > On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:
>> >> Hi Archit,
>> >>
>> >> On 01 February, 2017 10:44 CET, Archit Taneja  
>> >> wrote:
>> >>
>> >> >
>> >> >
>> >> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
>> >> > > On Sat, 28 Jan 2017, Peter Senna Tschudin  
>> >> > > wrote:
>> >> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>> >> > >> Hi Archit,
>> >> > >>
>> >> > >> Thank you for the comments!
>> >> > >>
>> >> > >> [...]
>> >> >  +  total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> >> >  +  if (total_size > EDID_LENGTH) {
>> >> >  +  kfree(block);
>> >> >  +  block = kmalloc(total_size, GFP_KERNEL);
>> >> >  +  if (!block)
>> >> >  +  return NULL;
>> >> >  +
>> >> >  +  /* Yes, read the entire buffer, and do not skip the 
>> >> >  first
>> >> >  +   * EDID_LENGTH bytes.
>> >> >  +   */
>> >> > >>>
>> >> > >>> Is this the reason why you aren't using drm_do_get_edid()?
>> >>
>> >> > >>
>> >> > >> Yes, for some hw specific reason, it is necessary to read the entire
>> >> > >> EDID buffer starting from 0, not block by block.
>> >> > >
>> >> > > Hrmh, I'm planning on moving the edid override and firmware edid
>> >> > > mechanisms at the drm_do_get_edid() level to be able to truly and
>> >> > > transparently use a different edid. Currently, they're only used for
>> >> > > modes, really, and lead to some info retrieved from overrides, some 
>> >> > > from
>> >> > > the real edid. This kind of hacks will bypass the override/firmware 
>> >> > > edid
>> >> > > mechanisms then too. :(
>> >> >
>> >> > It seems like there is a HW issue which prevents them from reading EDID
>> >> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
>> >>
>> >> >
>> >> > One way around this would be to hide the HW requirement in the
>> >> > get_edid_block func pointer passed to drm_do_get_edid(). This
>> >> > would, however, result in more i2c reads (equal to # of extension
>> >> > blocks) than what the patch currently does.
>> >> >
>> >> > Peter, if you think doing extra EDID reads isn't too costly on your
>> >> > platform, you could consider using drm_do_get_edid(). If not, I guess
>> >> > you'll miss out on the additional functionality Jani is going to add
>> >>
>> >> > in the future.
>> >>
>> >> My concern is that for almost one year now, every time I fix something
>> >> one or two new requests are made. I'm happy to fix the driver, but I
>> >> want a list of the changes that are required to get it upstream, before
>> >> I make more changes. Can we agree on exactly what is preventing this
>> >> driver to get upstream? Then I'll fix it.
>> >
>> > I think addressing this edid reading question post-merge is perfectly
>> > fine. Aside, want to keep maintaing your stuff as part of the drm-misc
>> > group, with the drivers-in-misc experiment?
>>
>> Wasn't there some entry level for people ? It's not like Peter is
>> thick or anything, just that he has not reviewed or replied (afaict)
>
>> to any patches, even on ones where he was explicitly cc'd.
>> Although he's got dozens of contributions elsewhere, there's only a
>> single patch of his in DRM.
>
> "Any" is an exaggeration. See:
>
> c6c1f9bc798bee7cf
> ...
> Tested-by: Peter Senna Tschudin 
>
> Even if my involvement with drm started with my driver last year, I took my 
> personal time to report this issue, and to test multiple iterations of the 
> fix. Then my "single patch in DRM" involved some work to get it right, and it 
> is something, that made imx-ldb better by adding a feature it did not support 
> before.
>
> You made me curious. Can you point to the explicit cases where Peter "has not 
> reviewed or replied to any patches, even on ones where he was explicitly 
> cc'd"? And can you list case by case your take on why I should have done 
> differently?
>
> Also it is not clear to me what the problem really is here. I'm assuming is 
> not me, but if I was qualified from your perspective then would be no 
> problem, right?
>
Ok seems like my wording came out, rather off.

Peter, as mentioned on IRC my messages is not aimed to be picking on
you in the slightest way. Pardon if it came as such.

It had two main points:
 - Daniel, gents - is drm-misc aimed at devs with limited (no?)
review/commit history in the area and/or the kernel in general ?
In this case, Peter have quite noticeable experience [in kernel
development] with little-to no in DRM.
Should one draw a line in the sand somewhere ?

 - You patch has been on a long [bit rocky road] 

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

 
On 01 February, 2017 12:35 CET, Daniel Vetter  wrote: 
 
> On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:
> > Hi Archit,
> > 
> > On 01 February, 2017 10:44 CET, Archit Taneja  
> > wrote:
> > 
> > >
> > >
> > > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > > > On Sat, 28 Jan 2017, Peter Senna Tschudin  
> > > > wrote:
> > > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> > > >> Hi Archit,
> > > >>
> > > >> Thank you for the comments!
> > > >>
> > > >> [...]
> > >  +total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > >  +if (total_size > EDID_LENGTH) {
> > >  +kfree(block);
> > >  +block = kmalloc(total_size, GFP_KERNEL);
> > >  +if (!block)
> > >  +return NULL;
> > >  +
> > >  +/* Yes, read the entire buffer, and do not skip the 
> > >  first
> > >  + * EDID_LENGTH bytes.
> > >  + */
> > > >>>
> > > >>> Is this the reason why you aren't using drm_do_get_edid()?
> > 
> > > >>
> > > >> Yes, for some hw specific reason, it is necessary to read the entire
> > > >> EDID buffer starting from 0, not block by block.
> > > >
> > > > Hrmh, I'm planning on moving the edid override and firmware edid
> > > > mechanisms at the drm_do_get_edid() level to be able to truly and
> > > > transparently use a different edid. Currently, they're only used for
> > > > modes, really, and lead to some info retrieved from overrides, some from
> > > > the real edid. This kind of hacks will bypass the override/firmware edid
> > > > mechanisms then too. :(
> > >
> > > It seems like there is a HW issue which prevents them from reading EDID
> > > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> > 
> > >
> > > One way around this would be to hide the HW requirement in the
> > > get_edid_block func pointer passed to drm_do_get_edid(). This
> > > would, however, result in more i2c reads (equal to # of extension
> > > blocks) than what the patch currently does.
> > >
> > > Peter, if you think doing extra EDID reads isn't too costly on your
> > > platform, you could consider using drm_do_get_edid(). If not, I guess
> > > you'll miss out on the additional functionality Jani is going to add
> > 
> > > in the future.
> > 
> > My concern is that for almost one year now, every time I fix something
> > one or two new requests are made. I'm happy to fix the driver, but I
> > want a list of the changes that are required to get it upstream, before
> > I make more changes. Can we agree on exactly what is preventing this
> > driver to get upstream? Then I'll fix it.
> 
> I think addressing this edid reading question post-merge is perfectly
> fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> group, with the drivers-in-misc experiment?

Yes, sure!

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
 
 
 
 


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

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

Hi Archit,
 
On 01 February, 2017 10:44 CET, Archit Taneja  wrote: 

> 
> 
> On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > On Sat, 28 Jan 2017, Peter Senna Tschudin  wrote:
> >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> >> Hi Archit,
> >>
> >> Thank you for the comments!
> >>
> >> [...]
>  +total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>  +if (total_size > EDID_LENGTH) {
>  +kfree(block);
>  +block = kmalloc(total_size, GFP_KERNEL);
>  +if (!block)
>  +return NULL;
>  +
>  +/* Yes, read the entire buffer, and do not skip the 
>  first
>  + * EDID_LENGTH bytes.
>  + */
> >>>
> >>> Is this the reason why you aren't using drm_do_get_edid()?
> >>
> >> Yes, for some hw specific reason, it is necessary to read the entire
> >> EDID buffer starting from 0, not block by block.
> >
> > Hrmh, I'm planning on moving the edid override and firmware edid
> > mechanisms at the drm_do_get_edid() level to be able to truly and
> > transparently use a different edid. Currently, they're only used for
> > modes, really, and lead to some info retrieved from overrides, some from
> > the real edid. This kind of hacks will bypass the override/firmware edid
> > mechanisms then too. :(
> 
> It seems like there is a HW issue which prevents them from reading EDID
> from an offset. So, I'm not sure if it is a hack or a HW limitation.
> 
> One way around this would be to hide the HW requirement in the
> get_edid_block func pointer passed to drm_do_get_edid(). This
> would, however, result in more i2c reads (equal to # of extension
> blocks) than what the patch currently does.
> 
> Peter, if you think doing extra EDID reads isn't too costly on your
> platform, you could consider using drm_do_get_edid(). If not, I guess
> you'll miss out on the additional functionality Jani is going to add
> in the future.

My concern is that for almost one year now, every time I fix something one or 
two new requests are made. I'm happy to fix the driver, but I want a list of 
the changes that are required to get it upstream, before I make more changes. 
Can we agree on exactly what is preventing this driver to get upstream? Then 
I'll fix it.

> 
> Thanks,
> Archit
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >>
> >> [...]
> >>
> >> I fixed all your other suggestions. Thank you!
> >> ___
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
 
 
 
 


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

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Wed, Feb 01, 2017 at 10:58:43AM +, Peter Senna Tschudin wrote:
> Hi Archit,
> 
> On 01 February, 2017 10:44 CET, Archit Taneja  wrote:
> 
> >
> >
> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > > On Sat, 28 Jan 2017, Peter Senna Tschudin  
> > > wrote:
> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> > >> Hi Archit,
> > >>
> > >> Thank you for the comments!
> > >>
> > >> [...]
> >  +  total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> >  +  if (total_size > EDID_LENGTH) {
> >  +  kfree(block);
> >  +  block = kmalloc(total_size, GFP_KERNEL);
> >  +  if (!block)
> >  +  return NULL;
> >  +
> >  +  /* Yes, read the entire buffer, and do not skip the 
> >  first
> >  +   * EDID_LENGTH bytes.
> >  +   */
> > >>>
> > >>> Is this the reason why you aren't using drm_do_get_edid()?
> 
> > >>
> > >> Yes, for some hw specific reason, it is necessary to read the entire
> > >> EDID buffer starting from 0, not block by block.
> > >
> > > Hrmh, I'm planning on moving the edid override and firmware edid
> > > mechanisms at the drm_do_get_edid() level to be able to truly and
> > > transparently use a different edid. Currently, they're only used for
> > > modes, really, and lead to some info retrieved from overrides, some from
> > > the real edid. This kind of hacks will bypass the override/firmware edid
> > > mechanisms then too. :(
> >
> > It seems like there is a HW issue which prevents them from reading EDID
> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> 
> >
> > One way around this would be to hide the HW requirement in the
> > get_edid_block func pointer passed to drm_do_get_edid(). This
> > would, however, result in more i2c reads (equal to # of extension
> > blocks) than what the patch currently does.
> >
> > Peter, if you think doing extra EDID reads isn't too costly on your
> > platform, you could consider using drm_do_get_edid(). If not, I guess
> > you'll miss out on the additional functionality Jani is going to add
> 
> > in the future.
> 
> My concern is that for almost one year now, every time I fix something
> one or two new requests are made. I'm happy to fix the driver, but I
> want a list of the changes that are required to get it upstream, before
> I make more changes. Can we agree on exactly what is preventing this
> driver to get upstream? Then I'll fix it.

I think addressing this edid reading question post-merge is perfectly
fine. Aside, want to keep maintaing your stuff as part of the drm-misc
group, with the drivers-in-misc experiment?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Archit Taneja]

On 01/30/2017 10:35 PM, Jani Nikula wrote:

On Sat, 28 Jan 2017, Peter Senna Tschudin  wrote:

On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
Hi Archit,

Thank you for the comments!

[...]

+   total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
+   if (total_size > EDID_LENGTH) {
+   kfree(block);
+   block = kmalloc(total_size, GFP_KERNEL);
+   if (!block)
+   return NULL;
+
+   /* Yes, read the entire buffer, and do not skip the first
+* EDID_LENGTH bytes.
+*/


Is this the reason why you aren't using drm_do_get_edid()?


Yes, for some hw specific reason, it is necessary to read the entire
EDID buffer starting from 0, not block by block.


Hrmh, I'm planning on moving the edid override and firmware edid
mechanisms at the drm_do_get_edid() level to be able to truly and
transparently use a different edid. Currently, they're only used for
modes, really, and lead to some info retrieved from overrides, some from
the real edid. This kind of hacks will bypass the override/firmware edid
mechanisms then too. :(


It seems like there is a HW issue which prevents them from reading EDID
from an offset. So, I'm not sure if it is a hack or a HW limitation.

One way around this would be to hide the HW requirement in the
get_edid_block func pointer passed to drm_do_get_edid(). This
would, however, result in more i2c reads (equal to # of extension
blocks) than what the patch currently does.

Peter, if you think doing extra EDID reads isn't too costly on your
platform, you could consider using drm_do_get_edid(). If not, I guess
you'll miss out on the additional functionality Jani is going to add
in the future.

Thanks,
Archit




BR,
Jani.




[...]

I fixed all your other suggestions. Thank you!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel




--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Jani Nikula]

On Sat, 28 Jan 2017, Peter Senna Tschudin  wrote:
> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> Hi Archit,
>
> Thank you for the comments!
>
> [...]
>> > +  total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> > +  if (total_size > EDID_LENGTH) {
>> > +  kfree(block);
>> > +  block = kmalloc(total_size, GFP_KERNEL);
>> > +  if (!block)
>> > +  return NULL;
>> > +
>> > +  /* Yes, read the entire buffer, and do not skip the first
>> > +   * EDID_LENGTH bytes.
>> > +   */
>> 
>> Is this the reason why you aren't using drm_do_get_edid()?
>
> Yes, for some hw specific reason, it is necessary to read the entire
> EDID buffer starting from 0, not block by block.

Hrmh, I'm planning on moving the edid override and firmware edid
mechanisms at the drm_do_get_edid() level to be able to truly and
transparently use a different edid. Currently, they're only used for
modes, really, and lead to some info retrieved from overrides, some from
the real edid. This kind of hacks will bypass the override/firmware edid
mechanisms then too. :(

BR,
Jani.


>
> [...]
>
> I fixed all your other suggestions. Thank you!
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
Hi Archit,

Thank you for the comments!

[...]
> > +   total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > +   if (total_size > EDID_LENGTH) {
> > +   kfree(block);
> > +   block = kmalloc(total_size, GFP_KERNEL);
> > +   if (!block)
> > +   return NULL;
> > +
> > +   /* Yes, read the entire buffer, and do not skip the first
> > +* EDID_LENGTH bytes.
> > +*/
> 
> Is this the reason why you aren't using drm_do_get_edid()?

Yes, for some hw specific reason, it is necessary to read the entire
EDID buffer starting from 0, not block by block.

[...]

I fixed all your other suggestions. Thank you!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

On 01 January, 2017 21:24 CET, Peter Senna Tschudin  wrote: 

[ ... ] 

> +static void ge_b850v3_lvds_dp_detach(struct drm_bridge *bridge)
> +{
> + struct ge_b850v3_lvds_dp *ptn_bridge
> + = bridge_to_ge_b850v3_lvds_dp(bridge);
> + struct i2c_client *ge_b850v3_lvds_dp_i2c
> + = ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +
> + /* Disable interrupts */
> + i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> + STDP4028_DPTX_IRQ_EN_REG, ~STDP4028_DPTX_IRQ_CONFIG);

 ~STDP4028_DPTX_IRQ_CONFIG? Argh! Will fix this and resend after reviews...

[ ... ]