soliciting feedback/direction on DDC support

soliciting feedback/direction on DDC support

[Dennis Munsie]

hi everyone,

now that I have the I2C bus support in the intelfb driver, I would  
like to use it for the reading the EDID block.  And actually, I've  
tested it and it works just fine.

but... the way it does it is basically the same as the aty driver.   
Which leads to an ugly mess of copy and pasting code from one driver  
to another.  Not ideal at all.

So, I was thinking of making it generic and sticking it in fbmon.c  
(or someplace similar).  But, to do it properly, I need to fiddle  
with fields in the i2c_algo_bit_data struct.  Ideally, I would want  
to make sure that the adapter that's passed in is using the i2c-algo- 
bit algorithm.  But, to do this would require exposing functionality  
from the i2c algorithm, which seems dirty.

Normally I wouldn't need to do this, but it looks like the aty code  
was accommodating older monitors that might not have implemented  
everything to spec.  And therefore, it looks like they took to do a  
little bit banging themselves.

I've managed to make the aty functionality generic to a point -- it  
doesn't fiddle the registers directly, but uses the routines defined  
for that purpose -- but not as generic as I would ideally like.

At this point, I'll probably still separate out the functionality,  
but I won't feel comfortable submitting it upstream until the issues  
of being safe and completely generic are resolved.

I really don't want this functionality to drop on the floor or be  
implemented directly (like the other drivers) in the intelfb driver,  
so any feedback would be greatly appreciated.  It might just be that  
I'm completely missing something here.

dennis

Re: soliciting feedback/direction on DDC support

[Antonino A. Daplas]

Dennis Munsie wrote:
> hi everyone,
> 
> now that I have the I2C bus support in the intelfb driver, I would  
> like to use it for the reading the EDID block.  And actually, I've  
> tested it and it works just fine.
> 
> but... the way it does it is basically the same as the aty driver.   
> Which leads to an ugly mess of copy and pasting code from one driver  
> to another.  Not ideal at all.
> 
> So, I was thinking of making it generic and sticking it in fbmon.c  
> (or someplace similar).  But, to do it properly, I need to fiddle  
> with fields in the i2c_algo_bit_data struct.  Ideally, I would want  
> to make sure that the adapter that's passed in is using the i2c-algo- 
> bit algorithm.  But, to do this would require exposing functionality  
> from the i2c algorithm, which seems dirty.
> 
> Normally I wouldn't need to do this, but it looks like the aty code  
> was accommodating older monitors that might not have implemented  
> everything to spec.  And therefore, it looks like they took to do a  
> little bit banging themselves.
> 
> I've managed to make the aty functionality generic to a point -- it  
> doesn't fiddle the registers directly, but uses the routines defined  
> for that purpose -- but not as generic as I would ideally like.
> 
> At this point, I'll probably still separate out the functionality,  
> but I won't feel comfortable submitting it upstream until the issues  
> of being safe and completely generic are resolved.
> 
> I really don't want this functionality to drop on the floor or be  
> implemented directly (like the other drivers) in the intelfb driver,  
> so any feedback would be greatly appreciated.  It might just be that  
> I'm completely missing something here.

I don't see a problem making this generic.  For drivers such as radeonfb,
we can add addtional hooks such as

fb_ddc_pre_read() - hack before probing
fb_ddc_post_read() - hack after probing
fb_ddc_stop_read() - hack after everything is done

to accommodate the hacks.

Tony

Re: soliciting feedback/direction on DDC support

[Dennis Munsie]

well, the hacks don't seem specific to the driver -- more that they  
are there to deal with quirks in the monitors -- which is something  
that shouldn't be in the driver.  It could also be that these hacks  
are completely unnecessary -- but that is something that I'm not  
qualified to determine.

But, the problem is that to make it robust and generic, i need access  
to some internal data in the i2c bit algo.  I can call the  
appropriate functions that tie back into the drivers, but I can't be  
100% sure that the I2C adapter passed to me is a bit algo and not  
something else.  There is no ID field in the adapter struct (or any  
of the structs inside of that one) that allows me to verify that I  
have good data.

Now, playing devil's advocate, we could just say that since this  
routine should only be called by drivers in the fb subsystem, they  
should know what i2c algorithm they are using, and it can be up to  
the caller to insure that everything is OK... but, I really don't  
feel comfortable using that method.  I'll leave that one up for  
debate...

I'm going to go ahead and write up the less than ideal version and  
post it up here for comments.  At least it can be a starting point  
that can be improved upon :)

dennis

On Jun 14, 2006, at 5:44 PM, Antonino A. Daplas wrote:
> I don't see a problem making this generic.  For drivers such as  
> radeonfb,
> we can add addtional hooks such as
>
> fb_ddc_pre_read() - hack before probing
> fb_ddc_post_read() - hack after probing
> fb_ddc_stop_read() - hack after everything is done
>
> to accommodate the hacks.

Re: soliciting feedback/direction on DDC support

[Luca]

Il Wed, Jun 14, 2006 at 08:30:27PM -0400, Dennis Munsie ha scritto: 
> well, the hacks don't seem specific to the driver -- more that they  
> are there to deal with quirks in the monitors -- which is something  
> that shouldn't be in the driver.  It could also be that these hacks  
> are completely unnecessary -- but that is something that I'm not  
> qualified to determine.

Those hacks are needed to wake up the EEPROMs on some weird monitors.
When I did the initial coding of the I2C stuff I asked ATI and they
confirmed that that black magic was not stricly needed but a "nice to
have" to make sure that EDID could be read also from older monitors.

> But, the problem is that to make it robust and generic, i need access  
> to some internal data in the i2c bit algo.  I can call the  
> appropriate functions that tie back into the drivers, but I can't be  
> 100% sure that the I2C adapter passed to me is a bit algo and not  
> something else. There is no ID field in the adapter struct (or any  
> of the structs inside of that one) that allows me to verify that I  
> have good data.
> 
> Now, playing devil's advocate, we could just say that since this  
> routine should only be called by drivers in the fb subsystem, they  
> should know what i2c algorithm they are using, and it can be up to  
> the caller to insure that everything is OK... but, I really don't  
> feel comfortable using that method.  I'll leave that one up for  
> debate...

Well, this stuff will be used only from kernel drivers. So I'd say that
if a driver is passing down something which is not a bit banging adaptor
then that driver is buggy, it violating the API.
I mean, right now I can pass crap to tons of kernel functions and then
watch the kernel explode; it's not a library which will be used by
userspace applications :P

Luca
-- 
Home: http://kronoz.cjb.net
"Ci sono le balle, le megaballe e le statistiche".
 Mark Twain