| Message ID | 20191202193230.21310-8-sam@ravnborg.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/panel infrastructure + backlight update | expand |
Hi Sam, Thank you for the patch. On Mon, Dec 02, 2019 at 08:32:11PM +0100, Sam Ravnborg wrote: > There was no users - so remove it. > The callback was implemented in two drivers - deleted. This looks good to me, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> but I'll let Thierry decide if he prefers keeping it. > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ > drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ > include/drm/drm_panel.h | 9 --------- > 3 files changed, 45 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > index b878930b17e4..3bcba64235c4 100644 > --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, > return seiko_panel_get_fixed_modes(p, connector); > } > > -static int seiko_panel_get_timings(struct drm_panel *panel, > - unsigned int num_timings, > - struct display_timing *timings) > -{ > - struct seiko_panel *p = to_seiko_panel(panel); > - unsigned int i; > - > - if (p->desc->num_timings < num_timings) > - num_timings = p->desc->num_timings; > - > - if (timings) > - for (i = 0; i < num_timings; i++) > - timings[i] = p->desc->timings[i]; > - > - return p->desc->num_timings; > -} > - > static const struct drm_panel_funcs seiko_panel_funcs = { > .disable = seiko_panel_disable, > .unprepare = seiko_panel_unprepare, > .prepare = seiko_panel_prepare, > .enable = seiko_panel_enable, > .get_modes = seiko_panel_get_modes, > - .get_timings = seiko_panel_get_timings, > }; > > static int seiko_panel_probe(struct device *dev, > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index d6299fe6d276..e225791a6fb2 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -326,30 +326,12 @@ static int panel_simple_get_modes(struct drm_panel *panel, > return num; > } > > -static int panel_simple_get_timings(struct drm_panel *panel, > - unsigned int num_timings, > - struct display_timing *timings) > -{ > - struct panel_simple *p = to_panel_simple(panel); > - unsigned int i; > - > - if (p->desc->num_timings < num_timings) > - num_timings = p->desc->num_timings; > - > - if (timings) > - for (i = 0; i < num_timings; i++) > - timings[i] = p->desc->timings[i]; > - > - return p->desc->num_timings; > -} > - > static const struct drm_panel_funcs panel_simple_funcs = { > .disable = panel_simple_disable, > .unprepare = panel_simple_unprepare, > .prepare = panel_simple_prepare, > .enable = panel_simple_enable, > .get_modes = panel_simple_get_modes, > - .get_timings = panel_simple_get_timings, > }; > > #define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 4fd61cb9eb93..c4e82b9ce586 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -102,15 +102,6 @@ struct drm_panel_funcs { > */ > int (*get_modes)(struct drm_panel *panel, > struct drm_connector *connector); > - > - /** > - * @get_timings: > - * > - * Copy display timings into the provided array and return > - * the number of display timings available. > - */ > - int (*get_timings)(struct drm_panel *panel, unsigned int num_timings, > - struct display_timing *timings); > }; > > /**
Hi, On Mon, Dec 02, 2019 at 08:32:11PM +0100, Sam Ravnborg wrote: > There was no users - so remove it. > The callback was implemented in two drivers - deleted. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ > drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ > include/drm/drm_panel.h | 9 --------- > 3 files changed, 45 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > index b878930b17e4..3bcba64235c4 100644 > --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, > return seiko_panel_get_fixed_modes(p, connector); > } > > -static int seiko_panel_get_timings(struct drm_panel *panel, > - unsigned int num_timings, > - struct display_timing *timings) > -{ > - struct seiko_panel *p = to_seiko_panel(panel); > - unsigned int i; > - > - if (p->desc->num_timings < num_timings) > - num_timings = p->desc->num_timings; > - > - if (timings) > - for (i = 0; i < num_timings; i++) > - timings[i] = p->desc->timings[i]; > - > - return p->desc->num_timings; > -} > - > static const struct drm_panel_funcs seiko_panel_funcs = { > .disable = seiko_panel_disable, > .unprepare = seiko_panel_unprepare, > .prepare = seiko_panel_prepare, > .enable = seiko_panel_enable, > .get_modes = seiko_panel_get_modes, > - .get_timings = seiko_panel_get_timings, > }; If anything, I think we should grow the usage of timings and / or make it usable by everyone. Using only the mode as we do currently has a bunch of shortcomings as almost no encoder will be able to provide the typical pixel clock, and that situation leads to multiple things: - If someone working on one encoder wants to upstream a panel they have tested, chances are this will not be the typical pixel clock / timings being used but rather the one that will match what that SoC is capable of. Trouble comes when a second user comes in with a different encoder and different capabilities, and then we have a maintainance fight over which timing is the true timing (with a significant chance that none of them are). - If we can't match the pixel clock, we currently have no easy way to make the usual measures of reducing / growing the porches and blankings areas to match the pixel clock we can provide, since we don't have an easy way to get the tolerance on those timings for a given panel. There's some ad hoc solutions on some drivers (I think vc4 has that?) to ignore the panel and just play around with the timings, but I think this should be generalised. Timings solves the first case since we have the operating range now and not a single set of timings, and it solves the second since we can use that range to take those measures instead of taking a shot in the dark. I appreciate that it's pretty far from where we are today, but removing the get_timings means that all the timings already defined in the panel drivers are becoming useless too, and that eventually it will get removed. Maxime
Hi Maxime, On Tue, Dec 03, 2019 at 08:46:59AM +0100, Maxime Ripard wrote: > On Mon, Dec 02, 2019 at 08:32:11PM +0100, Sam Ravnborg wrote: > > There was no users - so remove it. > > The callback was implemented in two drivers - deleted. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ > > drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ > > include/drm/drm_panel.h | 9 --------- > > 3 files changed, 45 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > index b878930b17e4..3bcba64235c4 100644 > > --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, > > return seiko_panel_get_fixed_modes(p, connector); > > } > > > > -static int seiko_panel_get_timings(struct drm_panel *panel, > > - unsigned int num_timings, > > - struct display_timing *timings) > > -{ > > - struct seiko_panel *p = to_seiko_panel(panel); > > - unsigned int i; > > - > > - if (p->desc->num_timings < num_timings) > > - num_timings = p->desc->num_timings; > > - > > - if (timings) > > - for (i = 0; i < num_timings; i++) > > - timings[i] = p->desc->timings[i]; > > - > > - return p->desc->num_timings; > > -} > > - > > static const struct drm_panel_funcs seiko_panel_funcs = { > > .disable = seiko_panel_disable, > > .unprepare = seiko_panel_unprepare, > > .prepare = seiko_panel_prepare, > > .enable = seiko_panel_enable, > > .get_modes = seiko_panel_get_modes, > > - .get_timings = seiko_panel_get_timings, > > }; > > If anything, I think we should grow the usage of timings and / or make > it usable by everyone. > > Using only the mode as we do currently has a bunch of shortcomings as > almost no encoder will be able to provide the typical pixel clock, and > that situation leads to multiple things: > > - If someone working on one encoder wants to upstream a panel they > have tested, chances are this will not be the typical pixel clock > / timings being used but rather the one that will match what that > SoC is capable of. Trouble comes when a second user comes in with > a different encoder and different capabilities, and then we have a > maintainance fight over which timing is the true timing (with a > significant chance that none of them are). > > - If we can't match the pixel clock, we currently have no easy way > to make the usual measures of reducing / growing the porches and > blankings areas to match the pixel clock we can provide, since we > don't have an easy way to get the tolerance on those timings for a > given panel. There's some ad hoc solutions on some drivers (I > think vc4 has that?) to ignore the panel and just play around with > the timings, but I think this should be generalised. > > Timings solves the first case since we have the operating range now > and not a single set of timings, and it solves the second since we can > use that range to take those measures instead of taking a shot in the > dark. > > I appreciate that it's pretty far from where we are today, but > removing the get_timings means that all the timings already defined in > the panel drivers are becoming useless too, and that eventually it > will get removed. I agree with you.
Hi Maxime. On Tue, Dec 03, 2019 at 08:46:59AM +0100, Maxime Ripard wrote: > Hi, > > On Mon, Dec 02, 2019 at 08:32:11PM +0100, Sam Ravnborg wrote: > > There was no users - so remove it. > > The callback was implemented in two drivers - deleted. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ > > drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ > > include/drm/drm_panel.h | 9 --------- > > 3 files changed, 45 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > index b878930b17e4..3bcba64235c4 100644 > > --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, > > return seiko_panel_get_fixed_modes(p, connector); > > } > > > > -static int seiko_panel_get_timings(struct drm_panel *panel, > > - unsigned int num_timings, > > - struct display_timing *timings) > > -{ > > - struct seiko_panel *p = to_seiko_panel(panel); > > - unsigned int i; > > - > > - if (p->desc->num_timings < num_timings) > > - num_timings = p->desc->num_timings; > > - > > - if (timings) > > - for (i = 0; i < num_timings; i++) > > - timings[i] = p->desc->timings[i]; > > - > > - return p->desc->num_timings; > > -} > > - > > static const struct drm_panel_funcs seiko_panel_funcs = { > > .disable = seiko_panel_disable, > > .unprepare = seiko_panel_unprepare, > > .prepare = seiko_panel_prepare, > > .enable = seiko_panel_enable, > > .get_modes = seiko_panel_get_modes, > > - .get_timings = seiko_panel_get_timings, > > }; > > If anything, I think we should grow the usage of timings and / or make > it usable by everyone. > > Using only the mode as we do currently has a bunch of shortcomings as > almost no encoder will be able to provide the typical pixel clock, and > that situation leads to multiple things: > > - If someone working on one encoder wants to upstream a panel they > have tested, chances are this will not be the typical pixel clock > / timings being used but rather the one that will match what that > SoC is capable of. Trouble comes when a second user comes in with > a different encoder and different capabilities, and then we have a > maintainance fight over which timing is the true timing (with a > significant chance that none of them are). > > - If we can't match the pixel clock, we currently have no easy way > to make the usual measures of reducing / growing the porches and > blankings areas to match the pixel clock we can provide, since we > don't have an easy way to get the tolerance on those timings for a > given panel. There's some ad hoc solutions on some drivers (I > think vc4 has that?) to ignore the panel and just play around with > the timings, but I think this should be generalised. > > Timings solves the first case since we have the operating range now > and not a single set of timings, and it solves the second since we can > use that range to take those measures instead of taking a shot in the > dark. > > I appreciate that it's pretty far from where we are today, but > removing the get_timings means that all the timings already defined in > the panel drivers are becoming useless too, and that eventually it > will get removed. Thanks for this nice explanation. I will drop the patch, and add an entry to my TODO list to look closer at this later. There are things to improve in this area. So the conclusion is more work rather than removing code :-) Sam
Hi Maxime, On Tue, Dec 3, 2019 at 8:47 AM Maxime Ripard <mripard@kernel.org> wrote: > Using only the mode as we do currently has a bunch of shortcomings as > almost no encoder will be able to provide the typical pixel clock, and > that situation leads to multiple things: > > - If someone working on one encoder wants to upstream a panel they > have tested, chances are this will not be the typical pixel clock > / timings being used but rather the one that will match what that > SoC is capable of. Trouble comes when a second user comes in with > a different encoder and different capabilities, and then we have a > maintainance fight over which timing is the true timing (with a > significant chance that none of them are). > > - If we can't match the pixel clock, we currently have no easy way > to make the usual measures of reducing / growing the porches and > blankings areas to match the pixel clock we can provide, since we > don't have an easy way to get the tolerance on those timings for a > given panel. There's some ad hoc solutions on some drivers (I > think vc4 has that?) to ignore the panel and just play around with > the timings, but I think this should be generalised. I've been confused with these things as they look today and it seems the whole struct drm_display_mode could need some improvement? If .clock is supposed to be htotal * vtotal * vrefresh, what is the .clock doing there anyway. Sadly I am too inexperienced to realize where the tolerances should be stated, but I guess just stating that hsync_start etc are typical, then specify some tolerance for each would help a bit? On the DSI displays in video mode there is also this EOL area which seems to be where the logic is normally just idling for a while, that can be adjusted on some hardware as well, but I don't quite understand it admittedly. Sometimes I wonder if anyone really understands DSI... :/ Yours, Linus Walleij
On Tue, Dec 03, 2019 at 09:39:36AM +0100, Sam Ravnborg wrote: > On Tue, Dec 03, 2019 at 08:46:59AM +0100, Maxime Ripard wrote: > > Hi, > > > > On Mon, Dec 02, 2019 at 08:32:11PM +0100, Sam Ravnborg wrote: > > > There was no users - so remove it. > > > The callback was implemented in two drivers - deleted. > > > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > --- > > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ > > > drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ > > > include/drm/drm_panel.h | 9 --------- > > > 3 files changed, 45 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > > index b878930b17e4..3bcba64235c4 100644 > > > --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > > +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c > > > @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, > > > return seiko_panel_get_fixed_modes(p, connector); > > > } > > > > > > -static int seiko_panel_get_timings(struct drm_panel *panel, > > > - unsigned int num_timings, > > > - struct display_timing *timings) > > > -{ > > > - struct seiko_panel *p = to_seiko_panel(panel); > > > - unsigned int i; > > > - > > > - if (p->desc->num_timings < num_timings) > > > - num_timings = p->desc->num_timings; > > > - > > > - if (timings) > > > - for (i = 0; i < num_timings; i++) > > > - timings[i] = p->desc->timings[i]; > > > - > > > - return p->desc->num_timings; > > > -} > > > - > > > static const struct drm_panel_funcs seiko_panel_funcs = { > > > .disable = seiko_panel_disable, > > > .unprepare = seiko_panel_unprepare, > > > .prepare = seiko_panel_prepare, > > > .enable = seiko_panel_enable, > > > .get_modes = seiko_panel_get_modes, > > > - .get_timings = seiko_panel_get_timings, > > > }; > > > > If anything, I think we should grow the usage of timings and / or make > > it usable by everyone. > > > > Using only the mode as we do currently has a bunch of shortcomings as > > almost no encoder will be able to provide the typical pixel clock, and > > that situation leads to multiple things: > > > > - If someone working on one encoder wants to upstream a panel they > > have tested, chances are this will not be the typical pixel clock > > / timings being used but rather the one that will match what that > > SoC is capable of. Trouble comes when a second user comes in with > > a different encoder and different capabilities, and then we have a > > maintainance fight over which timing is the true timing (with a > > significant chance that none of them are). > > > > - If we can't match the pixel clock, we currently have no easy way > > to make the usual measures of reducing / growing the porches and > > blankings areas to match the pixel clock we can provide, since we > > don't have an easy way to get the tolerance on those timings for a > > given panel. There's some ad hoc solutions on some drivers (I > > think vc4 has that?) to ignore the panel and just play around with > > the timings, but I think this should be generalised. > > > > Timings solves the first case since we have the operating range now > > and not a single set of timings, and it solves the second since we can > > use that range to take those measures instead of taking a shot in the > > dark. > > > > I appreciate that it's pretty far from where we are today, but > > removing the get_timings means that all the timings already defined in > > the panel drivers are becoming useless too, and that eventually it > > will get removed. > > Thanks for this nice explanation. I will drop the patch, > and add an entry to my TODO list to look closer at this later. > There are things to improve in this area. > > So the conclusion is more work rather than removing code :-) Yeah, unfortunately.. :) Maxime
Hi Linus, On Tue, Dec 03, 2019 at 04:20:24PM +0100, Linus Walleij wrote: > On Tue, Dec 3, 2019 at 8:47 AM Maxime Ripard <mripard@kernel.org> wrote: > > > Using only the mode as we do currently has a bunch of shortcomings as > > almost no encoder will be able to provide the typical pixel clock, and > > that situation leads to multiple things: > > > > - If someone working on one encoder wants to upstream a panel they > > have tested, chances are this will not be the typical pixel clock > > / timings being used but rather the one that will match what that > > SoC is capable of. Trouble comes when a second user comes in with > > a different encoder and different capabilities, and then we have a > > maintainance fight over which timing is the true timing (with a > > significant chance that none of them are). > > > > - If we can't match the pixel clock, we currently have no easy way > > to make the usual measures of reducing / growing the porches and > > blankings areas to match the pixel clock we can provide, since we > > don't have an easy way to get the tolerance on those timings for a > > given panel. There's some ad hoc solutions on some drivers (I > > think vc4 has that?) to ignore the panel and just play around with > > the timings, but I think this should be generalised. > > I've been confused with these things as they look today and it seems > the whole struct drm_display_mode could need some improvement? > > If .clock is supposed to be htotal * vtotal * vrefresh, what is the > .clock doing there anyway. It's one thing I wonder as well. I guess it's just more convenient for everyone, since it's exposed by the VESA modes (iirc) and a lot of drivers really care about the clock. > Sadly I am too inexperienced to realize where the tolerances should > be stated, but I guess just stating that hsync_start etc are typical, > then specify some tolerance for each would help a bit? The timings structure discussed in the patch that started this discussion is actually doing this nicely, you have for each timing the minimum, typical and maximum value. The current issue with it though is that it's pretty difficult to use it, since it's not really tied to any of the panel code (or DRM helpers). The only driver that was supporting it was omapdrm and it was removed recently. If we really wanted to support it, one path forward I can see would be to make the timings structure the primary one, and only use drm_display_mode for userspace facing code, and generate it from the timings. This would be a pretty invasive change though... > On the DSI displays in video mode there is also this EOL area > which seems to be where the logic is normally just idling for a > while, that can be adjusted on some hardware as well, but > I don't quite understand it admittedly. Sometimes I wonder if > anyone really understands DSI... :/ I'm not aware of any EOL area in MIPI-DSI that would make the hardware idle, don't you mean LP-11? Maxime
Hi Maxime, On Wed, Dec 04, 2019 at 09:16:50AM +0100, Maxime Ripard wrote: > On Tue, Dec 03, 2019 at 04:20:24PM +0100, Linus Walleij wrote: > > On Tue, Dec 3, 2019 at 8:47 AM Maxime Ripard wrote: > > > > > Using only the mode as we do currently has a bunch of shortcomings as > > > almost no encoder will be able to provide the typical pixel clock, and > > > that situation leads to multiple things: > > > > > > - If someone working on one encoder wants to upstream a panel they > > > have tested, chances are this will not be the typical pixel clock > > > / timings being used but rather the one that will match what that > > > SoC is capable of. Trouble comes when a second user comes in with > > > a different encoder and different capabilities, and then we have a > > > maintainance fight over which timing is the true timing (with a > > > significant chance that none of them are). > > > > > > - If we can't match the pixel clock, we currently have no easy way > > > to make the usual measures of reducing / growing the porches and > > > blankings areas to match the pixel clock we can provide, since we > > > don't have an easy way to get the tolerance on those timings for a > > > given panel. There's some ad hoc solutions on some drivers (I > > > think vc4 has that?) to ignore the panel and just play around with > > > the timings, but I think this should be generalised. > > > > I've been confused with these things as they look today and it seems > > the whole struct drm_display_mode could need some improvement? > > > > If .clock is supposed to be htotal * vtotal * vrefresh, what is the > > .clock doing there anyway. > > It's one thing I wonder as well. I guess it's just more convenient for > everyone, since it's exposed by the VESA modes (iirc) and a lot of > drivers really care about the clock. My understanding is that the clock is the authoritative parameter, while vrefresh is offered as a convenience to avoid calculating it manually through drivers. > > Sadly I am too inexperienced to realize where the tolerances should > > be stated, but I guess just stating that hsync_start etc are typical, > > then specify some tolerance for each would help a bit? > > The timings structure discussed in the patch that started this > discussion is actually doing this nicely, you have for each timing the > minimum, typical and maximum value. The current issue with it though > is that it's pretty difficult to use it, since it's not really tied to > any of the panel code (or DRM helpers). The only driver that was > supporting it was omapdrm and it was removed recently. > > If we really wanted to support it, one path forward I can see would be > to make the timings structure the primary one, and only use > drm_display_mode for userspace facing code, and generate it from the > timings. This would be a pretty invasive change though... > > > On the DSI displays in video mode there is also this EOL area > > which seems to be where the logic is normally just idling for a > > while, that can be adjusted on some hardware as well, but > > I don't quite understand it admittedly. Sometimes I wonder if > > anyone really understands DSI... :/ > > I'm not aware of any EOL area in MIPI-DSI that would make the hardware > idle, don't you mean LP-11?
On Wed, Dec 4, 2019 at 9:16 AM Maxime Ripard <mripard@kernel.org> wrote: > On Tue, Dec 03, 2019 at 04:20:24PM +0100, Linus Walleij wrote: > > On the DSI displays in video mode there is also this EOL area > > which seems to be where the logic is normally just idling for a > > while, that can be adjusted on some hardware as well, but > > I don't quite understand it admittedly. Sometimes I wonder if > > anyone really understands DSI... :/ > > I'm not aware of any EOL area in MIPI-DSI that would make the hardware > idle, don't you mean LP-11? I think in the spec the bubble used for this is tagged "BLLP" Blanking-Line-Low-Power or something. IIUC it is possible for displays to either receive continuous NULL packets or blanking packets or go to LP mode in this area. And since that is not there for e.g. DPI displays I feel it adds another layer of confusion to timings. Yours, Linus Walleij
diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c index b878930b17e4..3bcba64235c4 100644 --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c @@ -217,30 +217,12 @@ static int seiko_panel_get_modes(struct drm_panel *panel, return seiko_panel_get_fixed_modes(p, connector); } -static int seiko_panel_get_timings(struct drm_panel *panel, - unsigned int num_timings, - struct display_timing *timings) -{ - struct seiko_panel *p = to_seiko_panel(panel); - unsigned int i; - - if (p->desc->num_timings < num_timings) - num_timings = p->desc->num_timings; - - if (timings) - for (i = 0; i < num_timings; i++) - timings[i] = p->desc->timings[i]; - - return p->desc->num_timings; -} - static const struct drm_panel_funcs seiko_panel_funcs = { .disable = seiko_panel_disable, .unprepare = seiko_panel_unprepare, .prepare = seiko_panel_prepare, .enable = seiko_panel_enable, .get_modes = seiko_panel_get_modes, - .get_timings = seiko_panel_get_timings, }; static int seiko_panel_probe(struct device *dev, diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index d6299fe6d276..e225791a6fb2 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -326,30 +326,12 @@ static int panel_simple_get_modes(struct drm_panel *panel, return num; } -static int panel_simple_get_timings(struct drm_panel *panel, - unsigned int num_timings, - struct display_timing *timings) -{ - struct panel_simple *p = to_panel_simple(panel); - unsigned int i; - - if (p->desc->num_timings < num_timings) - num_timings = p->desc->num_timings; - - if (timings) - for (i = 0; i < num_timings; i++) - timings[i] = p->desc->timings[i]; - - return p->desc->num_timings; -} - static const struct drm_panel_funcs panel_simple_funcs = { .disable = panel_simple_disable, .unprepare = panel_simple_unprepare, .prepare = panel_simple_prepare, .enable = panel_simple_enable, .get_modes = panel_simple_get_modes, - .get_timings = panel_simple_get_timings, }; #define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 4fd61cb9eb93..c4e82b9ce586 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -102,15 +102,6 @@ struct drm_panel_funcs { */ int (*get_modes)(struct drm_panel *panel, struct drm_connector *connector); - - /** - * @get_timings: - * - * Copy display timings into the provided array and return - * the number of display timings available. - */ - int (*get_timings)(struct drm_panel *panel, unsigned int num_timings, - struct display_timing *timings); }; /**
There was no users - so remove it. The callback was implemented in two drivers - deleted. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ------------------ drivers/gpu/drm/panel/panel-simple.c | 18 ------------------ include/drm/drm_panel.h | 9 --------- 3 files changed, 45 deletions(-)