Re: [PATCH] i2c: mux: Remove class argument from i2c_mux_add_adapter()
Hi!
2024-04-18 at 22:55, Heiner Kallweit wrote:
> 99a741aa7a2d ("i2c: mux: gpio: remove support for class-based device
> instantiation") removed the last call to i2c_mux_add_adapter() with a
> non-null class argument. Therefore the class argument can be removed.
>
> Note: Class-based device instantiation is a legacy mechanism which
> shouldn't be used in new code, so we can rule out that this argument
> may be needed again in the future.
>
> Signed-off-by: Heiner Kallweit
Acked-by: Peter Rosin
Cheers,
Peter
Re: [PATCH] dt-bindings: Fix properties without any type
e. >0 selects active low, 1 selects active high. >If omitted then it is not used by the hardware > +$ref: /schemas/types.yaml#/definitions/uint32 > enum: [0, 1] > >pixelclk-active: > @@ -169,6 +172,7 @@ properties: >sample data on rising edge. >Use 1 to drive pixel data on rising edge and >sample data on falling edge > +$ref: /schemas/types.yaml#/definitions/uint32 > enum: [0, 1] > >syncclk-active: > @@ -179,6 +183,7 @@ properties: >sample sync on rising edge of pixel clock. >Use 1 to drive sync on rising edge and >sample sync on falling edge of pixel clock > +$ref: /schemas/types.yaml#/definitions/uint32 > enum: [0, 1] > >interlaced: > diff --git > a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.yaml > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.yaml > index 745dd247c409..617aa8c8c03a 100644 > --- a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.yaml > +++ b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.yaml > @@ -24,6 +24,7 @@ properties: > >dsi-lanes: > description: Number of DSI lanes to be used must be <3> or <4> > +$ref: /schemas/types.yaml#/definitions/uint32 > enum: [3, 4] > >v3p3-supply: > diff --git > a/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml > b/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml > index ca959451557e..1cdc91b3439f 100644 > --- a/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml > +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml > @@ -36,6 +36,7 @@ properties: > >init-delay: > description: delay after initialization sequence [ms] > +$ref: /schemas/types.yaml#/definitions/uint32 > >panel-width-mm: > description: physical panel width [mm] > diff --git a/Documentation/devicetree/bindings/gpio/fairchild,74hc595.yaml > b/Documentation/devicetree/bindings/gpio/fairchild,74hc595.yaml > index 5fe19fa5f67c..a99e7842ca17 100644 > --- a/Documentation/devicetree/bindings/gpio/fairchild,74hc595.yaml > +++ b/Documentation/devicetree/bindings/gpio/fairchild,74hc595.yaml > @@ -26,6 +26,7 @@ properties: > const: 2 > >registers-number: > +$ref: /schemas/types.yaml#/definitions/uint32 > description: Number of daisy-chained shift registers > >enable-gpios: > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > index e8f137abb03c..aa61fe64be63 100644 > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > @@ -31,6 +31,7 @@ properties: > type: boolean > >function-row-physmap: > +$ref: /schemas/types.yaml#/definitions/uint32-array > minItems: 1 > maxItems: 15 > description: | > diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.yaml > b/Documentation/devicetree/bindings/input/matrix-keymap.yaml > index 6699d5e32dca..9f703bb51e12 100644 > --- a/Documentation/devicetree/bindings/input/matrix-keymap.yaml > +++ b/Documentation/devicetree/bindings/input/matrix-keymap.yaml > @@ -27,6 +27,10 @@ properties: >column and linux key-code. The 32-bit big endian cell is packed as: >row << 24 | column << 16 | key-code > > + linux,no-autorepeat: > +type: boolean > +description: Disable keyrepeat > + >keypad,num-rows: > $ref: /schemas/types.yaml#/definitions/uint32 > description: Number of row lines connected to the keypad controller. > diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.yaml > b/Documentation/devicetree/bindings/media/i2c/adv7604.yaml > index c19d8391e2d5..7589d377c686 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv7604.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.yaml > @@ -60,7 +60,8 @@ properties: >enables hot-plug detection. > >default-input: > -maxItems: 1 > +$ref: /schemas/types.yaml#/definitions/uint32 > +enum: [ 0, 1 ] > description: >Select which input is selected after reset. > > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml > b/Documentation/devicetree/bindings/mux/reg-mux.yaml > index 60d5746eb39d..e2f6b11f1254 100644 > --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml > +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml > @@ -25,8 +25,12 @@ properties: > const: 1 > &
Re: [PATCH] drm/fb-helper: Add missed unlocks in setcmap_legacy()
Hi!
On 2020-12-03 15:42, Chuhong Yuan wrote:
> setcmap_legacy() does not call drm_modeset_unlock_all() in some exits,
> add the missed unlocks with goto to fix it.
>
> Fixes: 964c60063bff ("drm/fb-helper: separate the fb_setcmap helper into
> atomic and legacy paths")
> Signed-off-by: Chuhong Yuan
Yup, my patch fumbled the locking. Sorry, and thanks for cleaning up my mess!
Acked-by: Peter Rosin
(Spelled that as Ached-by at first, what does that mean??)
Cheers,
Peter
> ---
> drivers/gpu/drm/drm_fb_helper.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1543d9d10970..8033467db4be 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -923,11 +923,15 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct
> fb_info *info)
> drm_modeset_lock_all(fb_helper->dev);
> drm_client_for_each_modeset(modeset, _helper->client) {
> crtc = modeset->crtc;
> - if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> - return -EINVAL;
> + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (cmap->start + cmap->len > crtc->gamma_size)
> - return -EINVAL;
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> r = crtc->gamma_store;
> g = r + crtc->gamma_size;
> @@ -940,8 +944,9 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct
> fb_info *info)
> ret = crtc->funcs->gamma_set(crtc, r, g, b,
>crtc->gamma_size, NULL);
> if (ret)
> - return ret;
> + goto out;
> }
> +out:
> drm_modeset_unlock_all(fb_helper->dev);
>
> return ret;
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Revert "drm/panel: simple: Add support for Sharp LQ150X1LG11 panels"
This reverts commit 0f9cdd743f7f8d470fff51b11250f02fc554cf1b.
The interface of the panel is LVDS, not parallel.
The color depth is RGB888, not RGB565.
The panel has additional features, making it not so simple.
The only user (upstream) of this panel is appropriately using panel-lvds.
Suggested-by: Thierry Reding
Suggested-by: Sam Ravnborg
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/panel/panel-simple.c | 27 ---
1 file changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index e14c14ac62b5..fb8d61f5ae5d 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2768,30 +2768,6 @@ static const struct panel_desc sharp_lq123p1jx31 = {
},
};
-static const struct drm_display_mode sharp_lq150x1lg11_mode = {
- .clock = 71100,
- .hdisplay = 1024,
- .hsync_start = 1024 + 168,
- .hsync_end = 1024 + 168 + 64,
- .htotal = 1024 + 168 + 64 + 88,
- .vdisplay = 768,
- .vsync_start = 768 + 37,
- .vsync_end = 768 + 37 + 2,
- .vtotal = 768 + 37 + 2 + 8,
- .vrefresh = 60,
-};
-
-static const struct panel_desc sharp_lq150x1lg11 = {
- .modes = _lq150x1lg11_mode,
- .num_modes = 1,
- .bpc = 6,
- .size = {
- .width = 304,
- .height = 228,
- },
- .bus_format = MEDIA_BUS_FMT_RGB565_1X16,
-};
-
static const struct display_timing sharp_ls020b1dd01d_timing = {
.pixelclock = { 200, 420, 500 },
.hactive = { 240, 240, 240 },
@@ -3465,9 +3441,6 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "sharp,lq123p1jx31",
.data = _lq123p1jx31,
- }, {
- .compatible = "sharp,lq150x1lg11",
- .data = _lq150x1lg11,
}, {
.compatible = "sharp,ls020b1dd01d",
.data = _ls020b1dd01d,
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 28/33] drm/panel-simple: Fix dotclock for Sharp LQ150X1LG11
On 2020-03-03 15:20, Thierry Reding wrote: > On Mon, Mar 02, 2020 at 10:53:56PM +0000, Peter Rosin wrote: >> On 2020-03-02 21:34, Ville Syrjala wrote: >>> From: Ville Syrjälä >>> >>> The currently listed dotclock disagrees with the currently >>> listed vrefresh rate. Change the dotclock to match the vrefresh. >>> >>> Someone tell me which (if either) of the dotclock or vreresh is >>> correct? >> >> TL/DR; I do not care if you change the refresh rate or the dotclock. >> >> The whole entry for that panel in simple-panel is dubious. The panel >> is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable >> with the SELLVDS pin). With Jeida you can, as usual, omit the 4th >> data channel and use the panel with RGB666. In either case, you need >> an LVDS signal and nothing else... >> >> The panel can also rotate the picture 180 degrees using the RL/UD pin. >> >> These options are of course not expressed in the simple panel driver >> (and we have always used fixed signals for those pins in our designs, >> IIRC). As far as I'm concerned, the panel can be removed from >> simple-panel. Our device trees are nowadays correctly expressing the >> hardware with an LVDS encoder between the RGB output and the panel >> and points to the panel-lvds driver for the panel. > > How do you make sure that you always bind against the correct driver? If > it matches simple-panel and panel-lvds, it's not deterministically going > to pick the right one. Well, it may actually be deterministic on Linux, > but perhaps only by accident. You are probably right that it's fragile, but no problems so far. That said, I did wonder why the panel-lvds driver "wins" over simple-panel for compatible = "sharp,lq150x1lg11", "panel-lvds"; I figured it was by design and didn't spend too much time thinking about it. Maybe I should have? >> The reason that it is as it is, is that we obviously didn't understand >> what we were doing when we added the entry, and this garbage was what >> we came up with that produced a picture. >> >> If you want to keep the panel in simple-panel despite all this, the >> timing constraints are as follows: >> >> Pixel clock 50-80 MHz,65 MHz typical >> Horizontal period 1094-1720 clocks, 1344 typical >> 16.0-23.4 us 20.7 us >> Horizontal enable1024 clocks, always >> Vertical period776-990 lines,806 typical >> 13.3-18.0 ms 16.7 ms >> Vertical enable 768 lines, always >> >> Using a "long" (the datasheet is not very specific on this issue) vertical >> period may introduce deterioration of display quality, flicker etc. >> >> I don't think the division between front-porch/back-porch matters much. >> >> That said, I have no idea whatsoever if others have started using this >> panel entry. My guess is that it has zero users, but who can tell? > > A quick grep shows that arch/arm/boot/dts/at91-nattis-2-natte-2.dts is > the only device tree that uses this panel in the upstream kernel. This is our design, and what made us originally add the entry to simple panel, but as I said, we no longer need simple-panel support for it... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 28/33] drm/panel-simple: Fix dotclock for Sharp LQ150X1LG11
On 2020-03-03 16:05, Thierry Reding wrote: > On Tue, Mar 03, 2020 at 02:57:45PM +0000, Peter Rosin wrote: >> >> On 2020-03-03 15:20, Thierry Reding wrote: >>> On Mon, Mar 02, 2020 at 10:53:56PM +, Peter Rosin wrote: >>>> On 2020-03-02 21:34, Ville Syrjala wrote: >>>>> From: Ville Syrjälä >>>>> >>>>> The currently listed dotclock disagrees with the currently >>>>> listed vrefresh rate. Change the dotclock to match the vrefresh. >>>>> >>>>> Someone tell me which (if either) of the dotclock or vreresh is >>>>> correct? >>>> >>>> TL/DR; I do not care if you change the refresh rate or the dotclock. >>>> >>>> The whole entry for that panel in simple-panel is dubious. The panel >>>> is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable >>>> with the SELLVDS pin). With Jeida you can, as usual, omit the 4th >>>> data channel and use the panel with RGB666. In either case, you need >>>> an LVDS signal and nothing else... >>>> >>>> The panel can also rotate the picture 180 degrees using the RL/UD pin. >>>> >>>> These options are of course not expressed in the simple panel driver >>>> (and we have always used fixed signals for those pins in our designs, >>>> IIRC). As far as I'm concerned, the panel can be removed from >>>> simple-panel. Our device trees are nowadays correctly expressing the >>>> hardware with an LVDS encoder between the RGB output and the panel >>>> and points to the panel-lvds driver for the panel. >>> >>> How do you make sure that you always bind against the correct driver? If >>> it matches simple-panel and panel-lvds, it's not deterministically going >>> to pick the right one. Well, it may actually be deterministic on Linux, >>> but perhaps only by accident. >> >> You are probably right that it's fragile, but no problems so far. That >> said, I did wonder why the panel-lvds driver "wins" over simple-panel for >> >> compatible = "sharp,lq150x1lg11", "panel-lvds"; >> >> I figured it was by design and didn't spend too much time thinking about >> it. Maybe I should have? > > It's most likely because panel-lvds.o is linked into the kernel before > panel-simple.o and the first match wins. You may want to move the entry > to panel-lvds to make this a little more robust. Ok, or because I dropped the simple-panel driver when we no longer depended on it. Either way, what do you mean "move to [..] panel-lvds"? It has no list of panels, you have to end with "panel-lvds" in the compatible for the driver to bind. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 28/33] drm/panel-simple: Fix dotclock for Sharp LQ150X1LG11
On 2020-03-02 21:34, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> The currently listed dotclock disagrees with the currently
> listed vrefresh rate. Change the dotclock to match the vrefresh.
>
> Someone tell me which (if either) of the dotclock or vreresh is
> correct?
TL/DR; I do not care if you change the refresh rate or the dotclock.
The whole entry for that panel in simple-panel is dubious. The panel
is really an LVDS panel (capable of both VESA/Jeida RGB888, selectable
with the SELLVDS pin). With Jeida you can, as usual, omit the 4th
data channel and use the panel with RGB666. In either case, you need
an LVDS signal and nothing else...
The panel can also rotate the picture 180 degrees using the RL/UD pin.
These options are of course not expressed in the simple panel driver
(and we have always used fixed signals for those pins in our designs,
IIRC). As far as I'm concerned, the panel can be removed from
simple-panel. Our device trees are nowadays correctly expressing the
hardware with an LVDS encoder between the RGB output and the panel
and points to the panel-lvds driver for the panel.
The reason that it is as it is, is that we obviously didn't understand
what we were doing when we added the entry, and this garbage was what
we came up with that produced a picture.
If you want to keep the panel in simple-panel despite all this, the
timing constraints are as follows:
Pixel clock 50-80 MHz,65 MHz typical
Horizontal period 1094-1720 clocks, 1344 typical
16.0-23.4 us 20.7 us
Horizontal enable1024 clocks, always
Vertical period776-990 lines,806 typical
13.3-18.0 ms 16.7 ms
Vertical enable 768 lines, always
Using a "long" (the datasheet is not very specific on this issue) vertical
period may introduce deterioration of display quality, flicker etc.
I don't think the division between front-porch/back-porch matters much.
That said, I have no idea whatsoever if others have started using this
panel entry. My guess is that it has zero users, but who can tell?
Cheers,
Peter
> Cc: Gustaf Lindström
> Cc: Peter Rosin
> Cc: Thierry Reding
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/panel/panel-simple.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 7526af2d6d95..cb587de8c49e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -3026,7 +3026,7 @@ static const struct panel_desc sharp_lq123p1jx31 = {
> };
>
> static const struct drm_display_mode sharp_lq150x1lg11_mode = {
> - .clock = 71100,
> + .clock = 65722,
> .hdisplay = 1024,
> .hsync_start = 1024 + 168,
> .hsync_end = 1024 + 168 + 64,
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/6] drm: atmel-hlcdc: prefer a lower pixel-clock than requested
On 2020-01-06 10:24, claudiu.bez...@microchip.com wrote:
> On 02.01.2020 11:08, Sam Ravnborg wrote:
>> On Wed, Dec 18, 2019 at 02:28:28PM +0200, Claudiu Beznea wrote:
>>> From: Peter Rosin
>>>
>>> The intention was to only select a higher pixel-clock rate than the
>>> requested, if a slight overclocking would result in a rate significantly
>>> closer to the requested rate than if the conservative lower pixel-clock
>>> rate is selected. The fixed patch has the logic the other way around and
>>> actually prefers the higher frequency. Fix that.
>>>
>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock
>>> than requested")
>> The id is wrong here - the right one is:
>> 9946a3a9dbedaaacef8b7e94f6ac144f1daaf1de
>
> Right! Sorry for this one! Thank you for fixing it up.
Dito. This one was my fault. I wonder how I came up with the wrong id?
Probably some backport branch or something, but I'm not finding it. Oh
well, sorry again.
Cheers,
Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
On 2019-12-13 10:28, claudiu.bez...@microchip.com wrote:
>
>
> On 11.12.2019 15:28, Peter Rosin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 2019-12-11 12:45, claudiu.bez...@microchip.com wrote:
>>>
>>>
>>> On 10.12.2019 19:22, Peter Rosin wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>>> content is safe
>>>>
>>>> On 2019-12-10 15:59, claudiu.bez...@microchip.com wrote:
>>>>>
>>>>>
>>>>> On 10.12.2019 16:11, Peter Rosin wrote:
>>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>>>>> because allowing selecting a higher pixel clock may overclock
>>>>>>> LCD devices, not all of them being capable of this.
>>>>>>
>>>>>> Without this patch, there are panels that are *severly* underclocked (on
>>>>>> the
>>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't
>>>>>> remember
>>>>>> the exact figures).
>>>>>
>>>>> With patch that switches by default to 2xsystem clock for pixel clock, if
>>>>> using 133MHz system clock (as you specified in the patch I proposed for
>>>>> revert here) that would go, without this patch at 53MHz if 65MHz is
>>>>> requested. Correct me if I'm wrong.
>>>>
>>>> It might have been 53MHz, whatever it was it was too low for things to
>>>> work.
>>>>
>>>>>> And they are of course not capable of that. All panels
>>>>>> have *some* slack as to what frequencies are supported, and the patch was
>>>>>> written under the assumption that the preferred frequency of the panel
>>>>>> was
>>>>>> requested, which should leave at least a *little* headroom.
>>>>>
>>>>> I see, but from my point of view, the upper layers should decide what
>>>>> frequency settings should be done on the LCD controller and not let this
>>>>> at
>>>>> the driver's latitude.
>>>>
>>>> Right, but the upper layers do not support negotiating a frequency from
>>>> ranges. At least the didn't when the patch was written, and implementing
>>>> *that* seemed like a huge undertaking.
>>>>
>>>>>>
>>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock
>>>>>> it needs
>>>>>> and what it gets with/without the patch?
>>>>>
>>>>> I have 2 use cases:
>>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz.
>>>>> With
>>>>> the reverted patch the resulted computed pixel clock would be 80MHz.
>>>>> Previously it was at 66MHz
>>>>
>>>> I don't see how that's possible.
>>>>
>>>> [doing some calculation by hand]
>>>>
>>>> Arrgh. *blush*
>>>>
>>>> The code does not do what I intended for it to do.
>>>> Can you please try this instead of reverting?
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
>>>> From: Peter Rosin
>>>> Date: Tue, 10 Dec 2019 18:11:28 +0100
>>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
>>>>
>>>> The intention was to only select a higher pixel-clock rate than the
>>>> requested, if a slight overclocking would result in a rate significantly
>>>> closer to the requested rate than if the conservative lower pixel-clock
>>>> rate is selected. The fixed patch has the logic the other way around and
>>>> actually prefers the higher frequency. Fix that.
>>>>
>>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher
>>>> pixel-clock than requested")
>>>> Reported-by: Claudiu Beznea
>>>> Signed-off-by: Peter Rosin
>>>> ---
>>>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>
Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
On 2019-12-11 12:45, claudiu.bez...@microchip.com wrote:
>
>
> On 10.12.2019 19:22, Peter Rosin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 2019-12-10 15:59, claudiu.bez...@microchip.com wrote:
>>>
>>>
>>> On 10.12.2019 16:11, Peter Rosin wrote:
>>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>>> because allowing selecting a higher pixel clock may overclock
>>>>> LCD devices, not all of them being capable of this.
>>>>
>>>> Without this patch, there are panels that are *severly* underclocked (on
>>>> the
>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't
>>>> remember
>>>> the exact figures).
>>>
>>> With patch that switches by default to 2xsystem clock for pixel clock, if
>>> using 133MHz system clock (as you specified in the patch I proposed for
>>> revert here) that would go, without this patch at 53MHz if 65MHz is
>>> requested. Correct me if I'm wrong.
>>
>> It might have been 53MHz, whatever it was it was too low for things to work.
>>
>>>> And they are of course not capable of that. All panels
>>>> have *some* slack as to what frequencies are supported, and the patch was
>>>> written under the assumption that the preferred frequency of the panel was
>>>> requested, which should leave at least a *little* headroom.
>>>
>>> I see, but from my point of view, the upper layers should decide what
>>> frequency settings should be done on the LCD controller and not let this at
>>> the driver's latitude.
>>
>> Right, but the upper layers do not support negotiating a frequency from
>> ranges. At least the didn't when the patch was written, and implementing
>> *that* seemed like a huge undertaking.
>>
>>>>
>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it
>>>> needs
>>>> and what it gets with/without the patch?
>>>
>>> I have 2 use cases:
>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
>>> the reverted patch the resulted computed pixel clock would be 80MHz.
>>> Previously it was at 66MHz
>>
>> I don't see how that's possible.
>>
>> [doing some calculation by hand]
>>
>> Arrgh. *blush*
>>
>> The code does not do what I intended for it to do.
>> Can you please try this instead of reverting?
>>
>> Cheers,
>> Peter
>>
>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
>> From: Peter Rosin
>> Date: Tue, 10 Dec 2019 18:11:28 +0100
>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
>>
>> The intention was to only select a higher pixel-clock rate than the
>> requested, if a slight overclocking would result in a rate significantly
>> closer to the requested rate than if the conservative lower pixel-clock
>> rate is selected. The fixed patch has the logic the other way around and
>> actually prefers the higher frequency. Fix that.
>>
>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock
>> than requested")
>> Reported-by: Claudiu Beznea
>> Signed-off-by: Peter Rosin
>> ---
>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 9e34bce089d0..03691845d37a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct
>> drm_crtc *c)
>> int div_low = prate / mode_rate;
>>
>> if (div_low >= 2 &&
>> - ((prate / div_low - mode_rate) <
>> -10 * (mode_rate - prate / div)))
>> + (10 * (prate / div_low - mode_rate) <
>> +(mode_rate - prate / div)))
>
> I tested it on my setup (I have only one of those specified above) and it
> is OK. Doing some math for the other setup it should also be OK.
Glad to hear it, and thanks for testing/verify
Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
On 2019-12-10 15:59, claudiu.bez...@microchip.com wrote:
>
>
> On 10.12.2019 16:11, Peter Rosin wrote:
>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>> because allowing selecting a higher pixel clock may overclock
>>> LCD devices, not all of them being capable of this.
>>
>> Without this patch, there are panels that are *severly* underclocked (on the
>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>> the exact figures).
>
> With patch that switches by default to 2xsystem clock for pixel clock, if
> using 133MHz system clock (as you specified in the patch I proposed for
> revert here) that would go, without this patch at 53MHz if 65MHz is
> requested. Correct me if I'm wrong.
It might have been 53MHz, whatever it was it was too low for things to work.
>> And they are of course not capable of that. All panels
>> have *some* slack as to what frequencies are supported, and the patch was
>> written under the assumption that the preferred frequency of the panel was
>> requested, which should leave at least a *little* headroom.
>
> I see, but from my point of view, the upper layers should decide what
> frequency settings should be done on the LCD controller and not let this at
> the driver's latitude.
Right, but the upper layers do not support negotiating a frequency from
ranges. At least the didn't when the patch was written, and implementing
*that* seemed like a huge undertaking.
>>
>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it
>> needs
>> and what it gets with/without the patch?
>
> I have 2 use cases:
> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
> the reverted patch the resulted computed pixel clock would be 80MHz.
> Previously it was at 66MHz
I don't see how that's possible.
[doing some calculation by hand]
Arrgh. *blush*
The code does not do what I intended for it to do.
Can you please try this instead of reverting?
Cheers,
Peter
>From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
From: Peter Rosin
Date: Tue, 10 Dec 2019 18:11:28 +0100
Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
The intention was to only select a higher pixel-clock rate than the
requested, if a slight overclocking would result in a rate significantly
closer to the requested rate than if the conservative lower pixel-clock
rate is selected. The fixed patch has the logic the other way around and
actually prefers the higher frequency. Fix that.
Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock
than requested")
Reported-by: Claudiu Beznea
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 9e34bce089d0..03691845d37a 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc
*c)
int div_low = prate / mode_rate;
if (div_low >= 2 &&
- ((prate / div_low - mode_rate) <
-10 * (mode_rate - prate / div)))
+ (10 * (prate / div_low - mode_rate) <
+(mode_rate - prate / div)))
/*
* At least 10 times better when using a higher
* frequency than requested, instead of a lower.
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
On 2019-12-10 14:24, Claudiu Beznea wrote:
> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
> because allowing selecting a higher pixel clock may overclock
> LCD devices, not all of them being capable of this.
Without this patch, there are panels that are *severly* underclocked (on the
magnitude of 40MHz instead of 65MHz or something like that, I don't remember
the exact figures). And they are of course not capable of that. All panels
have *some* slack as to what frequencies are supported, and the patch was
written under the assumption that the preferred frequency of the panel was
requested, which should leave at least a *little* headroom.
So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
and what it gets with/without the patch?
Or is the revert based on some theory of a perceived risk of toasting a panel?
In short, this revert regresses my use case and I would like at least a hook to
re-enable the removed logic.
Cheers,
Peter
> Cc: Peter Rosin
> Signed-off-by: Claudiu Beznea
> ---
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 721fa88bf71d..1a70dff1a417 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct
> drm_crtc *c)
> div = DIV_ROUND_UP(prate, mode_rate);
> if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
> div = ATMEL_HLCDC_CLKDIV_MASK;
> - } else {
> - int div_low = prate / mode_rate;
> -
> - if (div_low >= 2 &&
> - ((prate / div_low - mode_rate) <
> - 10 * (mode_rate - prate / div)))
> - /*
> - * At least 10 times better when using a higher
> - * frequency than requested, instead of a lower.
> - * So, go with that.
> - */
> - div = div_low;
> }
>
> cfg |= ATMEL_HLCDC_CLKDIV(div);
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/3] fbdev: fbmem: avoid exporting fb_center_logo
On 2019-08-27 13:35, Geert Uytterhoeven wrote: > Hi Peter, > > On Tue, Aug 27, 2019 at 1:09 PM Peter Rosin wrote: >> The variable is only ever used from fbcon.c which is linked into the >> same module. Therefore, the export is not needed. >> >> Signed-off-by: Peter Rosin > > Reviewed-by: Geert Uytterhoeven > > But note that the same is true for fb_class, so perhaps it can be added > (or better, removed ;-)? Right. Someone please let me know if 3/3 needs to be extended. I'm also happy to just drop it... > Once drivers/staging/olpc_dcon/olpc_dcon.c stops abusing registered_fb[] > and num_registered_fb, those can go, too. > > Does anyone remembe why au1200fb calls fb_prepare_logo() and fb_show_logo() > itself? Maybe there should be a small drivers/video/fbdev/core/fbmem.h file (or something) with these "internal" declarations, to hide some clutter currently in include/linux/fb.h? Feels like that could be done later, after these other cleanups you mention, so that the new file has a few more things to declare. Cheers, Peter
[PATCH v3 2/3] fbdev: fbmem: allow overriding the number of bootup logos
Probably most useful if you want no logo at all, or if you only want one
logo regardless of how many CPU cores you have.
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 5 +
drivers/video/fbdev/core/fbcon.c | 7 +++
drivers/video/fbdev/core/fbmem.c | 12 +---
include/linux/fb.h | 1 +
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index 65ba40255137..e57a3d1d085a 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -174,6 +174,11 @@ C. Boot options
displayed due to multiple CPUs, the collected line of logos is moved
as a whole.
+9. fbcon=logo-count:
+
+ The value 'n' overrides the number of bootup logos. 0 disables the
+ logo, and -1 gives the default which is the number of online CPUs.
+
C. Attaching, Detaching and Unloading
Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c9235a2f42f8..bb6ae995c2e5 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -536,6 +536,13 @@ static int __init fb_console_setup(char *this_opt)
fb_center_logo = true;
continue;
}
+
+ if (!strncmp(options, "logo-count:", 11)) {
+ options += 11;
+ if (*options)
+ fb_logo_count = simple_strtol(options,
, 0);
+ continue;
+ }
}
return 1;
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 64dd732021d8..c7ddcb72025b 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -56,6 +56,8 @@ EXPORT_SYMBOL(num_registered_fb);
bool fb_center_logo __read_mostly;
EXPORT_SYMBOL(fb_center_logo);
+int fb_logo_count __read_mostly = -1;
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -620,7 +622,7 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
memset(_logo, 0, sizeof(struct logo_data));
if (info->flags & FBINFO_MISC_TILEBLITTING ||
- info->fbops->owner)
+ info->fbops->owner || !fb_logo_count)
return 0;
if (info->fix.visual == FB_VISUAL_DIRECTCOLOR) {
@@ -686,10 +688,14 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
int fb_show_logo(struct fb_info *info, int rotate)
{
+ unsigned int count;
int y;
- y = fb_show_logo_line(info, rotate, fb_logo.logo, 0,
- num_online_cpus());
+ if (!fb_logo_count)
+ return 0;
+
+ count = fb_logo_count < 0 ? num_online_cpus() : fb_logo_count;
+ y = fb_show_logo_line(info, rotate, fb_logo.logo, 0, count);
y = fb_show_extra_logos(info, y, rotate);
return y;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 303771264644..e37f72b2efca 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -630,6 +630,7 @@ extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
extern int num_registered_fb;
extern bool fb_center_logo;
+extern int fb_logo_count;
extern struct class *fb_class;
#define for_each_registered_fb(i) \
--
2.11.0
[PATCH v3 1/3] fbdev: fix numbering of fbcon options
Three shall be the number thou shalt count, and the number of the
counting shall be three. Four shalt thou not count...
One! Two! Five!
Fixes: efb985f6b265 ("[PATCH] fbcon: Console Rotation - Add framebuffer console
documentation")
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index ebca41785abe..65ba40255137 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -127,7 +127,7 @@ C. Boot options
is typically located on the same video card. Thus, the consoles that
are controlled by the VGA console will be garbled.
-4. fbcon=rotate:
+5. fbcon=rotate:
This option changes the orientation angle of the console display. The
value 'n' accepts the following:
@@ -152,21 +152,21 @@ C. Boot options
Actually, the underlying fb driver is totally ignorant of console
rotation.
-5. fbcon=margin:
+6. fbcon=margin:
This option specifies the color of the margins. The margins are the
leftover area at the right and the bottom of the screen that are not
used by text. By default, this area will be black. The 'color' value
is an integer number that depends on the framebuffer driver being used.
-6. fbcon=nodefer
+7. fbcon=nodefer
If the kernel is compiled with deferred fbcon takeover support, normally
the framebuffer contents, left in place by the firmware/bootloader, will
be preserved until there actually is some text is output to the console.
This option causes fbcon to bind immediately to the fbdev device.
-7. fbcon=logo-pos:
+8. fbcon=logo-pos:
The only possible 'location' is 'center' (without quotes), and when
given, the bootup logo is moved from the default top-left corner
--
2.11.0
[PATCH v3 3/3] fbdev: fbmem: avoid exporting fb_center_logo
The variable is only ever used from fbcon.c which is linked into the same module. Therefore, the export is not needed. Signed-off-by: Peter Rosin --- drivers/video/fbdev/core/fbmem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index c7ddcb72025b..d45e59ac351b 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -54,7 +54,6 @@ int num_registered_fb __read_mostly; EXPORT_SYMBOL(num_registered_fb); bool fb_center_logo __read_mostly; -EXPORT_SYMBOL(fb_center_logo); int fb_logo_count __read_mostly = -1; -- 2.11.0
[PATCH v3 0/3] Add possibility to specify the number of displayed logos
Hi! The first patch fixes the fact that there are two items numbered "4" in the list of fbcon options. This bug is a teenager... The second patch extends that list with a new option that allows the user to display any number of logos (that fits on the screen). I need it to limit the display to only one logo instead of one for each CPU core. Changes since v2 - make -1 the default and make 0 disable the logo [Geert Uytterhoeven] - cpu -> CPU Changes since v1 - do not needlessly export fb_logo_count [Matthew Wilcox] - added patch 3/3, which removes the export of fb_center_logo Cheers, Peter Peter Rosin (3): fbdev: fix numbering of fbcon options fbdev: fbmem: allow overriding the number of bootup logos fbdev: fbmem: avoid exporting fb_center_logo Documentation/fb/fbcon.rst | 13 + drivers/video/fbdev/core/fbcon.c | 7 +++ drivers/video/fbdev/core/fbmem.c | 13 + include/linux/fb.h | 1 + 4 files changed, 26 insertions(+), 8 deletions(-) -- 2.11.0
Re: [PATCH v2 2/3] fbdev: fbmem: allow overriding the number of bootup logos
On 2019-08-27 10:36, Geert Uytterhoeven wrote: > Hi Peter, > > On Mon, Aug 26, 2019 at 10:46 PM Peter Rosin wrote: >> Probably most useful if you only want one logo regardless of how many >> CPU cores you have. >> >> Signed-off-by: Peter Rosin > > Thanks for your patch! > >> --- a/Documentation/fb/fbcon.rst >> +++ b/Documentation/fb/fbcon.rst >> @@ -174,6 +174,11 @@ C. Boot options >> displayed due to multiple CPUs, the collected line of logos is moved >> as a whole. >> >> +9. fbcon=logo-count: >> + >> + The value 'n' overrides the number of bootup logos. Zero gives the >> + default, which is the number of online cpus. > > Isn't that a bit unexpected for the user? > What about making -1 the default (auto), and zero meaning no logos? I just naively assumed there was some other mechanism to disable it. Sigh, I'll take a look. v3 coming up... Cheers, Peter
[PATCH v2 0/3] Add possibility to specify the number of displayed logos
Hi! The first patch fixes the fact that there are two items numbered "4" in the list of fbcon options. This bug is a teenager... The second patch extends that list with a new option that allows the user to display any number of logos (that fits on the screen). I need it to limit the display to only one logo instead of one for each CPU core. Changes since v1 - do not needlessly export fb_logo_count [Matthew Wilcox] - added patch 3/3, which removes the export of fb_center_logo Cheers, Peter Peter Rosin (3): fbdev: fix numbering of fbcon options fbdev: fbmem: allow overriding the number of bootup logos fbdev: fbmem: avoid exporting fb_center_logo Documentation/fb/fbcon.rst | 13 + drivers/video/fbdev/core/fbcon.c | 7 +++ drivers/video/fbdev/core/fbmem.c | 5 +++-- include/linux/fb.h | 1 + 4 files changed, 20 insertions(+), 6 deletions(-) -- 2.11.0
[PATCH v2 3/3] fbdev: fbmem: avoid exporting fb_center_logo
The variable is only ever used from fbcon.c which is linked into the same module. Therefore, the export is not needed. Signed-off-by: Peter Rosin --- drivers/video/fbdev/core/fbmem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index a7d8022db516..17e250a515b0 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -54,7 +54,6 @@ int num_registered_fb __read_mostly; EXPORT_SYMBOL(num_registered_fb); bool fb_center_logo __read_mostly; -EXPORT_SYMBOL(fb_center_logo); unsigned int fb_logo_count __read_mostly; -- 2.11.0
[PATCH v2 2/3] fbdev: fbmem: allow overriding the number of bootup logos
Probably most useful if you only want one logo regardless of how many
CPU cores you have.
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 5 +
drivers/video/fbdev/core/fbcon.c | 7 +++
drivers/video/fbdev/core/fbmem.c | 4 +++-
include/linux/fb.h | 1 +
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index 65ba40255137..9f0b399d8d4e 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -174,6 +174,11 @@ C. Boot options
displayed due to multiple CPUs, the collected line of logos is moved
as a whole.
+9. fbcon=logo-count:
+
+ The value 'n' overrides the number of bootup logos. Zero gives the
+ default, which is the number of online cpus.
+
C. Attaching, Detaching and Unloading
Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c9235a2f42f8..be4bc5540aad 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -536,6 +536,13 @@ static int __init fb_console_setup(char *this_opt)
fb_center_logo = true;
continue;
}
+
+ if (!strncmp(options, "logo-count:", 11)) {
+ options += 11;
+ if (*options)
+ fb_logo_count = simple_strtoul(options,
, 0);
+ continue;
+ }
}
return 1;
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 64dd732021d8..a7d8022db516 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -56,6 +56,8 @@ EXPORT_SYMBOL(num_registered_fb);
bool fb_center_logo __read_mostly;
EXPORT_SYMBOL(fb_center_logo);
+unsigned int fb_logo_count __read_mostly;
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -689,7 +691,7 @@ int fb_show_logo(struct fb_info *info, int rotate)
int y;
y = fb_show_logo_line(info, rotate, fb_logo.logo, 0,
- num_online_cpus());
+ fb_logo_count ?: num_online_cpus());
y = fb_show_extra_logos(info, y, rotate);
return y;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 303771264644..5f2b05406262 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -630,6 +630,7 @@ extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
extern int num_registered_fb;
extern bool fb_center_logo;
+extern unsigned int fb_logo_count;
extern struct class *fb_class;
#define for_each_registered_fb(i) \
--
2.11.0
[PATCH v2 1/3] fbdev: fix numbering of fbcon options
Three shall be the number thou shalt count, and the number of the
counting shall be three. Four shalt thou not count...
One! Two! Five!
Fixes: efb985f6b265 ("[PATCH] fbcon: Console Rotation - Add framebuffer console
documentation")
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index ebca41785abe..65ba40255137 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -127,7 +127,7 @@ C. Boot options
is typically located on the same video card. Thus, the consoles that
are controlled by the VGA console will be garbled.
-4. fbcon=rotate:
+5. fbcon=rotate:
This option changes the orientation angle of the console display. The
value 'n' accepts the following:
@@ -152,21 +152,21 @@ C. Boot options
Actually, the underlying fb driver is totally ignorant of console
rotation.
-5. fbcon=margin:
+6. fbcon=margin:
This option specifies the color of the margins. The margins are the
leftover area at the right and the bottom of the screen that are not
used by text. By default, this area will be black. The 'color' value
is an integer number that depends on the framebuffer driver being used.
-6. fbcon=nodefer
+7. fbcon=nodefer
If the kernel is compiled with deferred fbcon takeover support, normally
the framebuffer contents, left in place by the firmware/bootloader, will
be preserved until there actually is some text is output to the console.
This option causes fbcon to bind immediately to the fbdev device.
-7. fbcon=logo-pos:
+8. fbcon=logo-pos:
The only possible 'location' is 'center' (without quotes), and when
given, the bootup logo is moved from the default top-left corner
--
2.11.0
[PATCH 0/2] Add possibility to specify the number of displayed logos
Hi! The first patch fixes the fact that there are two items numbered "4" in the list of fbcon options. This bug is a teenager... The second patch extends that list with a new option that allows the user to display any number of logos (that fits on the screen). I need it to limit the display to only one logo instead of one for each CPU core. Cheers, Peter Peter Rosin (2): fbdev: fix numbering of fbcon options fbdev: fbmem: allow overriding the number of bootup logos Documentation/fb/fbcon.rst | 13 + drivers/video/fbdev/core/fbcon.c | 7 +++ drivers/video/fbdev/core/fbmem.c | 5 - include/linux/fb.h | 1 + 4 files changed, 21 insertions(+), 5 deletions(-) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] fbdev: fbmem: allow overriding the number of bootup logos
Probably most useful if you only want one logo regardless of how many
CPU cores you have.
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 5 +
drivers/video/fbdev/core/fbcon.c | 7 +++
drivers/video/fbdev/core/fbmem.c | 5 -
include/linux/fb.h | 1 +
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index 65ba40255137..9f0b399d8d4e 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -174,6 +174,11 @@ C. Boot options
displayed due to multiple CPUs, the collected line of logos is moved
as a whole.
+9. fbcon=logo-count:
+
+ The value 'n' overrides the number of bootup logos. Zero gives the
+ default, which is the number of online cpus.
+
C. Attaching, Detaching and Unloading
Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c9235a2f42f8..be4bc5540aad 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -536,6 +536,13 @@ static int __init fb_console_setup(char *this_opt)
fb_center_logo = true;
continue;
}
+
+ if (!strncmp(options, "logo-count:", 11)) {
+ options += 11;
+ if (*options)
+ fb_logo_count = simple_strtoul(options,
, 0);
+ continue;
+ }
}
return 1;
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 64dd732021d8..4c57d522b72e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -56,6 +56,9 @@ EXPORT_SYMBOL(num_registered_fb);
bool fb_center_logo __read_mostly;
EXPORT_SYMBOL(fb_center_logo);
+unsigned int fb_logo_count __read_mostly;
+EXPORT_SYMBOL(fb_logo_count);
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -689,7 +692,7 @@ int fb_show_logo(struct fb_info *info, int rotate)
int y;
y = fb_show_logo_line(info, rotate, fb_logo.logo, 0,
- num_online_cpus());
+ fb_logo_count ?: num_online_cpus());
y = fb_show_extra_logos(info, y, rotate);
return y;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 303771264644..5f2b05406262 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -630,6 +630,7 @@ extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
extern int num_registered_fb;
extern bool fb_center_logo;
+extern unsigned int fb_logo_count;
extern struct class *fb_class;
#define for_each_registered_fb(i) \
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] fbdev: fix numbering of fbcon options
Three shall be the number thou shalt count, and the number of the
counting shall be three. Four shalt thou not count...
One! Two! Five!
Fixes: efb985f6b265 ("[PATCH] fbcon: Console Rotation - Add framebuffer console
documentation")
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index ebca41785abe..65ba40255137 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -127,7 +127,7 @@ C. Boot options
is typically located on the same video card. Thus, the consoles that
are controlled by the VGA console will be garbled.
-4. fbcon=rotate:
+5. fbcon=rotate:
This option changes the orientation angle of the console display. The
value 'n' accepts the following:
@@ -152,21 +152,21 @@ C. Boot options
Actually, the underlying fb driver is totally ignorant of console
rotation.
-5. fbcon=margin:
+6. fbcon=margin:
This option specifies the color of the margins. The margins are the
leftover area at the right and the bottom of the screen that are not
used by text. By default, this area will be black. The 'color' value
is an integer number that depends on the framebuffer driver being used.
-6. fbcon=nodefer
+7. fbcon=nodefer
If the kernel is compiled with deferred fbcon takeover support, normally
the framebuffer contents, left in place by the firmware/bootloader, will
be preserved until there actually is some text is output to the console.
This option causes fbcon to bind immediately to the fbdev device.
-7. fbcon=logo-pos:
+8. fbcon=logo-pos:
The only possible 'location' is 'center' (without quotes), and when
given, the bootup logo is moved from the default top-left corner
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] fbdev: fbmem: allow overriding the number of bootup logos
On 2019-08-24 17:34, Matthew Wilcox wrote: > On Fri, Aug 23, 2019 at 08:47:47AM +0000, Peter Rosin wrote: >> +++ b/drivers/video/fbdev/core/fbcon.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -56,6 +56,9 @@ EXPORT_SYMBOL(num_registered_fb); >> bool fb_center_logo __read_mostly; >> EXPORT_SYMBOL(fb_center_logo); >> >> +unsigned int fb_logo_count __read_mostly; >> +EXPORT_SYMBOL(fb_logo_count); > > Why does this symbol need to be exported? As I read the Makefile, fbcon > and fbmem are combined into the same module, so while the symbol needs > to be non-static, it doesn't need to be exported to other modules. I guess you are right. I'll send a v2 tomorrow with an added patch to unexport the fb_center_logo variable while at it... Thanks for the feedback. Cheers, Peter
Re: [PATCH 00/34] treewide: simplify getting the adapter of an I2C client
On 2019-06-08 12:55, Wolfram Sang wrote: > While preparing a refactoring series, I noticed that some drivers use a > complicated way of determining the adapter of a client. The easy way is > to use the intended pointer: client->adapter > > These drivers do: > to_i2c_adapter(client->dev.parent); > > The I2C core populates the parent pointer as: > client->dev.parent = >adapter->dev; > > Now take into consideration that > to_i2c_adapter(>dev); > > is a complicated way of saying 'adapter', then we can even formally > prove that the complicated expression can be simplified by using > client->adapter. > > The conversion was done using a coccinelle script with some manual > indentation fixes applied on top. > > To avoid a brown paper bag mistake, I double checked this on a Renesas > Salvator-XS board (R-Car M3N) and verified both expression result in the > same pointer. Other than that, the series is only build tested. Similar things go on in: drivers/hwmon/lm90.c drivers/leds/leds-is31fl319x.c drivers/of/unittest.c Those have this pattern: struct device *dev = >dev; struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); And drivers/rtc/rtc-fm3130.c has a couple of these: tmp = i2c_transfer(to_i2c_adapter(fm3130->client->dev.parent), ...); where fm3130->client is of type "struct i2c_client *" Cheers, Peter
Re: [PATCH 0/4] drm/atmel-hlcdc: fix plane clipping/rotation issues
On 2019-01-27 09:27, Boris Brezillon wrote: > On Thu, 10 Jan 2019 15:10:28 + > Peter Rosin wrote: > >> Hi! >> >> I found an unfortunate issue while recoding plane handling to use >> drm_atomic_helper_check_plane_state(). The driver rotates clockwise, >> which is not correct. I simply fixed it (patch 1/4), but maybe that >> will cause regressions for unsuspecting users who simply assumed >> that the clockwise rotation was correct? I don't know what to do >> about that? Adding an option to get the old broken behavior seems >> useless, wouldn't it be just as easy to just fix whatever app to >> rotate the other way instead of adding an option somewhere? >> >> I have only tested this series on sama5d3, but I did check the docs >> for various other chips (sama5d2, sama5d4, sam9n12, sam9g15, sam9g35 >> and sam9x35) supported by the driver (relevant to patch 4/4). >> >> Cheers, >> Peter >> >> Peter Rosin (4): >> drm/atmel-hlcdc: rotate planes counterclockwise >> drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated >> drm/atmel-hlcdc: fix clipping of planes > > Queued patches 1-3 to drm-misc-next. Great, thanks. >> drm/atmel-hlcdc: do not immediately disable planes, wait for next >> frame > > Still waiting for Nicolas feedback on this one. [Adding back Nicolas, he seems to have gone missing from the list recipients.] I have done some testing of that patch and for me it's a definite improvement. The test I did was removing a white plane from a white background. Without the patch, the driver will output black where the plane was for the current frame (since the driver does that disc-area thing for the largest hidden part of the background). With the patch, I get no visual glitches when removing a plane. I use a plane to scroll a text, and if you know what to look for, the black rectangle that flickers by as the plane with the scrolling text is removed is little bit disturbing. Not a significant problem, and maybe only geeks notice it, but still... Just wanted to say that the resulting "black hole" mentioned in the other thread really does exist and that the patch may make sense beyond the fact that it removes usage of undocumented features. I have not seen any bad side effects fro the patch, but admittedly my testing was very limited and I did not try to remove the plane while doing other stuff with the driver. So, there might still be reasons for removing planes immediately... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/bridge: lvds-encoder: remove surplus NULL checks
The gpio API explicitly allows skipping the NULL check, precisely to
allow for neat support for optional gpios. Which is exactly what is at
play here.
Reported-by: Andrzej Hajda
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 36d8557bc097..584007eaf6e1 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -36,8 +36,7 @@ static void lvds_encoder_enable(struct drm_bridge *bridge)
struct lvds_encoder,
bridge);
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
}
static void lvds_encoder_disable(struct drm_bridge *bridge)
@@ -46,8 +45,7 @@ static void lvds_encoder_disable(struct drm_bridge *bridge)
struct lvds_encoder,
bridge);
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
}
static struct drm_bridge_funcs funcs = {
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd, line option
On 2019-01-16 17:45, Bartlomiej Zolnierkiewicz wrote: > On 01/07/2019 11:35 AM, Peter Rosin wrote: >> Right. So, here's an update... >> >> Again, it would probably be best if this went in before 5.0 is released. >> >> Changes since v1: >> - rename from fbcon=center-logo option to fbcon=logo-pos:center (and adjust >> the help text accordingly). >> >> Cheers, >> Peter >> >> From ebfb2feb7ea4298ac9c222e6ea6f9c9a92452084 Mon Sep 17 00:00:00 2001 >> From: Peter Rosin >> Cc: Bartlomiej Zolnierkiewicz >> Cc: Jonathan Corbet >> Cc: Peter Rosin >> Cc: dri-devel@lists.freedesktop.org >> Cc: linux-fb...@vger.kernel.org >> Cc: linux-...@vger.kernel.org >> To: linux-ker...@vger.kernel.org >> Date: Mon, 7 Jan 2019 08:35:26 +0100 >> Subject: [PATCH v2] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd >> line option >> >> A command line option is much more flexible than a config option and >> the supporting code is small. Gets rid of #ifdefs in the code too... >> >> Suggested-by: Geert Uytterhoeven >> Signed-off-by: Peter Rosin > > Patch queued for 5.0, thanks. Hmm, I see the patch on the fbdev-for-next branch, but that sounds like stuff destined for 5.1? Maybe you simply don't have any fbdev-for-current or fbdev-fixes branch or something like that? Just checking to make sure we're on the same page... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated
On 2019-01-10 18:48, Boris Brezillon wrote:
> On Thu, 10 Jan 2019 15:10:39 +
> Peter Rosin wrote:
>
>> The destination crtc rectangle is independent of source plane rotation.
>>
>> Signed-off-by: Peter Rosin
>> ---
>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> index ea8fc0deb814..d6f93f029020 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
>> @@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct
>> drm_plane *p,
>> * Swap width and size in case of 90 or 270 degrees rotation
>> */
>> if (drm_rotation_90_or_270(state->base.rotation)) {
>> -tmp = state->crtc_w;
>> -state->crtc_w = state->crtc_h;
>> -state->crtc_h = tmp;
>
> Again, I guess I assumed ->crtc_h/w were the width and height before
> rotation when I initially added rotation support.
And I thought so too, possibly since I have only been doing drm-stuff
with this driver, but I also suspect that the incompleteness of the
libdrm modetest program is to blame. I don't think it's possible to
specify individual src and dst rectangles with it, and that seems
rather limiting when dealing with rotated planes. I can easily see
why someone using modetest thinks the crtc rect should be rotated by
the driver...
> This change might break users too.
Right you are, and the same impossible scenario. Fix things to do the
right thing and risk breaking users, or don't and preserve the buggy
non-portable issues of the driver making it unusable for others.
I don't care either way, because rotating planes with this stride-
method is practically useless here. It simply requires to much
memory bandwidth. I might work ok for smaller panels with lower
pixel clock frequencies though? I think the LCDC might read the same
data more than once when data is not in the "natural" order? (no,
I do not need an answer to this question, and I do not have time to
dig in this area at the moment...)
However, if you can't do both patch 1 and 2 (because users regress),
then patch 3 is no good either. The reason is that
drm_atomic_helper_check_plane_state assumes the rotational
properties fixed by patch 1 and 2, and the behavior is "odd" if you
have that wrong.
Cheers,
Peter
>> tmp = state->src_w;
>> state->src_w = state->src_h;
>> state->src_h = tmp;
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 4/5] drm/bridge: lvds-encoder: add dev helper variable in .probe()
Make the code easier to read and modify.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..f6770e83d49d 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -34,48 +34,47 @@ static struct drm_bridge_funcs funcs = {
static int lvds_encoder_probe(struct platform_device *pdev)
{
+ struct device *dev = >dev;
struct device_node *port;
struct device_node *endpoint;
struct device_node *panel_node;
struct drm_panel *panel;
struct lvds_encoder *lvds_encoder;
- lvds_encoder = devm_kzalloc(>dev, sizeof(*lvds_encoder),
- GFP_KERNEL);
+ lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
if (!lvds_encoder)
return -ENOMEM;
/* Locate the panel DT node. */
- port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
+ port = of_graph_get_port_by_id(dev->of_node, 1);
if (!port) {
- dev_dbg(>dev, "port 1 not found\n");
+ dev_dbg(dev, "port 1 not found\n");
return -ENXIO;
}
endpoint = of_get_child_by_name(port, "endpoint");
of_node_put(port);
if (!endpoint) {
- dev_dbg(>dev, "no endpoint for port 1\n");
+ dev_dbg(dev, "no endpoint for port 1\n");
return -ENXIO;
}
panel_node = of_graph_get_remote_port_parent(endpoint);
of_node_put(endpoint);
if (!panel_node) {
- dev_dbg(>dev, "no remote endpoint for port 1\n");
+ dev_dbg(dev, "no remote endpoint for port 1\n");
return -ENXIO;
}
panel = of_drm_find_panel(panel_node);
of_node_put(panel_node);
if (!panel) {
- dev_dbg(>dev, "panel not found, deferring probe\n");
+ dev_dbg(dev, "panel not found, deferring probe\n");
return -EPROBE_DEFER;
}
lvds_encoder->panel_bridge =
- devm_drm_panel_bridge_add(>dev,
- panel, DRM_MODE_CONNECTOR_LVDS);
+ devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_LVDS);
if (IS_ERR(lvds_encoder->panel_bridge))
return PTR_ERR(lvds_encoder->panel_bridge);
@@ -83,7 +82,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
* but we need a bridge attached to our of_node for our user
* to look up.
*/
- lvds_encoder->bridge.of_node = pdev->dev.of_node;
+ lvds_encoder->bridge.of_node = dev->of_node;
lvds_encoder->bridge.funcs =
drm_bridge_add(_encoder->bridge);
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 5/5] drm/bridge: lvds-encoder: add powerdown-gpios support
Optionally power down the LVDS-encoder when it is not in use.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index f6770e83d49d..36d8557bc097 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -11,11 +11,13 @@
#include
#include
+#include
#include
struct lvds_encoder {
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct gpio_desc *powerdown_gpio;
};
static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +30,30 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
bridge);
}
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
+}
+
static struct drm_bridge_funcs funcs = {
.attach = lvds_encoder_attach,
+ .enable = lvds_encoder_enable,
+ .disable = lvds_encoder_disable,
};
static int lvds_encoder_probe(struct platform_device *pdev)
@@ -45,6 +69,16 @@ static int lvds_encoder_probe(struct platform_device *pdev)
if (!lvds_encoder)
return -ENOMEM;
+ lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(lvds_encoder->powerdown_gpio)) {
+ int err = PTR_ERR(lvds_encoder->powerdown_gpio);
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "powerdown GPIO failure: %d\n", err);
+ return err;
+ }
+
/* Locate the panel DT node. */
port = of_graph_get_port_by_id(dev->of_node, 1);
if (!port) {
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/4] drm/atmel-hlcdc: fix plane clipping/rotation issues
On 2019-01-10 21:16, Sam Ravnborg wrote: > Hi Peter. > > (Hijacking this thread as I lost the orginal mails) Assuming you wanted to reply to this patch? https://patchwork.kernel.org/patch/10753571/ >>> I found an unfortunate issue while recoding plane handling to use >>> drm_atomic_helper_check_plane_state(). The driver rotates clockwise, >>> which is not correct. I simply fixed it (patch 1/4), but maybe that >>> will cause regressions for unsuspecting users who simply assumed >>> that the clockwise rotation was correct? I don't know what to do >>> about that? Adding an option to get the old broken behavior seems >>> useless, wouldn't it be just as easy to just fix whatever app to >>> rotate the other way instead of adding an option somewhere? >> >> Hm, rotation support has been added before the standard rotation >> property was created, and at that time I assumed rotation was clockwise >> (which apparently was an unwise choice). Anyway, I don't have a >> solution for this problem, so I'll let Nicolas decide if it's >> acceptable to change the rotation behavior. >> >>> I have only tested this series on sama5d3, but I did check the docs >>> for various other chips (sama5d2, sama5d4, sam9n12, sam9g15, sam9g35 >>> and sam9x35) supported by the driver (relevant to patch 4/4). > > I wonder if, when this code path is anyway touched, could benefit > from drm_rect_rotate(). > > It is obviously not a simple replacement, but could it be used then > I hope the resulting code is simpler. What are you referring to that goes beyond what is done in patch 3/4 in this series? After setting up the strides, the only use of src_[xywh] are to calculate the scaling factors, and for that the position is irrelevant. I.e. src_x and src_y are not used. Sure, in some theoretical sense it might be good if src_[xy] are also transformed into the rotated coordinate system along with src_[wh], but it seems a bit backwards to switch over to struct drm_rect when the interesting properties are the width and height, not the coordinates of the corners. No strong feelings on the issue though... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 3/5] dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios
The name powerdown-gpios is the standard property name for the functionality covered by the previous pwdn-gpios name. This rename should be safe to do since the linux driver supporting the binding (lvds-encoder.c) never implemented the property, and no dts file names it. At least not upstream. Reviewed-by: Rob Herring Signed-off-by: Peter Rosin --- Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt index 527e236e9a2a..fee3c88e1a17 100644 --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt @@ -10,7 +10,7 @@ Required properties: Optional properties: -- pwdn-gpios: Power down control GPIO +- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low). Required nodes: -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 0/5] drm/bridge: various small lvds-encoder things
Hi! I'm not sure if I should have added the texas chips to the lvds_encoder_match list in the driver, right next to the thine,thc63lvdm83d entry, but ended up not doing that. That can always be added later, if needed... Changes since v3: - retained a (modified) note in lvds-transmitter.txt about encoders with additional device-specific properties - added tag from Rob on patch 3/5 Changes since v2: - changed from pwdn-gpios to powerdown-gpios after discussion with Rob with new patch 3/5 updating the thine,thc63lvdm83d binding as well as a consequence - added patch 4/5 which helps keep lines shorter in the lvds-encoder driver - added tag from Rob on patch 2/5 Changes since v1: - fork out the bindings for the texas chips into their own file in order to avoid clutter in the generic lvds-transmitter binding. - added a patch to remove some surplus stuff in the generic lvds-transmitter binding. Cheers, Peter Peter Rosin (5): dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter dt-bindings: display: bridge: lvds-transmitter: cleanup example dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios drm/bridge: lvds-encoder: add dev helper variable in .probe() drm/bridge: lvds-encoder: add powerdown-gpios support .../bindings/display/bridge/lvds-transmitter.txt | 12 ++--- .../bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +- .../bindings/display/bridge/ti,ds90c185.txt| 55 ++ drivers/gpu/drm/bridge/lvds-encoder.c | 53 + 4 files changed, 103 insertions(+), 19 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/5] dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter
DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
binding, which is meant to be generic.
The sister chip DS90C187 is similar to DS90C185, describe it here as well.
Signed-off-by: Peter Rosin
---
.../bindings/display/bridge/lvds-transmitter.txt | 10 ++--
.../bindings/display/bridge/ti,ds90c185.txt| 55 ++
2 files changed, 59 insertions(+), 6 deletions(-)
create mode 100644
Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..bc6960741cb5 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,13 +22,11 @@ among others.
Required properties:
-- compatible: Must be one or more of the following
- - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
- - "lvds-encoder" for a generic LVDS encoder device
+- compatible: Must be "lvds-encoder"
- When compatible with the generic version, nodes must list the
- device-specific version corresponding to the device first
- followed by the generic version.
+ Any encoder compatible with this generic binding, but with additional
+ properties not listed here, must list a device specific compatible first
+ followed by this generic compatible.
Required nodes:
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
new file mode 100644
index ..e575f996959a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
@@ -0,0 +1,55 @@
+Texas Instruments FPD-Link (LVDS) Serializer
+
+
+The DS90C185 and DS90C187 are low-power serializers for portable
+battery-powered applications that reduces the size of the RGB
+interface between the host GPU and the display.
+
+Required properties:
+
+- compatible: Should be
+ "ti,ds90c185", "lvds-encoder" for the TI DS90C185 FPD-Link Serializer
+ "ti,ds90c187", "lvds-encoder" for the TI DS90C187 FPD-Link Serializer
+
+Optional properties:
+
+- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
+
+Required nodes:
+
+The devices have two video ports. Their connections are modeled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for parallel input
+- Video port 1 for LVDS output
+
+
+Example
+---
+
+lvds-encoder {
+ compatible = "ti,ds90c185", "lvds-encoder";
+
+ powerdown-gpios = < 17 GPIO_ACTIVE_LOW>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lvds_enc_in: endpoint {
+ remote-endpoint = <_out_rgb>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ lvds_enc_out: endpoint {
+ remote-endpoint = <_panel_in>;
+ };
+ };
+ };
+};
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 2/5] dt-bindings: display: bridge: lvds-transmitter: cleanup example
Drop #address-cells and #size-cells from the root node in the
example, they are unused.
Reviewed-by: Rob Herring
Signed-off-by: Peter Rosin
---
Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 2 --
1 file changed, 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index bc6960741cb5..60091db5dfa5 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -42,8 +42,6 @@ Example
lvds-encoder {
compatible = "lvds-encoder";
- #address-cells = <1>;
- #size-cells = <0>;
ports {
#address-cells = <1>;
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/atmel-hlcdc: do not immediately disable planes, wait for next frame
On 2019-01-10 20:25, Boris Brezillon wrote:
> On Thu, 10 Jan 2019 18:51:21 +
> Peter Rosin wrote:
>
>> On 2019-01-10 18:29, Boris Brezillon wrote:
>>> On Thu, 10 Jan 2019 15:10:48 +
>>> Peter Rosin wrote:
>>>
>>>> The A2Q and UPDATE bits have no effect in the channel disable registers.
>>>> However, since they are present, assume that the intention is to disable
>>>> planes, not immediately as indicated by the RST bit, but on the next
>>>> frame shift since that is what A2Q and UPDATE means in the channel enable
>>>> registers.
>>>>
>>>> Disabling the plane on the next frame shift is done with the EN bit,
>>>> so use that.
>>>
>>> It's been a long time, but I think I had a good reason for forcing a
>>> reset. IIRC, when you don't do that and the CRTC is disabled before the
>>> plane, the EN bit stays around, and next time you queue a plane update,
>>> you'll start with an invalid buf pointer.
>>
>> It might be possible to clear the EN bit in ...CHDR before enabling the
>> plane in ...CHER. Or is that too late?
>
> I think I tried that, but I'm not sure (BTW, this change was done in
> bd4248bb5e8b ("drm: atmel-hlcdc: reset layer A2Q and UPDATE bits when
That patch is a big fat NOP if you read the documentation. Those bits
are marked with a '-' for all LCDC channel disable registers, for all
supported chips IIUC. Are the effects of those bits mentioned in any
errata?
It would be good with a comment that the present undocumented disable
method is intentional. That would have kept me from assuming the whole
thing was just copy-paste junk from ..._enable that happened to work.
>> disabling it")). Anyway, I'm not even sure this is still needed now
>> that atomic updates have a wait_for_flip_done/vblank() in the commit
>> path.
The documentation for the RST bit states "Resets the layer immediately.
The frame is aborted." which sounds a bit scary and heavy-handed. But
again, I don't know what that actually means and what the effects are
but that was the reason for me wanting to avoid the RST bit.
Cheers,
Peter
>> But this patch is not overly
>> important, I just wanted to avoid the resulting "black hole" when the
>> plane DMA is disabled mid-frame. But disabling planes is probably not
>> something that happens frequently and will perhaps not be noticed at
>> all...
>
> Okay. Other patches look good to me, I'm just waiting for Nicolas
> feedback before applying them.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/4] drm/atmel-hlcdc: rotate planes counterclockwise
Ouch, the driver rotates planes clockwise, which is simply not correct.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 30 -
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 47e0992f3908..ea8fc0deb814 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -691,13 +691,14 @@ static int atmel_hlcdc_plane_atomic_check(struct
drm_plane *p,
switch (state->base.rotation & DRM_MODE_ROTATE_MASK) {
case DRM_MODE_ROTATE_90:
- offset = ((y_offset + state->src_y + patched_src_w - 1)
/
- ydiv) * fb->pitches[i];
- offset += ((x_offset + state->src_x) / xdiv) *
- state->bpp[i];
- state->xstride[i] = ((patched_src_w - 1) / ydiv) *
- fb->pitches[i];
- state->pstride[i] = -fb->pitches[i] - state->bpp[i];
+ offset = ((y_offset + state->src_y) / ydiv) *
+fb->pitches[i];
+ offset += ((x_offset + state->src_x + patched_src_h -
1) /
+ xdiv) * state->bpp[i];
+ state->xstride[i] = -(((patched_src_w - 1) / ydiv) *
+ fb->pitches[i]) -
+ (2 * state->bpp[i]);
+ state->pstride[i] = fb->pitches[i] - state->bpp[i];
break;
case DRM_MODE_ROTATE_180:
offset = ((y_offset + state->src_y + patched_src_h - 1)
/
@@ -709,14 +710,13 @@ static int atmel_hlcdc_plane_atomic_check(struct
drm_plane *p,
state->pstride[i] = -2 * state->bpp[i];
break;
case DRM_MODE_ROTATE_270:
- offset = ((y_offset + state->src_y) / ydiv) *
-fb->pitches[i];
- offset += ((x_offset + state->src_x + patched_src_h -
1) /
- xdiv) * state->bpp[i];
- state->xstride[i] = -(((patched_src_w - 1) / ydiv) *
- fb->pitches[i]) -
- (2 * state->bpp[i]);
- state->pstride[i] = fb->pitches[i] - state->bpp[i];
+ offset = ((y_offset + state->src_y + patched_src_w - 1)
/
+ ydiv) * fb->pitches[i];
+ offset += ((x_offset + state->src_x) / xdiv) *
+ state->bpp[i];
+ state->xstride[i] = ((patched_src_w - 1) / ydiv) *
+ fb->pitches[i];
+ state->pstride[i] = -fb->pitches[i] - state->bpp[i];
break;
case DRM_MODE_ROTATE_0:
default:
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/4] drm/atmel-hlcdc: do not immediately disable planes, wait for next frame
The A2Q and UPDATE bits have no effect in the channel disable registers. However, since they are present, assume that the intention is to disable planes, not immediately as indicated by the RST bit, but on the next frame shift since that is what A2Q and UPDATE means in the channel enable registers. Disabling the plane on the next frame shift is done with the EN bit, so use that. Signed-off-by: Peter Rosin --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 05519e8c6586..f2f570642f84 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -728,9 +728,7 @@ static void atmel_hlcdc_plane_atomic_disable(struct drm_plane *p, /* Disable the layer */ atmel_hlcdc_layer_write_reg(>layer, ATMEL_HLCDC_LAYER_CHDR, - ATMEL_HLCDC_LAYER_RST | - ATMEL_HLCDC_LAYER_A2Q | - ATMEL_HLCDC_LAYER_UPDATE); + ATMEL_HLCDC_LAYER_EN); /* Clear all pending interrupts */ atmel_hlcdc_layer_read_reg(>layer, ATMEL_HLCDC_LAYER_ISR); -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated
The destination crtc rectangle is independent of source plane rotation.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index ea8fc0deb814..d6f93f029020 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane
*p,
* Swap width and size in case of 90 or 270 degrees rotation
*/
if (drm_rotation_90_or_270(state->base.rotation)) {
- tmp = state->crtc_w;
- state->crtc_w = state->crtc_h;
- state->crtc_h = tmp;
tmp = state->src_w;
state->src_w = state->src_h;
state->src_h = tmp;
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/4] drm/atmel-hlcdc: fix plane clipping/rotation issues
On 2019-01-10 18:45, Boris Brezillon wrote: > On Thu, 10 Jan 2019 15:10:28 + > Peter Rosin wrote: > >> Hi! >> >> I found an unfortunate issue while recoding plane handling to use >> drm_atomic_helper_check_plane_state(). The driver rotates clockwise, >> which is not correct. I simply fixed it (patch 1/4), but maybe that >> will cause regressions for unsuspecting users who simply assumed >> that the clockwise rotation was correct? I don't know what to do >> about that? Adding an option to get the old broken behavior seems >> useless, wouldn't it be just as easy to just fix whatever app to >> rotate the other way instead of adding an option somewhere? > > Hm, rotation support has been added before the standard rotation > property was created, and at that time I assumed rotation was clockwise > (which apparently was an unwise choice). Anyway, I don't have a > solution for this problem, so I'll let Nicolas decide if it's > acceptable to change the rotation behavior. Speaking of unwise, fbcon rotation is clockwise and drm rotation is counterclockwise. 'nuff said. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/4] drm/atmel-hlcdc: fix clipping of planes
With the help from drm_atomic_helper_check_plane_state function, clipping
now handles planes to be partially or totally off-screen. The plane is
disabled if it is not visible.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 162 +---
1 file changed, 61 insertions(+), 101 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index d6f93f029020..05519e8c6586 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -548,7 +548,8 @@ atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state
*c_state)
ovl_state = drm_plane_state_to_atmel_hlcdc_plane_state(ovl_s);
- if (!ovl_s->fb ||
+ if (!ovl_s->visible ||
+ !ovl_s->fb ||
ovl_s->fb->format->has_alpha ||
ovl_s->alpha != DRM_BLEND_ALPHA_OPAQUE)
continue;
@@ -600,15 +601,10 @@ static int atmel_hlcdc_plane_atomic_check(struct
drm_plane *p,
struct drm_framebuffer *fb = state->base.fb;
const struct drm_display_mode *mode;
struct drm_crtc_state *crtc_state;
- unsigned int patched_crtc_w;
- unsigned int patched_crtc_h;
- unsigned int patched_src_w;
- unsigned int patched_src_h;
unsigned int tmp;
- int x_offset = 0;
- int y_offset = 0;
int hsub = 1;
int vsub = 1;
+ int ret;
int i;
if (!state->base.crtc || !fb)
@@ -617,14 +613,21 @@ static int atmel_hlcdc_plane_atomic_check(struct
drm_plane *p,
crtc_state = drm_atomic_get_existing_crtc_state(s->state, s->crtc);
mode = _state->adjusted_mode;
- state->src_x = s->src_x;
- state->src_y = s->src_y;
- state->src_h = s->src_h;
- state->src_w = s->src_w;
- state->crtc_x = s->crtc_x;
- state->crtc_y = s->crtc_y;
- state->crtc_h = s->crtc_h;
- state->crtc_w = s->crtc_w;
+ ret = drm_atomic_helper_check_plane_state(s, crtc_state,
+ (1 << 16) / 2048,
+ INT_MAX, true, true);
+ if (ret || !s->visible)
+ return ret;
+
+ state->src_x = s->src.x1;
+ state->src_y = s->src.y1;
+ state->src_w = drm_rect_width(>src);
+ state->src_h = drm_rect_height(>src);
+ state->crtc_x = s->dst.x1;
+ state->crtc_y = s->dst.y1;
+ state->crtc_w = drm_rect_width(>dst);
+ state->crtc_h = drm_rect_height(>dst);
+
if ((state->src_x | state->src_y | state->src_w | state->src_h) &
SUBPIXEL_MASK)
return -EINVAL;
@@ -638,42 +641,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane
*p,
if (state->nplanes > ATMEL_HLCDC_LAYER_MAX_PLANES)
return -EINVAL;
- /*
-* Swap width and size in case of 90 or 270 degrees rotation
-*/
- if (drm_rotation_90_or_270(state->base.rotation)) {
- tmp = state->src_w;
- state->src_w = state->src_h;
- state->src_h = tmp;
- }
-
- if (state->crtc_x + state->crtc_w > mode->hdisplay)
- patched_crtc_w = mode->hdisplay - state->crtc_x;
- else
- patched_crtc_w = state->crtc_w;
-
- if (state->crtc_x < 0) {
- patched_crtc_w += state->crtc_x;
- x_offset = -state->crtc_x;
- state->crtc_x = 0;
- }
-
- if (state->crtc_y + state->crtc_h > mode->vdisplay)
- patched_crtc_h = mode->vdisplay - state->crtc_y;
- else
- patched_crtc_h = state->crtc_h;
-
- if (state->crtc_y < 0) {
- patched_crtc_h += state->crtc_y;
- y_offset = -state->crtc_y;
- state->crtc_y = 0;
- }
-
- patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w,
- state->crtc_w);
- patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h,
- state->crtc_h);
-
hsub = drm_format_horz_chroma_subsampling(fb->format->format);
vsub = drm_format_vert_chroma_subsampling(fb->format->format);
@@ -688,41 +655,38 @@ static int atmel_hlcdc_plane_atomic_check(struct
drm_plane *p,
switch (state->base.rotation & DRM_MODE_ROTATE_MASK) {
case DRM_MODE_ROTATE_90:
- offset = ((y_offset + state->src_y) / ydiv) *
+ offset = (state->src_y / ydiv) *
[PATCH 0/4] drm/atmel-hlcdc: fix plane clipping/rotation issues
Hi! I found an unfortunate issue while recoding plane handling to use drm_atomic_helper_check_plane_state(). The driver rotates clockwise, which is not correct. I simply fixed it (patch 1/4), but maybe that will cause regressions for unsuspecting users who simply assumed that the clockwise rotation was correct? I don't know what to do about that? Adding an option to get the old broken behavior seems useless, wouldn't it be just as easy to just fix whatever app to rotate the other way instead of adding an option somewhere? I have only tested this series on sama5d3, but I did check the docs for various other chips (sama5d2, sama5d4, sam9n12, sam9g15, sam9g35 and sam9x35) supported by the driver (relevant to patch 4/4). Cheers, Peter Peter Rosin (4): drm/atmel-hlcdc: rotate planes counterclockwise drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated drm/atmel-hlcdc: fix clipping of planes drm/atmel-hlcdc: do not immediately disable planes, wait for next frame drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 179 +--- 1 file changed, 67 insertions(+), 112 deletions(-) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/atmel-hlcdc: do not immediately disable planes, wait for next frame
On 2019-01-10 18:29, Boris Brezillon wrote: > On Thu, 10 Jan 2019 15:10:48 + > Peter Rosin wrote: > >> The A2Q and UPDATE bits have no effect in the channel disable registers. >> However, since they are present, assume that the intention is to disable >> planes, not immediately as indicated by the RST bit, but on the next >> frame shift since that is what A2Q and UPDATE means in the channel enable >> registers. >> >> Disabling the plane on the next frame shift is done with the EN bit, >> so use that. > > It's been a long time, but I think I had a good reason for forcing a > reset. IIRC, when you don't do that and the CRTC is disabled before the > plane, the EN bit stays around, and next time you queue a plane update, > you'll start with an invalid buf pointer. It might be possible to clear the EN bit in ...CHDR before enabling the plane in ...CHER. Or is that too late? But this patch is not overly important, I just wanted to avoid the resulting "black hole" when the plane DMA is disabled mid-frame. But disabling planes is probably not something that happens frequently and will perhaps not be noticed at all... Cheers, Peter >> >> Signed-off-by: Peter Rosin >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> index 05519e8c6586..f2f570642f84 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> @@ -728,9 +728,7 @@ static void atmel_hlcdc_plane_atomic_disable(struct >> drm_plane *p, >> >> /* Disable the layer */ >> atmel_hlcdc_layer_write_reg(>layer, ATMEL_HLCDC_LAYER_CHDR, >> -ATMEL_HLCDC_LAYER_RST | >> -ATMEL_HLCDC_LAYER_A2Q | >> -ATMEL_HLCDC_LAYER_UPDATE); >> +ATMEL_HLCDC_LAYER_EN); >> >> /* Clear all pending interrupts */ >> atmel_hlcdc_layer_read_reg(>layer, ATMEL_HLCDC_LAYER_ISR); > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atmel-hlcdc: prevent divide by zero
On 2019-01-09 11:12, Daniel Vetter wrote: > On Tue, Jan 08, 2019 at 12:31:36PM +0000, Peter Rosin wrote: >> While trying to temporarily hide a plane, one thing that was attempted >> was to call (from libdrm) >> >> drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0, >> 0, 0, 0, 0, 0, 0, 0, 0); >> >> This call causes a pair of "Division by zero in kernel." messages. Kill >> those messages, but preserve the current behaviour that also happen to >> make the plane disappear with the above call. >> >> Signed-off-by: Peter Rosin >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++ >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> Side note, when comparing with drm_atomic_helper_check_plane_state(), I get >> the feeling that the src rect should be clipped together with the crtc rect >> if/when clipping is needed. That function calls drm_rect_clip_scaled(), and >> the equivalent does not seem to happen here. Should clipping be performed >> on the src rect? > > Any reasons atmel can't switch over to that helper? Would compute a nice > ->visible state variable for you ... > -Daniel I have not researched that, and as stated above, I was not sure if that was the correct approach to begin with. Boris, any insights? Does this code predate the helper or something? Maybe there's some register bit that hides a plane explicitly, matching the ->visible state variable? I haven't looked at that either... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/atmel-hlcdc: prevent divide by zero
While trying to temporarily hide a plane, one thing that was attempted was to call (from libdrm) drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0, 0, 0, 0, 0, 0, 0, 0, 0); This call causes a pair of "Division by zero in kernel." messages. Kill those messages, but preserve the current behaviour that also happen to make the plane disappear with the above call. Signed-off-by: Peter Rosin --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) Side note, when comparing with drm_atomic_helper_check_plane_state(), I get the feeling that the src rect should be clipped together with the crtc rect if/when clipping is needed. That function calls drm_rect_clip_scaled(), and the equivalent does not seem to happen here. Should clipping be performed on the src rect? Cheers, Peter diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 3cc489b870fe..1bdb30dc218c 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -675,10 +675,16 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, state->crtc_y = 0; } - patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w, - state->crtc_w); - patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h, - state->crtc_h); + if (state->crtc_w) + patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w, + state->crtc_w); + else + patched_src_w = 0; + if (state->crtc_h) + patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h, + state->crtc_h); + else + patched_src_h = 0; hsub = drm_format_horz_chroma_subsampling(fb->format->format); vsub = drm_format_vert_chroma_subsampling(fb->format->format); -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line, option
On 2019-01-07 09:59, Peter Rosin wrote: > On 2019-01-07 09:40, Geert Uytterhoeven wrote: >> Hi Peter, >> >> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin wrote: >>> On 2019-01-06 10:33, Geert Uytterhoeven wrote: >>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin wrote: >>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these >>>>> extra logos are not considered when centering the first logo vertically. >>>>> >>>>> Signed-off-by: Peter Rosin >>>> >>>>> --- a/drivers/video/logo/Kconfig >>>>> +++ b/drivers/video/logo/Kconfig >>>>> @@ -10,6 +10,15 @@ menuconfig LOGO >>>>> >>>>> if LOGO >>>>> >>>>> +config FB_LOGO_CENTER >>>>> + bool "Center the logo" >>>>> + depends on FB=y >>>>> + help >>>>> + When this option is selected, the bootup logo is centered both >>>>> + horizontally and vertically. If more than one logo is displayed >>>>> + due to multiple CPUs, the collected line of logos is centered >>>>> + as a whole. >>>>> + >>>> >>>> Isn't a kernel command line option more suitable to configure the position >>>> of the logo? >>> >>> That didn't occur to me previously, but it does make sense now that you >>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we >>> can avoid possible regressions for folks who might start to rely on >>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this. >>> >>> Cheers, >>> Peter >>> >>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001 >>> From: Peter Rosin >>> Cc: Bartlomiej Zolnierkiewicz >>> Cc: Jonathan Corbet >>> Cc: Peter Rosin >>> Cc: dri-devel@lists.freedesktop.org >>> Cc: linux-fb...@vger.kernel.org >>> Cc: linux-...@vger.kernel.org >>> To: linux-ker...@vger.kernel.org >>> Date: Mon, 7 Jan 2019 08:35:26 +0100 >>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line >>> option >>> >>> A command line option is much more flexible than a config option and >>> the supporting code is small. Gets rid of #ifdefs in the code too... >>> >>> Suggested-by: Geert Uytterhoeven >>> Signed-off-by: Peter Rosin >> >> Thanks for your patch! >> >>> --- a/Documentation/fb/fbcon.txt >>> +++ b/Documentation/fb/fbcon.txt >>> @@ -163,6 +163,12 @@ C. Boot options >>> be preserved until there actually is some text is output to the >>> console. >>> This option causes fbcon to bind immediately to the fbdev device. >>> >>> +7. fbcon=center-logo >>> + >>> + When this option is selected, the bootup logo is centered both >>> + horizontally and vertically. If more than one logo is displayed due >>> to >>> + multiple CPUs, the collected line of logos is centered as a whole. >>> + >> >> What about making this more generic, than an option specific for centering? >> Else people want fbcon=left-logo or fbcon=right-logo soon? >> >> fbcon=logo-pos: >> >> With being "center", "left", "top", "right", "bottom", and combinations >> like "top,center"? Or is that complicating stuff too much? > > I'm not too keen on implementing all that when I have zero use for it. I can > however rename the trigger for the existing fb_center_logo bool to > > fbcon=logo-pos:center > > and add a check for "top,left" to reset the bool to false. Then the other > variants can be added later by whomever and when needed. > > Is that good enough? Ouch, I just realized that using a comma is a no-go, as that is already separating fbcon suboptions, so I suggest top-left instead. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd, line option
On 2019-01-07 10:11, Geert Uytterhoeven wrote: > Hi Peter, > > On Mon, Jan 7, 2019 at 10:03 AM Peter Rosin wrote: >> On 2019-01-07 09:59, Peter Rosin wrote: >>> On 2019-01-07 09:40, Geert Uytterhoeven wrote: >>>> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin wrote: >>>>> On 2019-01-06 10:33, Geert Uytterhoeven wrote: >>>>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin wrote: >>>>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these >>>>>>> extra logos are not considered when centering the first logo vertically. >>>>>>> >>>>>>> Signed-off-by: Peter Rosin >>>>>> >>>>>>> --- a/drivers/video/logo/Kconfig >>>>>>> +++ b/drivers/video/logo/Kconfig >>>>>>> @@ -10,6 +10,15 @@ menuconfig LOGO >>>>>>> >>>>>>> if LOGO >>>>>>> >>>>>>> +config FB_LOGO_CENTER >>>>>>> + bool "Center the logo" >>>>>>> + depends on FB=y >>>>>>> + help >>>>>>> + When this option is selected, the bootup logo is centered both >>>>>>> + horizontally and vertically. If more than one logo is >>>>>>> displayed >>>>>>> + due to multiple CPUs, the collected line of logos is centered >>>>>>> + as a whole. >>>>>>> + >>>>>> >>>>>> Isn't a kernel command line option more suitable to configure the >>>>>> position >>>>>> of the logo? >>>>> >>>>> That didn't occur to me previously, but it does make sense now that you >>>>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we >>>>> can avoid possible regressions for folks who might start to rely on >>>>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this. >>>>> >>>>> Cheers, >>>>> Peter >>>>> >>>>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001 >>>>> From: Peter Rosin >>>>> Cc: Bartlomiej Zolnierkiewicz >>>>> Cc: Jonathan Corbet >>>>> Cc: Peter Rosin >>>>> Cc: dri-devel@lists.freedesktop.org >>>>> Cc: linux-fb...@vger.kernel.org >>>>> Cc: linux-...@vger.kernel.org >>>>> To: linux-ker...@vger.kernel.org >>>>> Date: Mon, 7 Jan 2019 08:35:26 +0100 >>>>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd >>>>> line >>>>> option >>>>> >>>>> A command line option is much more flexible than a config option and >>>>> the supporting code is small. Gets rid of #ifdefs in the code too... >>>>> >>>>> Suggested-by: Geert Uytterhoeven >>>>> Signed-off-by: Peter Rosin >>>> >>>> Thanks for your patch! >>>> >>>>> --- a/Documentation/fb/fbcon.txt >>>>> +++ b/Documentation/fb/fbcon.txt >>>>> @@ -163,6 +163,12 @@ C. Boot options >>>>> be preserved until there actually is some text is output to the >>>>> console. >>>>> This option causes fbcon to bind immediately to the fbdev device. >>>>> >>>>> +7. fbcon=center-logo >>>>> + >>>>> + When this option is selected, the bootup logo is centered both >>>>> + horizontally and vertically. If more than one logo is displayed >>>>> due to >>>>> + multiple CPUs, the collected line of logos is centered as a whole. >>>>> + >>>> >>>> What about making this more generic, than an option specific for centering? >>>> Else people want fbcon=left-logo or fbcon=right-logo soon? >>>> >>>> fbcon=logo-pos: >>>> >>>> With being "center", "left", "top", "right", "bottom", and >>>> combinations >>>> like "top,center"? Or is that complicating stuff too much? >>> >>> I'm not too keen on implementing all that when I have zero use for it. I can >>> however rename the trigger for the existing fb_center_logo bool to >>> >>> fbcon=logo-pos:center >>> >>> and add
[PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line, option
On 2019-01-06 10:33, Geert Uytterhoeven wrote:
> Hi Peter,
>
> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin wrote:
>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
>> extra logos are not considered when centering the first logo vertically.
>>
>> Signed-off-by: Peter Rosin
>
>> --- a/drivers/video/logo/Kconfig
>> +++ b/drivers/video/logo/Kconfig
>> @@ -10,6 +10,15 @@ menuconfig LOGO
>>
>> if LOGO
>>
>> +config FB_LOGO_CENTER
>> + bool "Center the logo"
>> + depends on FB=y
>> + help
>> + When this option is selected, the bootup logo is centered both
>> + horizontally and vertically. If more than one logo is displayed
>> + due to multiple CPUs, the collected line of logos is centered
>> + as a whole.
>> +
>
> Isn't a kernel command line option more suitable to configure the position
> of the logo?
That didn't occur to me previously, but it does make sense now that you
mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
can avoid possible regressions for folks who might start to rely on
CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
Cheers,
Peter
From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
From: Peter Rosin
Cc: Bartlomiej Zolnierkiewicz
Cc: Jonathan Corbet
Cc: Peter Rosin
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-...@vger.kernel.org
To: linux-ker...@vger.kernel.org
Date: Mon, 7 Jan 2019 08:35:26 +0100
Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
option
A command line option is much more flexible than a config option and
the supporting code is small. Gets rid of #ifdefs in the code too...
Suggested-by: Geert Uytterhoeven
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.txt | 6 ++
drivers/video/fbdev/core/fbcon.c | 5 +
drivers/video/fbdev/core/fbmem.c | 19 ++-
drivers/video/logo/Kconfig | 9 -
include/linux/fb.h | 1 +
5 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/Documentation/fb/fbcon.txt b/Documentation/fb/fbcon.txt
index 62af30511a95..fb5fa0a8d553 100644
--- a/Documentation/fb/fbcon.txt
+++ b/Documentation/fb/fbcon.txt
@@ -163,6 +163,12 @@ C. Boot options
be preserved until there actually is some text is output to the console.
This option causes fbcon to bind immediately to the fbdev device.
+7. fbcon=center-logo
+
+ When this option is selected, the bootup logo is centered both
+ horizontally and vertically. If more than one logo is displayed due to
+ multiple CPUs, the collected line of logos is centered as a whole.
+
C. Attaching, Detaching and Unloading
Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 8976190b6c1f..552cfee63d76 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -510,6 +510,11 @@ static int __init fb_console_setup(char *this_opt)
continue;
}
#endif
+
+ if (!strcmp(options, "center-logo")) {
+ fb_center_logo = true;
+ continue;
+ }
}
return 1;
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 558ed2ed3124..cb43a2258c51 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -53,6 +53,9 @@ EXPORT_SYMBOL(registered_fb);
int num_registered_fb __read_mostly;
EXPORT_SYMBOL(num_registered_fb);
+bool fb_center_logo __read_mostly;
+EXPORT_SYMBOL(fb_center_logo);
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -506,8 +509,7 @@ static int fb_show_logo_line(struct fb_info *info, int
rotate,
fb_set_logo(info, logo, logo_new, fb_logo.depth);
}
-#ifdef CONFIG_FB_LOGO_CENTER
- {
+ if (fb_center_logo) {
int xres = info->var.xres;
int yres = info->var.yres;
@@ -520,11 +522,11 @@ static int fb_show_logo_line(struct fb_info *info, int
rotate,
--n;
image.dx = (xres - n * (logo->width + 8) - 8) / 2;
image.dy = y ?: (yres - logo->height) / 2;
+ } else {
+ image.dx = 0;
+ image.dy = y;
}
-#else
- image.dx = 0;
- image.dy = y;
-#endif
+
image.width = logo->width;
image.height = logo->height;
@@ -684,9 +686,8 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
}
height = fb_logo.logo->height;
-#ifdef CONFIG_FB_LOGO_CENTER
- height += (yres - fb_logo.logo->height) / 2;
-#endif
Re: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line, option
On 2019-01-07 09:40, Geert Uytterhoeven wrote: > Hi Peter, > > On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin wrote: >> On 2019-01-06 10:33, Geert Uytterhoeven wrote: >>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin wrote: >>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these >>>> extra logos are not considered when centering the first logo vertically. >>>> >>>> Signed-off-by: Peter Rosin >>> >>>> --- a/drivers/video/logo/Kconfig >>>> +++ b/drivers/video/logo/Kconfig >>>> @@ -10,6 +10,15 @@ menuconfig LOGO >>>> >>>> if LOGO >>>> >>>> +config FB_LOGO_CENTER >>>> + bool "Center the logo" >>>> + depends on FB=y >>>> + help >>>> + When this option is selected, the bootup logo is centered both >>>> + horizontally and vertically. If more than one logo is displayed >>>> + due to multiple CPUs, the collected line of logos is centered >>>> + as a whole. >>>> + >>> >>> Isn't a kernel command line option more suitable to configure the position >>> of the logo? >> >> That didn't occur to me previously, but it does make sense now that you >> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we >> can avoid possible regressions for folks who might start to rely on >> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this. >> >> Cheers, >> Peter >> >> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001 >> From: Peter Rosin >> Cc: Bartlomiej Zolnierkiewicz >> Cc: Jonathan Corbet >> Cc: Peter Rosin >> Cc: dri-devel@lists.freedesktop.org >> Cc: linux-fb...@vger.kernel.org >> Cc: linux-...@vger.kernel.org >> To: linux-ker...@vger.kernel.org >> Date: Mon, 7 Jan 2019 08:35:26 +0100 >> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line >> option >> >> A command line option is much more flexible than a config option and >> the supporting code is small. Gets rid of #ifdefs in the code too... >> >> Suggested-by: Geert Uytterhoeven >> Signed-off-by: Peter Rosin > > Thanks for your patch! > >> --- a/Documentation/fb/fbcon.txt >> +++ b/Documentation/fb/fbcon.txt >> @@ -163,6 +163,12 @@ C. Boot options >> be preserved until there actually is some text is output to the >> console. >> This option causes fbcon to bind immediately to the fbdev device. >> >> +7. fbcon=center-logo >> + >> + When this option is selected, the bootup logo is centered both >> + horizontally and vertically. If more than one logo is displayed due >> to >> + multiple CPUs, the collected line of logos is centered as a whole. >> + > > What about making this more generic, than an option specific for centering? > Else people want fbcon=left-logo or fbcon=right-logo soon? > > fbcon=logo-pos: > > With being "center", "left", "top", "right", "bottom", and combinations > like "top,center"? Or is that complicating stuff too much? I'm not too keen on implementing all that when I have zero use for it. I can however rename the trigger for the existing fb_center_logo bool to fbcon=logo-pos:center and add a check for "top,left" to reset the bool to false. Then the other variants can be added later by whomever and when needed. Is that good enough? Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/5] drm/bridge: various small lvds-encoder things
Hi! I'm not sure if I should have added the texas chips to the lvds_encoder_match list in the driver, right next to the thine,thc63lvdm83d entry, but ended up not doing that. That can always be added later, if needed... Changes since v2: - changed from pwdn-gpios to powerdown-gpios after discussion with Rob with new patch 3/5 updating the thine,thc63lvdm83d binding as well as a consequence - added patch 4/5 which helps keep lines shorter in the lvds-encoder driver - added tag from Rob on patch 2/5 Changes since v1: - fork out the bindings for the texas chips into their own file in order to avoid clutter in the generic lvds-transmitter binding. - added a patch to remove some surplus stuff in the generic lvds-transmitter binding. Cheers, Peter Peter Rosin (5): dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter dt-bindings: display: bridge: lvds-transmitter: cleanup example dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios drm/bridge: lvds-encoder: add dev helper variable in .probe() drm/bridge: lvds-encoder: add powerdown-gpios support .../bindings/display/bridge/lvds-transmitter.txt | 10 +--- .../bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +- .../bindings/display/bridge/ti,ds90c185.txt| 55 ++ drivers/gpu/drm/bridge/lvds-encoder.c | 53 + 4 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/5] dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios
The name powerdown-gpios is the standard property name for the functionality covered by the previous pwdn-gpios name. This rename should be safe to do since the linux driver supporting the binding (lvds-encoder.c) never implemented the property, and no dts file names it. At least not upstream. Signed-off-by: Peter Rosin --- Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt index 527e236e9a2a..fee3c88e1a17 100644 --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt @@ -10,7 +10,7 @@ Required properties: Optional properties: -- pwdn-gpios: Power down control GPIO +- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low). Required nodes: -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/5] dt-bindings: display: bridge: lvds-transmitter: cleanup example
Drop #address-cells and #size-cells from the root node in the
example, they are unused.
Reviewed-by: Rob Herring
Signed-off-by: Peter Rosin
---
Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 2 --
1 file changed, 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..36ce07c8d8e6 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -38,8 +38,6 @@ Example
lvds-encoder {
compatible = "lvds-encoder";
- #address-cells = <1>;
- #size-cells = <0>;
ports {
#address-cells = <1>;
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/5] dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter
DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
binding, which is meant to be generic.
The sister chip DS90C187 is similar to DS90C185, describe it here as well.
Signed-off-by: Peter Rosin
---
.../bindings/display/bridge/lvds-transmitter.txt | 8 +---
.../bindings/display/bridge/ti,ds90c185.txt| 55 ++
2 files changed, 56 insertions(+), 7 deletions(-)
create mode 100644
Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..fd39ad34c383 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,13 +22,7 @@ among others.
Required properties:
-- compatible: Must be one or more of the following
- - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
- - "lvds-encoder" for a generic LVDS encoder device
-
- When compatible with the generic version, nodes must list the
- device-specific version corresponding to the device first
- followed by the generic version.
+- compatible: Must be "lvds-encoder"
Required nodes:
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
new file mode 100644
index ..e575f996959a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
@@ -0,0 +1,55 @@
+Texas Instruments FPD-Link (LVDS) Serializer
+
+
+The DS90C185 and DS90C187 are low-power serializers for portable
+battery-powered applications that reduces the size of the RGB
+interface between the host GPU and the display.
+
+Required properties:
+
+- compatible: Should be
+ "ti,ds90c185", "lvds-encoder" for the TI DS90C185 FPD-Link Serializer
+ "ti,ds90c187", "lvds-encoder" for the TI DS90C187 FPD-Link Serializer
+
+Optional properties:
+
+- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
+
+Required nodes:
+
+The devices have two video ports. Their connections are modeled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for parallel input
+- Video port 1 for LVDS output
+
+
+Example
+---
+
+lvds-encoder {
+ compatible = "ti,ds90c185", "lvds-encoder";
+
+ powerdown-gpios = < 17 GPIO_ACTIVE_LOW>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lvds_enc_in: endpoint {
+ remote-endpoint = <_out_rgb>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ lvds_enc_out: endpoint {
+ remote-endpoint = <_panel_in>;
+ };
+ };
+ };
+};
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 4/5] drm/bridge: lvds-encoder: add dev helper variable in .probe()
Make the code easier to read and modify.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..f6770e83d49d 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -34,48 +34,47 @@ static struct drm_bridge_funcs funcs = {
static int lvds_encoder_probe(struct platform_device *pdev)
{
+ struct device *dev = >dev;
struct device_node *port;
struct device_node *endpoint;
struct device_node *panel_node;
struct drm_panel *panel;
struct lvds_encoder *lvds_encoder;
- lvds_encoder = devm_kzalloc(>dev, sizeof(*lvds_encoder),
- GFP_KERNEL);
+ lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
if (!lvds_encoder)
return -ENOMEM;
/* Locate the panel DT node. */
- port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
+ port = of_graph_get_port_by_id(dev->of_node, 1);
if (!port) {
- dev_dbg(>dev, "port 1 not found\n");
+ dev_dbg(dev, "port 1 not found\n");
return -ENXIO;
}
endpoint = of_get_child_by_name(port, "endpoint");
of_node_put(port);
if (!endpoint) {
- dev_dbg(>dev, "no endpoint for port 1\n");
+ dev_dbg(dev, "no endpoint for port 1\n");
return -ENXIO;
}
panel_node = of_graph_get_remote_port_parent(endpoint);
of_node_put(endpoint);
if (!panel_node) {
- dev_dbg(>dev, "no remote endpoint for port 1\n");
+ dev_dbg(dev, "no remote endpoint for port 1\n");
return -ENXIO;
}
panel = of_drm_find_panel(panel_node);
of_node_put(panel_node);
if (!panel) {
- dev_dbg(>dev, "panel not found, deferring probe\n");
+ dev_dbg(dev, "panel not found, deferring probe\n");
return -EPROBE_DEFER;
}
lvds_encoder->panel_bridge =
- devm_drm_panel_bridge_add(>dev,
- panel, DRM_MODE_CONNECTOR_LVDS);
+ devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_LVDS);
if (IS_ERR(lvds_encoder->panel_bridge))
return PTR_ERR(lvds_encoder->panel_bridge);
@@ -83,7 +82,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
* but we need a bridge attached to our of_node for our user
* to look up.
*/
- lvds_encoder->bridge.of_node = pdev->dev.of_node;
+ lvds_encoder->bridge.of_node = dev->of_node;
lvds_encoder->bridge.funcs =
drm_bridge_add(_encoder->bridge);
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 5/5] drm/bridge: lvds-encoder: add powerdown-gpios support
Optionally power down the LVDS-encoder when it is not in use.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index f6770e83d49d..36d8557bc097 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -11,11 +11,13 @@
#include
#include
+#include
#include
struct lvds_encoder {
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct gpio_desc *powerdown_gpio;
};
static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +30,30 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
bridge);
}
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
+}
+
static struct drm_bridge_funcs funcs = {
.attach = lvds_encoder_attach,
+ .enable = lvds_encoder_enable,
+ .disable = lvds_encoder_disable,
};
static int lvds_encoder_probe(struct platform_device *pdev)
@@ -45,6 +69,16 @@ static int lvds_encoder_probe(struct platform_device *pdev)
if (!lvds_encoder)
return -ENOMEM;
+ lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(lvds_encoder->powerdown_gpio)) {
+ int err = PTR_ERR(lvds_encoder->powerdown_gpio);
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "powerdown GPIO failure: %d\n", err);
+ return err;
+ }
+
/* Locate the panel DT node. */
port = of_graph_get_port_by_id(dev->of_node, 1);
if (!port) {
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/3] dt-bindings: display: bridge: fork out ti, ds90c185 from lvds-transmitter
On 2018-12-27 22:27, Rob Herring wrote:
> On Wed, Dec 19, 2018 at 02:04:47PM +0100, Peter Rosin wrote:
>> From: Peter Rosin
>>
>> DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
>> binding, which is meant to be generic.
>>
>> The sister chip DS90C187 is similar to DS90C185, describe it here as well.
>>
>> Signed-off-by: Peter Rosin
>> ---
>> .../bindings/display/bridge/lvds-transmitter.txt | 8 +---
>> .../bindings/display/bridge/ti,ds90c185.txt| 55
>> ++
>> 2 files changed, 56 insertions(+), 7 deletions(-)
>> create mode 100644
>> Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> index 50220190c203..fd39ad34c383 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> @@ -22,13 +22,7 @@ among others.
>>
>> Required properties:
>>
>> -- compatible: Must be one or more of the following
>> - - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
>> - - "lvds-encoder" for a generic LVDS encoder device
>> -
>> - When compatible with the generic version, nodes must list the
>> - device-specific version corresponding to the device first
>> - followed by the generic version.
>> +- compatible: Must be "lvds-encoder"
>>
>> Required nodes:
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>> b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>> new file mode 100644
>> index ..a13e778503e6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>> @@ -0,0 +1,55 @@
>> +Texas Instruments FPD-Link (LVDS) Serializer
>> +
>> +
>> +The DS90C185 and DS90C187 are low-power serializers for portable
>> +battery-powered applications that reduces the size of the RGB
>> +interface between the host GPU and the display.
>> +
>> +Required properties:
>> +
>> +- compatible: Should be
>> + "ti,ds90c185", "lvds-encoder" for the TI DS90C185 FPD-Link Serializer
>> + "ti,ds90c187", "lvds-encoder" for the TI DS90C187 FPD-Link Serializer
>> +
>> +Optional properties:
>> +
>> +- pwdn-gpios: Power down control GPIO (the PDB pin, active-low)
>
> powerdown-gpios is the standard name.
The lvds-encoder driver handles this binding, and that driver incidentally
also implements the thine,thc63lvdm83d binding which already has a
pwdn-gpios property. Should the thine,thc63lvdm83d binding be updated and
the driver be made to support both properties?
Since the lvds-encoder driver never had support for the pwdn-gpios (at least
not upstream) I suppose there is also the option to simply go with
powerdown-gpios as you suggest and not bother with support for the
pwdn-gpios property.
I'm quite willing to send an updated series, but I don't know what is
preferred, and am in need of guidance.
Cheers,
Peter
>> +
>> +Required nodes:
>> +
>> +The devices have two video ports. Their connections are modeled using the OF
>> +graph bindings specified in Documentation/devicetree/bindings/graph.txt.
>> +
>> +- Video port 0 for parallel input
>> +- Video port 1 for LVDS output
>> +
>> +
>> +Example
>> +---
>> +
>> +lvds-encoder {
>> +compatible = "ti,ds90c185", "lvds-encoder";
>> +
>> +pwdn-gpios = < 17 GPIO_ACTIVE_LOW>;
>> +
>> +ports {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +port@0 {
>> +reg = <0>;
>> +
>> +lvds_enc_in: endpoint {
>> +remote-endpoint = <_out_rgb>;
>> +};
>> +};
>> +
>> +port@1 {
>> +reg = <1>;
>> +
>> +lvds_enc_out: endpoint {
>> +remote-endpoint = <_panel_in>;
>> +};
>> +};
>> +};
>> +};
>> --
>> 2.11.0
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] dt-bindings: display: bridge: lvds-transmitter: cleanup example
From: Peter Rosin
Drop #address-cells and #size-cells from the root node in the
example, they are unused.
Signed-off-by: Peter Rosin
---
Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 2 --
1 file changed, 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..36ce07c8d8e6 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -38,8 +38,6 @@ Example
lvds-encoder {
compatible = "lvds-encoder";
- #address-cells = <1>;
- #size-cells = <0>;
ports {
#address-cells = <1>;
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/3] drm/bridge: various small lvds-encoder things
From: Peter Rosin Hi! I'm not sure if I should have added the texas chips to the lvds_encoder_match list in the driver, right next to the thine,thc63lvdm83d entry, but ended up not doing that. That can always be added later, it needed... Changes since v1: - fork out the bindings for the texas chips into their own file in order to avoid clutter in the generic lvds-transmitter binding. - added a patch to remove some surplus stuff in the generic lvds-transmitter binding. Cheers, Peter Peter Rosin (3): dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter dt-bindings: display: bridge: lvds-transmitter: cleanup example drm/bridge: add pwdn-gpios support to the lvds-encoder .../bindings/display/bridge/lvds-transmitter.txt | 10 +--- .../bindings/display/bridge/ti,ds90c185.txt| 55 ++ drivers/gpu/drm/bridge/lvds-encoder.c | 34 + 3 files changed, 90 insertions(+), 9 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios
On 2018-12-19 12:38, Laurent Pinchart wrote: > Hello, > > On Wednesday, 19 December 2018 11:57:32 EET Peter Rosin wrote: >> On 2018-12-19 10:12, Andrzej Hajda wrote: >>> On 19.12.2018 00:19, Peter Rosin wrote: >>>> Add optional property to specify a power-down GPIO. >>>> The pwdn-gpios name is already in use by the thine,thc63lvdm83d >>>> binding, so go with that. >>>> >>>> Signed-off-by: Peter Rosin >>>> --- >>>> >>>> Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | >>>> 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >>>> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >>>> index f9e7dd666f58..47941d39f92f 100644 >>>> --- >>>> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >>>> +++ >>>> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >>>> @@ -31,6 +31,9 @@ Required properties: >>>>device-specific version corresponding to the device first >>>>followed by the generic version. >>>> >>>> +Optional properties: >>>> +- pwdn-gpios: Power-down control GPIO >>>> + >>> >>> Since naming is not enforced by any datasheet I would propose something >>> more popular with less twisted logic. Maybe: >>> >>> - enable-gpios: ... (active high). >> >> That was my original thought too, but the driver implementing the >> lvds-encoder bindings also handles the mentioned thine,thc63lvdm83d >> lvds encoder, and that binding has the "pwdn" naming. So, for driver >> implementation simplicity I went with what was already there, thus >> allowing adding support for both bindings with one implementation >> (in patch 3/3). >> >> Adding code just to handle multiple names for the same thing does >> not sounds too appealing. > > I'm afraid I think we shouldn't add pwdn-gpios support to the lvds encoder DT > bindings. The reason is that control GPIOs (and regulators) come with device- > specific semantics. If we add pwdn-gpios now, we'll then add reset-gpios, and > power supplies, and all of a sudden we'll end up having to encode sequencing > of GPIOs and power supplies in DT. That path has been tried in the past, with > no good results. > > I would instead create device-specific bindings, like done for > thine,thc63lvdm83d. It's fine to then add support for the pwdn-gpios property > in the lvds-encoder driver, as long as the meaning of the property comes from > specific DT bindings, not from the generic ones. Right, I'll fork out the bindings for the texas chips. v2 coming up. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios
On 2018-12-19 10:12, Andrzej Hajda wrote: > On 19.12.2018 00:19, Peter Rosin wrote: >> Add optional property to specify a power-down GPIO. >> The pwdn-gpios name is already in use by the thine,thc63lvdm83d >> binding, so go with that. >> >> Signed-off-by: Peter Rosin >> --- >> Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 3 >> +++ >> 1 file changed, 3 insertions(+) >> >> diff --git >> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >> index f9e7dd666f58..47941d39f92f 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt >> @@ -31,6 +31,9 @@ Required properties: >>device-specific version corresponding to the device first >>followed by the generic version. >> >> +Optional properties: >> +- pwdn-gpios: Power-down control GPIO >> + > > > Since naming is not enforced by any datasheet I would propose something > more popular with less twisted logic. Maybe: > > - enable-gpios: ... (active high). That was my original thought too, but the driver implementing the lvds-encoder bindings also handles the mentioned thine,thc63lvdm83d lvds encoder, and that binding has the "pwdn" naming. So, for driver implementation simplicity I went with what was already there, thus allowing adding support for both bindings with one implementation (in patch 3/3). Adding code just to handle multiple names for the same thing does not sounds too appealing. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/3] drm/bridge: add pwdn-gpios support to the lvds-encoder
From: Peter Rosin
Optionally power down the LVDS-encoder when it is not in use.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..43d584a6a6b9 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -11,11 +11,13 @@
#include
#include
+#include
#include
struct lvds_encoder {
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct gpio_desc *pwdn_gpio;
};
static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +30,30 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
bridge);
}
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->pwdn_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 0);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->pwdn_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 1);
+}
+
static struct drm_bridge_funcs funcs = {
.attach = lvds_encoder_attach,
+ .enable = lvds_encoder_enable,
+ .disable = lvds_encoder_disable,
};
static int lvds_encoder_probe(struct platform_device *pdev)
@@ -45,6 +69,16 @@ static int lvds_encoder_probe(struct platform_device *pdev)
if (!lvds_encoder)
return -ENOMEM;
+ lvds_encoder->pwdn_gpio = devm_gpiod_get_optional(>dev, "pwdn",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(lvds_encoder->pwdn_gpio)) {
+ int err = PTR_ERR(lvds_encoder->pwdn_gpio);
+
+ if (err != -EPROBE_DEFER)
+ dev_err(>dev, "pwdn GPIO failure: %d\n", err);
+ return err;
+ }
+
/* Locate the panel DT node. */
port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
if (!port) {
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] dt-bindings: display: bridge: fork out ti, ds90c185 from lvds-transmitter
From: Peter Rosin
DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
binding, which is meant to be generic.
The sister chip DS90C187 is similar to DS90C185, describe it here as well.
Signed-off-by: Peter Rosin
---
.../bindings/display/bridge/lvds-transmitter.txt | 8 +---
.../bindings/display/bridge/ti,ds90c185.txt| 55 ++
2 files changed, 56 insertions(+), 7 deletions(-)
create mode 100644
Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
diff --git
a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..fd39ad34c383 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,13 +22,7 @@ among others.
Required properties:
-- compatible: Must be one or more of the following
- - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
- - "lvds-encoder" for a generic LVDS encoder device
-
- When compatible with the generic version, nodes must list the
- device-specific version corresponding to the device first
- followed by the generic version.
+- compatible: Must be "lvds-encoder"
Required nodes:
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
new file mode 100644
index ..a13e778503e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
@@ -0,0 +1,55 @@
+Texas Instruments FPD-Link (LVDS) Serializer
+
+
+The DS90C185 and DS90C187 are low-power serializers for portable
+battery-powered applications that reduces the size of the RGB
+interface between the host GPU and the display.
+
+Required properties:
+
+- compatible: Should be
+ "ti,ds90c185", "lvds-encoder" for the TI DS90C185 FPD-Link Serializer
+ "ti,ds90c187", "lvds-encoder" for the TI DS90C187 FPD-Link Serializer
+
+Optional properties:
+
+- pwdn-gpios: Power down control GPIO (the PDB pin, active-low)
+
+Required nodes:
+
+The devices have two video ports. Their connections are modeled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for parallel input
+- Video port 1 for LVDS output
+
+
+Example
+---
+
+lvds-encoder {
+ compatible = "ti,ds90c185", "lvds-encoder";
+
+ pwdn-gpios = < 17 GPIO_ACTIVE_LOW>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lvds_enc_in: endpoint {
+ remote-endpoint = <_out_rgb>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ lvds_enc_out: endpoint {
+ remote-endpoint = <_panel_in>;
+ };
+ };
+ };
+};
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c187
This chip is compatible with "lvds-encoder". Signed-off-by: Peter Rosin --- Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt index 50220190c203..f9e7dd666f58 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt @@ -24,6 +24,7 @@ Required properties: - compatible: Must be one or more of the following - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer + - "ti,ds90c187" for the TI DS90C187 Dual Pixel FPD-Link Serializer - "lvds-encoder" for a generic LVDS encoder device When compatible with the generic version, nodes must list the -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/bridge: add pwdn-gpios support to the lvds-encoder
Optionally power down the LVDS-encoder when it is not in use.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c
b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..43d584a6a6b9 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -11,11 +11,13 @@
#include
#include
+#include
#include
struct lvds_encoder {
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct gpio_desc *pwdn_gpio;
};
static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +30,30 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
bridge);
}
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->pwdn_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 0);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+ struct lvds_encoder *lvds_encoder = container_of(bridge,
+struct lvds_encoder,
+bridge);
+
+ if (lvds_encoder->pwdn_gpio)
+ gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 1);
+}
+
static struct drm_bridge_funcs funcs = {
.attach = lvds_encoder_attach,
+ .enable = lvds_encoder_enable,
+ .disable = lvds_encoder_disable,
};
static int lvds_encoder_probe(struct platform_device *pdev)
@@ -45,6 +69,16 @@ static int lvds_encoder_probe(struct platform_device *pdev)
if (!lvds_encoder)
return -ENOMEM;
+ lvds_encoder->pwdn_gpio = devm_gpiod_get_optional(>dev, "pwdn",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(lvds_encoder->pwdn_gpio)) {
+ int err = PTR_ERR(lvds_encoder->pwdn_gpio);
+
+ if (err != -EPROBE_DEFER)
+ dev_err(>dev, "pwdn GPIO failure: %d\n", err);
+ return err;
+ }
+
/* Locate the panel DT node. */
port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
if (!port) {
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios
Add optional property to specify a power-down GPIO. The pwdn-gpios name is already in use by the thine,thc63lvdm83d binding, so go with that. Signed-off-by: Peter Rosin --- Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt index f9e7dd666f58..47941d39f92f 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt @@ -31,6 +31,9 @@ Required properties: device-specific version corresponding to the device first followed by the generic version. +Optional properties: +- pwdn-gpios: Power-down control GPIO + Required nodes: This device has two video ports. Their connections are modeled using the OF -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] drm/bridge: various small lvds-encoder things
Hi! These patches are useful for me. Please consider merging. Cheers, Peter Peter Rosin (3): dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c187 dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios drm/bridge: add pwdn-gpios support to the lvds-encoder .../bindings/display/bridge/lvds-transmitter.txt | 4 +++ drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++ 2 files changed, 38 insertions(+) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION PATCH] fbdev: fbmem: behave better with small rotated displays and many CPUs
Ping?!
Cheers,
Peter
On 2018-11-08 10:54, Peter Rosin wrote:
> Blitting an image with "negative" offsets is not working since there
> is no clipping. It hopefully just crashes. For the bootup logo, there
> is protection so that blitting does not happen as the image is drawn
> further and further to the right (ROTATE_UR) or further and further
> down (ROTATE_CW). There is however no protection when drawing in the
> opposite directions (ROTATE_UD and ROTATE_CCW).
>
> Add back this protection.
>
> The regression is 20-odd years old but the mindless warning-killing
> mentality displayed in commit 34bdb666f4b2 ("fbdev: fbmem: remove
> positive test on unsigned values") is also to blame, methinks.
>
> Fixes: 448d479747b8 ("fbdev: fb_do_show_logo() updates")
> Signed-off-by: Peter Rosin
> ---
> drivers/video/fbdev/core/fbmem.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index bb7f5f23e347..1abeb0b72455 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -435,7 +435,9 @@ static void fb_do_show_logo(struct fb_info *info, struct
> fb_image *image,
> image->dx += image->width + 8;
> }
> } else if (rotate == FB_ROTATE_UD) {
> - for (x = 0; x < num; x++) {
> + u32 dx = image->dx;
> +
> + for (x = 0; x < num && image->dx <= dx; x++) {
> info->fbops->fb_imageblit(info, image);
> image->dx -= image->width + 8;
> }
> @@ -447,7 +449,9 @@ static void fb_do_show_logo(struct fb_info *info, struct
> fb_image *image,
> image->dy += image->height + 8;
> }
> } else if (rotate == FB_ROTATE_CCW) {
> - for (x = 0; x < num; x++) {
> + u32 dy = image->dy;
> +
> + for (x = 0; x < num && image->dy <= dy; x++) {
> info->fbops->fb_imageblit(info, image);
> image->dy -= image->height + 8;
> }
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] fbdev: fbmem: make fb_show_logo_line return the end instead of the height
In preparation for allowing centering of the bootup logo, make fb_show_logo_line return where the next free framebuffer line is, instead of returning the height of the shown logo. Signed-off-by: Peter Rosin --- drivers/video/fbdev/core/fbmem.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 861bf8081619..43d4047f472a 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -521,7 +521,7 @@ static int fb_show_logo_line(struct fb_info *info, int rotate, info->pseudo_palette = saved_pseudo_palette; kfree(logo_new); kfree(logo_rotate); - return logo->height; + return image.dy + logo->height; } @@ -573,8 +573,8 @@ static int fb_show_extra_logos(struct fb_info *info, int y, int rotate) unsigned int i; for (i = 0; i < fb_logo_ex_num; i++) - y += fb_show_logo_line(info, rotate, - fb_logo_ex[i].logo, y, fb_logo_ex[i].n); + y = fb_show_logo_line(info, rotate, + fb_logo_ex[i].logo, y, fb_logo_ex[i].n); return y; } -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION PATCH] fbdev: fbmem: behave better with small rotated displays and many CPUs
On 2018-11-26 15:33, Bartlomiej Zolnierkiewicz wrote: > On 11/26/2018 03:16 PM, Peter Rosin wrote: >> Ping?! > > Hi, > > Thank you for your patch, it will be considered for 4.21 (as it is not > a recent regression + I'm busy with other things currently). Right, thanks for letting me know! Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] fbdev: fbmem: add config option to center the bootup logo
If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
extra logos are not considered when centering the first logo vertically.
Signed-off-by: Peter Rosin
---
drivers/video/fbdev/core/fbmem.c | 25 -
drivers/video/logo/Kconfig | 9 +
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 43d4047f472a..fdbad029e5e6 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -502,8 +502,25 @@ static int fb_show_logo_line(struct fb_info *info, int
rotate,
fb_set_logo(info, logo, logo_new, fb_logo.depth);
}
+#ifdef CONFIG_FB_LOGO_CENTER
+ {
+ int xres = info->var.xres;
+ int yres = info->var.yres;
+
+ if (rotate == FB_ROTATE_CW || rotate == FB_ROTATE_CCW) {
+ xres = info->var.yres;
+ yres = info->var.xres;
+ }
+
+ while (n && (n * (logo->width + 8) - 8 > xres))
+ --n;
+ image.dx = (xres - n * (logo->width + 8) - 8) / 2;
+ image.dy = y ?: (yres - logo->height) / 2;
+ }
+#else
image.dx = 0;
image.dy = y;
+#endif
image.width = logo->width;
image.height = logo->height;
@@ -600,6 +617,7 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
{
int depth = fb_get_color_depth(>var, >fix);
unsigned int yres;
+ int height;
memset(_logo, 0, sizeof(struct logo_data));
@@ -661,7 +679,12 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
}
}
- return fb_prepare_extra_logos(info, fb_logo.logo->height, yres);
+ height = fb_logo.logo->height;
+#ifdef CONFIG_FB_LOGO_CENTER
+ height += (yres - fb_logo.logo->height) / 2;
+#endif
+
+ return fb_prepare_extra_logos(info, height, yres);
}
int fb_show_logo(struct fb_info *info, int rotate)
diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig
index d1f6196c8b9a..1e972c4e88b1 100644
--- a/drivers/video/logo/Kconfig
+++ b/drivers/video/logo/Kconfig
@@ -10,6 +10,15 @@ menuconfig LOGO
if LOGO
+config FB_LOGO_CENTER
+ bool "Center the logo"
+ depends on FB=y
+ help
+ When this option is selected, the bootup logo is centered both
+ horizontally and vertically. If more than one logo is displayed
+ due to multiple CPUs, the collected line of logos is centered
+ as a whole.
+
config FB_LOGO_EXTRA
bool
depends on FB=y
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] add config option to center the bootup logo
Hi! I'm using these patches to get early display output at the center of the screen. Maybe someone else will also find that useful? Cheers, Peter Peter Rosin (2): fbdev: fbmem: make fb_show_logo_line return the end instead of the height fbdev: fbmem: add config option to center the bootup logo drivers/video/fbdev/core/fbmem.c | 31 +++ drivers/video/logo/Kconfig | 9 + 2 files changed, 36 insertions(+), 4 deletions(-) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/bridge/sii902x: Add missing dependency on I2C_MUX
On 2018-11-19 14:26, Fabrizio Castro wrote:
> kbuild test robot reports:
>
>>> ERROR: "i2c_mux_add_adapter" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
>>> ERROR: "i2c_mux_alloc" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
>>> ERROR: "i2c_mux_del_adapters" [drivers/gpu/drm/bridge/sii902x.ko]
> undefined!
Hi!
My preference would be to not wrap the quoted text. To hell with
warnings about long lines for cases like this. However, I'm not
going to require a new iteration for that detail, but keep that
in mind for the next patch or if you end up resending for some
unrelated reason.
>
> Quite obviously the driver depends on I2C_MUX, but adding a "depends on"
> introduces a recursive dependency, therefore this patch selects I2C_MUX
> instead.
This driver will not be the first to select I2C_MUX. So, even if it
it is a bit untidy to select stuff that is also selectable by the
user...
Acked-by: Peter Rosin
>
> Fixes: 21d808405fe4 ("drm/bridge/sii902x: Fix EDID readback")
> Signed-off-by: Fabrizio Castro
> Link: https://lists.01.org/pipermail/kbuild-all/2018-November/054924.html
> ---
> v2->v3:
> * Changed the title
>
> v1->v2:
> * Added "Fixes" tag
>
> drivers/gpu/drm/bridge/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 9eeb8ef..2fee47b 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -95,6 +95,7 @@ config DRM_SII902X
> depends on OF
> select DRM_KMS_HELPER
> select REGMAP_I2C
> + select I2C_MUX
> ---help---
> Silicon Image sii902x bridge chip driver.
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/bridge/sii902x: Fix EDID readback
On 2018-11-06 12:52, Fabrizio Castro wrote: > While adding SiI9022A support to the iwg23s board, it came > up that when the HDMI transmitter is in pass through mode the > device is not compliant with the I2C specification anymore, > as it requires a far bigger tbuf, due to a delay the HDMI > transmitter is adding when relaying the STOP condition on the > monitor i2c side of things. > > When not providing an appropriate delay after the STOP condition > the i2c bus would get stuck. Also, any other traffic on the bus > while talking to the monitor may cause the transaction to fail > or even cause issues with the i2c bus as well. > > I2c-gates seemed to reach consent as a possible way to address > these issues, and as such this patch is implementing a solution > based on that. Since others are clearly relying on the current > implementation of the driver, this patch won't require any DT > changes. > > Since we don't want any interference during the DDC Bus > Request/Grant procedure and while talking to the monitor, we > have to use the adapter locking primitives rather than the > i2c-mux locking primitives. > > Signed-off-by: Fabrizio Castro Reviewed-by: Peter Rosin Cheers, Peter > > --- > v2->v3: > * Incorporated comments from Boris Brezillon and Peter Rosin > > v1->v2: > * Incorporated comments from Peter Rosin > > drivers/gpu/drm/bridge/sii902x.c | 247 > --- > 1 file changed, 178 insertions(+), 69 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sound and the TDA998x binding
Hi! I'm wondering about some programming details regarding the TDA998x driver... The bindings documentation [1] state that one should fill in the desired register content of the AP_ENA register. However, I cannot find any details anywhere about how one determines what is desired. When I look for data sheets on the Internet, all I find is a "short" data sheet, which is far from enlightening. There are hints about a "full" data sheet in the chapter on legal information of the "short" data sheet. Is the "full" data sheet protected by an NDA or something? That's the only reason I can find for it not being available... Maybe someone with such a "full" data sheet at hand can enlighten me on the workings of this AP_ENA register so that I know what it is that I need to fill in? Since it is so difficult to find this info, maybe it should be added to the binding? Pointers on how to obtain a copy of said "full" data sheet would be appreciated. The chip I have is TDA19988, if that is of any consequence... Cheers, Peter [1] Documentation/devicetree/bindings/display/bridge/tda998x.txt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Sound and the TDA998x binding
On 2018-11-13 20:09, Russell King - ARM Linux wrote: > On Tue, Nov 13, 2018 at 06:12:37PM +0000, Peter Rosin wrote: >> On 2018-11-13 18:24, Russell King - ARM Linux wrote: >>> On Tue, Nov 13, 2018 at 01:28:40PM +, Peter Rosin wrote: >>>> Hi! >>>> >>>> I'm wondering about some programming details regarding the TDA998x >>>> driver... >>>> >>>> The bindings documentation [1] state that one should fill in the >>>> desired register content of the AP_ENA register. However, I cannot >>>> find any details anywhere about how one determines what is desired. >>>> When I look for data sheets on the Internet, all I find is a "short" >>>> data sheet, which is far from enlightening. There are hints about >>>> a "full" data sheet in the chapter on legal information of the >>>> "short" data sheet. Is the "full" data sheet protected by an NDA >>>> or something? That's the only reason I can find for it not being >>>> available... >>>> >>>> Maybe someone with such a "full" data sheet at hand can enlighten >>>> me on the workings of this AP_ENA register so that I know what it is >>>> that I need to fill in? Since it is so difficult to find this info, >>>> maybe it should be added to the binding? >>> >>> There's various public documents for the TDA998x chips, some of which >>> do contain the register programming information - although we don't >>> have definitive information for every variant that the driver supports. >> >> I have looked, and not found anything. Do you have any pointer? For what >> chip(s) in the family are there register information? > > TDA9983B is the main one, but as I say, it doesn't document several > registers found in the TDA19988 - we have no definitive register > descriptions for this chip. That's something at least. I was hoping for some info on how to set things up for external 12MHz for the CEC chip, but it looks like I'm out of luck. I guess I'll have to experiment in order to maybe get that working... >>> Even so, that doesn't give us documentation for this register, so we >>> have to resort to code received from other sources. >>> >>> The AP_ENA register "audio port enable" is one bit per AP signal, from >>> the AP0 pin being bit 0, up to the AP7 pin being bit 7. >> >> Thanks! However, in the "short" sheet for the TDA19988 that I have, there >> is this table: >> >> AP0 WS (word select) >> AP1 S/PDIF input I2S-bus channel 0 >> AP2 S/PDIF input I2S-bus channel 1 >> AP3 I2S-bus channel 2 >> AP4 I2S-bus channel 3 >> ACLK SCK (I2S-bus clock) >> >> AP5 through AP7 are nowhere to be found... > > Right, so only bits 0 to 4 are meaningful. > >> Should I assume that ACLK is an alias for AP5, and that the register >> content should be 0x23 for I2S on channel 0? Or is ACLK not part of >> the AP_ENA register at all, so that 0x03 is more likely to be correct? > > I believe 0x03 as per the example binding in the tda998x document. > The example given is from Jean's work for the Dove Cubox which > uses a TDA19988, which has both I2S and S/PDIF wired to the > TDA19988 - I2S data on AP1, S/PDIF data on AP2. Ok, thanks! Next question. The example in the tda998x binding has a line #sound-dai-cells = <2>; with no further explanation. I think this is a bug, and that it should be <1>, but that's just a hunch (I haven't dug into the code). I base my hunch on the fact that the Cubox Audio example in the simple-card binding [1] has a couple of lines like this sound-dai = < 1>; i.e. with only one cell. One cell is also what I would expect, given how you can define more than one audio connection in the tda998x node and that 2 cells seems over the top for what is needed. However, arch/arm/boot/dts/am335x-boneblack-common.dtsi has #sound-dai-cells = <0>, and that's the only upstream mention of audio and tda998x in a real device tree context. So there is apparently some confusion. Maybe #sound-dai-cells = <0> works if the tda998x node only defines one audio input? Cheers, Peter [1] Documentation/devicetree/bindings/sound/simple-card.txt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Sound and the TDA998x binding
On 2018-11-13 18:24, Russell King - ARM Linux wrote: > On Tue, Nov 13, 2018 at 01:28:40PM +0000, Peter Rosin wrote: >> Hi! >> >> I'm wondering about some programming details regarding the TDA998x >> driver... >> >> The bindings documentation [1] state that one should fill in the >> desired register content of the AP_ENA register. However, I cannot >> find any details anywhere about how one determines what is desired. >> When I look for data sheets on the Internet, all I find is a "short" >> data sheet, which is far from enlightening. There are hints about >> a "full" data sheet in the chapter on legal information of the >> "short" data sheet. Is the "full" data sheet protected by an NDA >> or something? That's the only reason I can find for it not being >> available... >> >> Maybe someone with such a "full" data sheet at hand can enlighten >> me on the workings of this AP_ENA register so that I know what it is >> that I need to fill in? Since it is so difficult to find this info, >> maybe it should be added to the binding? > > There's various public documents for the TDA998x chips, some of which > do contain the register programming information - although we don't > have definitive information for every variant that the driver supports. I have looked, and not found anything. Do you have any pointer? For what chip(s) in the family are there register information? > Even so, that doesn't give us documentation for this register, so we > have to resort to code received from other sources. > > The AP_ENA register "audio port enable" is one bit per AP signal, from > the AP0 pin being bit 0, up to the AP7 pin being bit 7. Thanks! However, in the "short" sheet for the TDA19988 that I have, there is this table: AP0 WS (word select) AP1 S/PDIF input I2S-bus channel 0 AP2 S/PDIF input I2S-bus channel 1 AP3 I2S-bus channel 2 AP4 I2S-bus channel 3 ACLK SCK (I2S-bus clock) AP5 through AP7 are nowhere to be found... Should I assume that ACLK is an alias for AP5, and that the register content should be 0x23 for I2S on channel 0? Or is ACLK not part of the AP_ENA register at all, so that 0x03 is more likely to be correct? Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/7] tda998x: allow use with bridge based devices
On 2018-08-02 08:06, Peter Rosin wrote: > On 2018-08-01 11:35, Russell King - ARM Linux wrote: >> On Wed, Aug 01, 2018 at 11:01:12AM +0200, Peter Rosin wrote: >>> I don't think it's a problem with the atmel I2C driver. IIRC, the >>> tda998x driver issues the command a initiate the EDID read, but that >>> times out. So it appears to be the TDA19988 that fails to read the >>> EDID over the DDC bus? Which brings me to the double problem with the >>> scopes mentioned above... >> >> It sounds like it. >> >> It may be helpful to know that there are HDMI pass-through boards >> available that give access to all the HDMI signals: >> >> https://elabbay.myshopify.com/collections/camera >> https://elabbay.myshopify.com/collections/camera/products/hdmi-af-af-v1a-hdmi-type-a-female-to-hdmi-type-a-male-pass-through-adapter-breakout-board >> >> I've never bought from them, so please don't take this as a >> recommendation - the fact that there seems to be no company details >> on their site doesn't seem good, and as the whois for elabbay.com is >> obscured also doesn't give me any confidence to buy from them. > > I still will not be able to inspect the DDC bus between the TDA19988 > and the buffer circuit (IP4786), but the gadget seems useful enough and > it's not a shitload of money. We'll see how long it takes for it to get > here... > > Thanks for the pointer! Maybe :-) I got the pass-through board a while back, and that board works as expected and there was no problem with ordering etc. What I could see with that was that the TDA19988 was able to initiate a start condition (SDA -> low) but then nothing more happened. Then last week, someone noticed that even though the TDA19988 is driven by 1.8V, it still needs the high signals of the DDC bus to be above 3V, which was unexpected and not catered for by the design. Changing VCC(SYS) of the buffer circuit in place (IP4786, pin 27) to 3.3V fixed the issue and EDID reading works, and this was confirmed earlier today. So, the problem was that the TDA19988 only ever saw "low" DDC signals, and probably aborted when the bus appeared busy. Or something. If anyone cares... Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/bridge/sii902x: Fix EDID readback
On 2018-11-06 12:52, Fabrizio Castro wrote: > While adding SiI9022A support to the iwg23s board, it came > up that when the HDMI transmitter is in pass through mode the > device is not compliant with the I2C specification anymore, > as it requires a far bigger tbuf, due to a delay the HDMI > transmitter is adding when relaying the STOP condition on the > monitor i2c side of things. > > When not providing an appropriate delay after the STOP condition > the i2c bus would get stuck. Also, any other traffic on the bus > while talking to the monitor may cause the transaction to fail > or even cause issues with the i2c bus as well. > > I2c-gates seemed to reach consent as a possible way to address > these issues, and as such this patch is implementing a solution > based on that. Since others are clearly relying on the current > implementation of the driver, this patch won't require any DT > changes. > > Since we don't want any interference during the DDC Bus > Request/Grant procedure and while talking to the monitor, we > have to use the adapter locking primitives rather than the > i2c-mux locking primitives. > > Signed-off-by: Fabrizio Castro Reviewed-by: Peter Rosin ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[REGRESSION PATCH] fbdev: fbmem: behave better with small rotated displays and many CPUs
Blitting an image with "negative" offsets is not working since there
is no clipping. It hopefully just crashes. For the bootup logo, there
is protection so that blitting does not happen as the image is drawn
further and further to the right (ROTATE_UR) or further and further
down (ROTATE_CW). There is however no protection when drawing in the
opposite directions (ROTATE_UD and ROTATE_CCW).
Add back this protection.
The regression is 20-odd years old but the mindless warning-killing
mentality displayed in commit 34bdb666f4b2 ("fbdev: fbmem: remove
positive test on unsigned values") is also to blame, methinks.
Fixes: 448d479747b8 ("fbdev: fb_do_show_logo() updates")
Signed-off-by: Peter Rosin
---
drivers/video/fbdev/core/fbmem.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bb7f5f23e347..1abeb0b72455 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -435,7 +435,9 @@ static void fb_do_show_logo(struct fb_info *info, struct
fb_image *image,
image->dx += image->width + 8;
}
} else if (rotate == FB_ROTATE_UD) {
- for (x = 0; x < num; x++) {
+ u32 dx = image->dx;
+
+ for (x = 0; x < num && image->dx <= dx; x++) {
info->fbops->fb_imageblit(info, image);
image->dx -= image->width + 8;
}
@@ -447,7 +449,9 @@ static void fb_do_show_logo(struct fb_info *info, struct
fb_image *image,
image->dy += image->height + 8;
}
} else if (rotate == FB_ROTATE_CCW) {
- for (x = 0; x < num; x++) {
+ u32 dy = image->dy;
+
+ for (x = 0; x < num && image->dy <= dy; x++) {
info->fbops->fb_imageblit(info, image);
image->dy -= image->height + 8;
}
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/bridge/sii902x: Fix EDID readback
Hi!
A handful of nitpicks inline...
On 2018-11-05 14:56, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
>
> ---
> v1->v2:
> * Incorporated comments from Peter Rosin
>
> drivers/gpu/drm/bridge/sii902x.c | 256
> ---
> 1 file changed, 188 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..dd4318b 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,49 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + union i2c_smbus_data data;
> + int ret;
> +
> + ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> +I2C_SMBUS_READ, reg, I2C_SMBUS_BYTE_DATA, );
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = data.byte;
> + return 0;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + union i2c_smbus_data data;
> +
> + data.byte = val;
> +
> + return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> + I2C_SMBUS_WRITE, reg, I2C_SMBUS_BYTE_DATA,
> + );
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, );
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> {
> return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +179,11 @@ static const struct drm_connector_funcs
> sii902x_connector_funcs = {
> static int sii902x_get_modes(struct drm_connector *connector)
> {
> struct sii902x *sii902x = connector_to_sii902x(connector);
> - struct regmap *regmap = sii902x->regmap;
> u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> - struct device *dev = >i2c->dev;
> - unsigned long timeout;
> - unsigned int retries;
> - unsigned int status;
> struct edid *edid;
> - int num = 0;
> - int ret;
> -
> - ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> - SII902X_SYS_CTRL_DDC_BUS_REQ,
> - SII902X_SYS_CTRL_DDC_BUS_REQ);
> - if (ret)
> - return ret;
> -
> - timeout = jiffies +
> - msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> - do {
> - ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, );
> - if (ret)
> - return ret;
> - } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> - time_before(jiffies, timeout));
> -
> - if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> - dev_err
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
>
> Thank you guys
>
> drivers/gpu/drm/bridge/sii902x.c | 264
> +--
> 1 file changed, 196 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,68 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + struct i2c_msg xfer[] = {
> + {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = ,
> + }, {
> + .addr = i2c->addr,
> + .flags = I2C_M_RD,
> + .len = 1,
> + .buf = val,
> + }
> + };
> + unsigned char xfers = ARRAY_SIZE(xfer);
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> + if (ret < 0)
> + return ret;
> + } while (ret != xfers && --retries);
> + return ret == xfers ? 0 : -1;
New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)
union i2c_smbus_data data;
int ret;
ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
I2C_SMBUS_READ, reg,
I2C_SMBUS_BYTE_DATA, );
if (ret < 0)
return ret;
*val = data.byte;
return 0;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + u8 data[2] = {reg, val};
> + struct i2c_msg xfer = {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = sizeof(data),
> + .buf = data,
> + };
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, , 1);
> + if (ret < 0)
> + return ret;
> + } while (!ret && --retries);
> + return !ret ? -1 : 0;
union i2c_smbus_data data;
data.byte = val;
return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
I2C_SMBUS_WRITE, reg,
I2C_SMBUS_BYTE_DATA, );
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, );
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> {
> return container_of(bridge, struct s
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 17:16, Fabrizio Castro wrote: >>> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the >>> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is >>> also doing rmw accesses to that register in other parts of the driver. I >>> think you need to either add comment as to why that is safe (maybe other >>> things make it impossible for the two rmw accesses to cross?), or add the >>> missing coordination. >>> >> >> The other two places where SII902X_SYS_CTRL_DATA is being handled are >> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is >> any chance of the modes being probed while the bridge gets enabled/disabled, >> but now that you make me think about it user space may trigger the readback >> of the EDID at the most inconvenient times. Anyway, this is a good point, and >> since I don't think I am supposed to access regmap's lock from outside the >> APIs, >> I could switch to the unlocked version of update_bits from within >> sii902x_bridge_disable >> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do >> you think? >> > > The bridge enable/disable callbacks deal with different bits of register > SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA after > sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select > gets triggered > so basically even if we managed to get in the way of the regmap rmw (in the > sense that > for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call > into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the > write operation the value of register SII902X_SYS_CTRL_DATA is the same as > the one > read by regmap, as "select xfer deselect" won't alter the final value of > SII902X_SYS_CTRL_DATA > and nobody can get in between. > What do you think I should do? Do you think I can leave this alone and maybe > add some > comments or do you think I should explicitly protect access to > SII902X_SYS_CTRL_DATA? If the things you write about the bits are true (haven't looked closely), I wouldn't add any regmap coordination. But if I was the maintainer, I'd like a comment about this so that the next contributor has a better chance to understand the subtlety. But I'm not the maintainer, so this is not my call, but by adding the comment you also clarify the situation for the maintainer, which is something that is likely to be appreciated. Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
>
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
>
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
>
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
>
> Signed-off-by: Fabrizio Castro
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
>
> Thank you guys
>
> drivers/gpu/drm/bridge/sii902x.c | 264
> +--
> 1 file changed, 196 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
> * Copyright (C) 2016 Atmel
> * Bo Shen
> *
> @@ -21,6 +23,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -86,8 +89,68 @@ struct sii902x {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct gpio_desc *reset_gpio;
> + struct i2c_mux_core *i2cmux;
> };
>
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> + struct i2c_msg xfer[] = {
> + {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = ,
> + }, {
> + .addr = i2c->addr,
> + .flags = I2C_M_RD,
> + .len = 1,
> + .buf = val,
> + }
> + };
> + unsigned char xfers = ARRAY_SIZE(xfer);
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> + if (ret < 0)
> + return ret;
> + } while (ret != xfers && --retries);
> + return ret == xfers ? 0 : -1;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> + u8 data[2] = {reg, val};
> + struct i2c_msg xfer = {
> + .addr = i2c->addr,
> + .flags = 0,
> + .len = sizeof(data),
> + .buf = data,
> + };
> + int ret, retries = 5;
> +
> + do {
> + ret = __i2c_transfer(i2c->adapter, , 1);
> + if (ret < 0)
> + return ret;
> + } while (!ret && --retries);
> + return !ret ? -1 : 0;
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8
> mask,
> + u8 val)
> +{
> + int ret;
> + u8 status;
> +
> + ret = sii902x_read_unlocked(i2c, reg, );
> + if (ret)
> + return ret;
> + status &= ~mask;
> + status |= val & mask;
> + return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> {
> return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs
> sii902x_connector_funcs = {
> static int sii902x_get_modes(struct drm_connector *connector)
> {
> struct sii902x *sii902x = connector_to_sii902x(connector);
> - struct regmap *regmap = sii902x->regmap;
> u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> - struct device *dev = >i2c->dev;
> - unsigned long timeout;
> - unsigned int retries;
> - unsigned int status;
> struct edid *edid;
> - int num = 0;
> - int ret;
> -
> - ret = regmap_update_bits(r
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 18:32, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > snip > >>> >>> To further detail the problem, the system is vulnerable from before the >>> last write >>> performed by sii902x_i2c_bypass_select to after we confirm we need the >>> switch >>> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount >>> of time >>> we could keep the parent adapter locked for, I guess I am stuck with a >>> parent-locking >>> architecture, aren't I? >> >> If that problem description is correct, then yes, I think the *only* solution >> is to combine the three parts "open bypass mode", "edid xfer" and "close >> bypass >> mode", and to keep the i2c adapter locked during the procedure so that other >> xfers do not creep in and crap thing up from the side. And one way to combine >> the three parts is to use a parent-locked i2c gate. And since you need to >> keep >> the i2c adapter locked over the whole procedure, you need to use unlocked >> xfers >> (as you have already discovered). But how do you know that this problem >> description is accurate? > > I basically observed what was going on on the bus (with a logic analyser) > while generating > traffic on the parent adapter > >> Why is it not ok for unrelated xfers to creep in >> between opening the bypass mode and the edid xfer, and how do you know that >> this is not ok? > > Because those transfers would come with no extra delay between STOP and START > conditions while the HDMI transmitter is in passthrough mode Yeah yeah, now I get it. It's not the edid eeprom that's bad, it's the passthrough mode in the hdmi transmitter that's broken. Your problem description follows naturally. >> >>> But I guess I could release it when it's not actually needed, >> >> How would you figure out when it's not needed? > > The moment you tell the HDMI transmitter you are going to talk to the monitor > nothing else can > flow on the bus, up until you gracefully close the pass through session, > which means I wouldn't > really need to hold the parent lock during the entire duration of the select > callback, I would need > to hold the parent lock only from before the last write as that's when we > tell the HDMI transmitter > to activate the passthrough mode, but I guess it would make the driver hard > to maintain (as in > others would need to understand this properly before making any changes), > wouldn't it? Yes, that would just complicate things and would probably not have all that much benefit. I don't think I'd go there... >> >>> or is this going to be a pain to maintain? Shall I just keep going with the >>> parent-locking >>> but using bare i2c transactions only within select and deselect and leave >>> regmap >>> to deal with everything else? >> >> That's a possibility. Take care to not mess up any cached state in regmap >> though. > > The original version of the driver wasn't using any caching, so I guess I > would need to fallback > exactly to the same implementation. > > So, what should I do? Should I keep the parent-locking, the unlocked flavours > of rd/wr/rmw from > within select/deselect, and put back regmap based rd/wr/rmw with no caching > for everything else? I think that sounds like a reasonable compromise, but be careful since you still might need the two implementations to interact, e.g. the two rmw variants might still need a common lock so that they don't trample on each others toes. At least if there are accesses to the same register (SII902X_SYS_CTRL_DATA in this case if I read it right). Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 17:04, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >>> The "mux-locked" implementation was the one I first tried and I discovered >>> it doesn't work for me, as other traffic on the parent adapter can get in >>> the >>> way. What we need for this device is no other traffic on the bus during the >>> "select transaction deselect" procedure, that's why I went with the parent >>> locking. Also this device needs a delay between stop and start conditions >>> while addressing the monitor. >> >> Ok, I thought the problem was that a delay was needed between the STOP >> of the command opening the gate and the START of the edid eeprom xfer, and >> that everything else worked normally. Too bad this wasn't the actual problem. >> >> Hmm. If it is indeed true that other xfers must never creep into the "select >> xfer deselect" procedure then you are indeed stuck with a parent-locking. >> But is that really the case? Could it be that the extra delay between >> STOP-START is only needed after the absolutely last xfer before the >> command closing the gate? >> >> If that problem description is correct, it should be possible to go back to >> a mux-locked gate, but then in your deselect implementation grab the i2c >> adapter >> lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before >> the >> 30 us delay, then open code the command to close the gate with an unlocked >> i2c >> access, and finally release the i2c bus lock. That way you have ensured >> silence >> on the bus for the required time before closing the gate. You would still >> need >> to bypass regmap, but only in this one place (but maybe you should bypass >> regmap for opening the gate too, for symmetry). > > To further detail the problem, the system is vulnerable from before the last > write > performed by sii902x_i2c_bypass_select to after we confirm we need the switch > to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of > time > we could keep the parent adapter locked for, I guess I am stuck with a > parent-locking > architecture, aren't I? If that problem description is correct, then yes, I think the *only* solution is to combine the three parts "open bypass mode", "edid xfer" and "close bypass mode", and to keep the i2c adapter locked during the procedure so that other xfers do not creep in and crap thing up from the side. And one way to combine the three parts is to use a parent-locked i2c gate. And since you need to keep the i2c adapter locked over the whole procedure, you need to use unlocked xfers (as you have already discovered). But how do you know that this problem description is accurate? Why is it not ok for unrelated xfers to creep in between opening the bypass mode and the edid xfer, and how do you know that this is not ok? > But I guess I could release it when it's not actually needed, How would you figure out when it's not needed? > or is this going to be a pain to maintain? Shall I just keep going with the > parent-locking > but using bare i2c transactions only within select and deselect and leave > regmap > to deal with everything else? That's a possibility. Take care to not mess up any cached state in regmap though. The registers you touch from select/deselect should probably be volatile or something like that? Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-11-01 12:04, Fabrizio Castro wrote:
> Hello Peter,
>
> Thank you for your feedback!
>
> snip
>
>>> Yeah, there is some issue with locking here, and that's coming down
>>> from the fact that when we call into drm_get_edid, we implicitly call
>>> i2c_transfer which ultimately locks the i2c adapter, and then calls
>>> into our sii902x_i2c_bypass_select, which in turn calls into the regmap
>>> functions and therefore we try and lock the same i2c adapter again,
>>> driving the system into a deadlock.
>>> Having the option of using "unlocked" flavours of reads and writes
>>> is what we need here, but looking at drivers/base/regmap/regmap-i2c.c
>>> I couldn't find anything suitable for my case, maybe Mark could advise
>>> on this one? I am sure I overlooked something here, is there a better
>>> way to address this?
>>
>> This recursive locking problem surfaces when an i2c mux is operated
>> by writing commands on the same i2c bus that is muxed. When this
>> happens for a typical i2c mux chip like nxp,pca9548 this can be kept
>> local to that driver. However, when there are interactions with other
>> parts of the kernel (e.g. if the i2c-mux is operated by gpio pins,
>> and those gpio pins in turn are operated by accesses to the i2c bus
>> that is muxed) this locked vs. unlocked thing would spread like
>> wildfire.
>>
>> Since I did not like that wildfire, I came up with the "mux-locked"
>> scheme. It is not appropriate everywhere, but in this case I think it
>> is a perfect fit. By using it, you should be able to dodge all locking
>> issues and it should be possible to keep the regmap usage as-is and the
>> patch would in all likelihood be much less intrusive.
>>
>> For some background on "mux-locked" vs. "parent-locked" (which is what
>> you have used in this RFC patch), see Documentation/i2c/i2c-topology.
>
> The "mux-locked" implementation was the one I first tried and I discovered
> it doesn't work for me, as other traffic on the parent adapter can get in the
> way. What we need for this device is no other traffic on the bus during the
> "select transaction deselect" procedure, that's why I went with the parent
> locking. Also this device needs a delay between stop and start conditions
> while addressing the monitor.
Ok, I thought the problem was that a delay was needed between the STOP
of the command opening the gate and the START of the edid eeprom xfer, and
that everything else worked normally. Too bad this wasn't the actual problem.
Hmm. If it is indeed true that other xfers must never creep into the "select
xfer deselect" procedure then you are indeed stuck with a parent-locking.
But is that really the case? Could it be that the extra delay between
STOP-START is only needed after the absolutely last xfer before the
command closing the gate?
If that problem description is correct, it should be possible to go back to
a mux-locked gate, but then in your deselect implementation grab the i2c adapter
lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the
30 us delay, then open code the command to close the gate with an unlocked i2c
access, and finally release the i2c bus lock. That way you have ensured silence
on the bus for the required time before closing the gate. You would still need
to bypass regmap, but only in this one place (but maybe you should bypass
regmap for opening the gate too, for symmetry).
Cheers,
Peter
>>
>> There are a couple of more comments below, inline.
>>
>
> snip
>
>
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32
> chan_id)
> +{
> + struct sii902x *sii902x = i2c_mux_priv(mux);
> + struct device *dev = >i2c->dev;
> + unsigned long timeout;
> + u8 status;
> + int ret;
> +
> + ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> + SII902X_SYS_CTRL_DDC_BUS_REQ,
> + SII902X_SYS_CTRL_DDC_BUS_REQ);
> +
> + timeout = jiffies +
> +
> msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> + do {
> + ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +);
> + if (ret)
> + return ret;
> + } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +time_before(jiffies, timeout));
> +
> + if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> + dev_err(dev, "Failed to acquire the i2c bus\n");
> + return -ETIMEDOUT;
> + }
> +
> + ret = __sii902x_write(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> status);
> + if (ret)
> + return ret;
>>
>> Why do I not see a delay here? I thought the whole point of adding the i2c
>> gate
>> was that it enabled the introduction of a delay between
Re: [RFC] drm/bridge/sii902x: Fix EDID readback
On 2018-10-31 17:55, Fabrizio Castro wrote: > Hello Linus, > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback >> >> Hi Fabrizio, >> >> thanks for your patch! > > Thank you for your feedback! > >> >> On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro >> wrote: >> >>> While adding SiI9022A support to the iwg23s board it came up >>> that when the HDMI transmitter is in pass through mode the >>> device is not compliant with the I2C specification anymore, >>> as it requires a far bigger tbuf due to a delay the HDMI >>> transmitter is adding when relaying the STOP condition on the >>> monitor i2c side of things. When not providing an appropriate >>> delay after the STOP condition the i2c bus would get stuck. >>> Also, any other traffic on the bus while talking to the monitor >>> may cause the transaction to fail or even cause issues with the >>> i2c bus as well. >>> >>> I2c-gates seemed to reach consent as a possible way to address >>> these issues, and as such this patch is implementing a solution >>> based on that. Since others are clearly relying on the current >>> implementation of the driver, this patch won't require any DT >>> changes. >>> >>> Since we don't want any interference during the DDC Bus >>> Request/Grant procedure and while talking to the monitor, we have >>> to use the adapter locking primitives rather than the i2c-mux >>> locking primitives, but in order to achieve that I had to get >>> rid of regmap. >> >> Why did you have to remove regmap? It is perfectly possible >> to override any reading/writing operations locally if you don't >> want to use the regmap i2c variants. >> >> The code in your patch does not make it evident to me exactly >> where the problem is with regmap, also I bet the regmap >> maintainer would love to hear about it so we can attempt to >> fix it there instead of locally in this driver. >> >> AFAICT there is some locked vs unlocked business and I >> strongly hope there is some way to simply pass that down >> into whatever functions regmap end up calling so we don't >> have to change all call sites. > > Yeah, there is some issue with locking here, and that's coming down > from the fact that when we call into drm_get_edid, we implicitly call > i2c_transfer which ultimately locks the i2c adapter, and then calls > into our sii902x_i2c_bypass_select, which in turn calls into the regmap > functions and therefore we try and lock the same i2c adapter again, > driving the system into a deadlock. > Having the option of using "unlocked" flavours of reads and writes > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > I couldn't find anything suitable for my case, maybe Mark could advise > on this one? I am sure I overlooked something here, is there a better > way to address this? This recursive locking problem surfaces when an i2c mux is operated by writing commands on the same i2c bus that is muxed. When this happens for a typical i2c mux chip like nxp,pca9548 this can be kept local to that driver. However, when there are interactions with other parts of the kernel (e.g. if the i2c-mux is operated by gpio pins, and those gpio pins in turn are operated by accesses to the i2c bus that is muxed) this locked vs. unlocked thing would spread like wildfire. Since I did not like that wildfire, I came up with the "mux-locked" scheme. It is not appropriate everywhere, but in this case I think it is a perfect fit. By using it, you should be able to dodge all locking issues and it should be possible to keep the regmap usage as-is and the patch would in all likelihood be much less intrusive. For some background on "mux-locked" vs. "parent-locked" (which is what you have used in this RFC patch), see Documentation/i2c/i2c-topology. There are a couple of more comments below, inline. >> >> (So please include Mark Brown on CC in future iterations.) >> >>> Signed-off-by: Fabrizio Castro >>> --- >>> Dear All, >>> >>> this is a follow up to: >>> https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html >>> >>> Comments very welcome! >> >> At the very least I think the patch needs to be split in two, >> one dealing with the locking business and one dealing with >> the buffer size. As it looks now it is very hard to review. > > The change with the buffer size comes down from sii902x_bulk_write > implementation, > which btw is the only function that doesn't resembles its regmap equivalent, > in terms > of parameters. > >> >>> >>> Thanks, >>> Fab >>> >>> drivers/gpu/drm/bridge/sii902x.c | 404 >>> ++- >>> 1 file changed, 274 insertions(+), 130 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sii902x.c >>> b/drivers/gpu/drm/bridge/sii902x.c >>> index e59a135..137a05a 100644 >>> --- a/drivers/gpu/drm/bridge/sii902x.c >>> +++ b/drivers/gpu/drm/bridge/sii902x.c >>> @@ -21,9 +21,9 @@ >>> */ >>> >>> #include >>> +#include >>> #include >>> #include >>> -#include >>> >>> #include >>> #include
Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge
On 2018-07-06 14:43, Russell King - ARM Linux wrote:
> On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
>> On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
>>> Oh yes. But in this case the substandard solution is already there and
>>> it is already widely used, despite it being severely broken. I am merely
>>> trying to fix the existing substandard solution.
>>>
>>> I admit that a full integration with component helpers would probably be
>>> more elegant solution to the problem, but the amount of work is just too
>>> much. The change would impact the way all the master drm drivers pull
>>> them selves together. The drivers that already use the component helpers
>>> for some internal stuff will add their own challenge. Separate component
>>> matching implementations are needed for device-tree and ACPI (are ther
>>> more flavors?) etc. I just do not see this happening any time soon (am
>>> happy to be wrong about this).
>>
>> The issue is actually worse than that:
>>
>> - drivers that are already componentised can't use bridges
>> - drivers that use bridges can't use componentised stuff
>>
>> because bridges don't register themselves with the component helper,
>> and the helpers in drm_of.c assume that all graph nodes will be
>> components.
>>
>> The whole thing about whether stuff is componentised or bridge based
>> is really getting out of hand, and the push is towards bridge based
>> stuff even though that is technically inferior when it comes to being
>> able to develop and test (which involves being able to remove and
>> re-insert modules.)
>>
>> Consequently more and more code is being written for bridges, and
>> the component helper ignored, and the problems with bridges are
>> being ignored. This is not healthy.
>>
>> The problem is only going to get worse. Someone needs to bite the
>> bullet and fix bridges before the problem gets any more out of hand.
>
> This patch (which is actually two patches locally) allows the component
> helper to know what's going on inside the bridge code wrt bridge
> availability, and takes the appropriate action at the correct time.
> No need for device links or similar, or incompatibilities between
> bridges and components. The only requirement is that bridges set the
> "device" member of struct drm_bridge to opt-in to this.
>
> Tested with Armada converted to support bridges, TDA998x as a
> componentised bridge, and dumb-vga-dac as a non-componentised bridge:
>
> root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
> master namestatus
> -
> display-subsystem bound
>
> device namestatus
> -
> port registered
> port registered
> hdmi-encoder registered
> vga-bridge registered
> root@cubox:~# dmesg |grep bound
> [1.921798] armada-drm display-subsystem: bound f182.lcd-controller
> (ops
> armada_lcd_ops)
> [1.931014] armada-drm display-subsystem: bound f181.lcd-controller
> (ops
> armada_lcd_ops)
> [2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
> [2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)
>
> Without this, the same DT fails because "vga-bridge" is never added
> to the component helpers.
What did you need to do to convert Armada to support bridges? How much
work is it to convert drivers that support bridges so that they
support components? Maybe that's not needed? What happens with tda998x?
I mean, it already calls component_add, and with this there's an
indirect call in drm_bridge_add which it also calls. I guess I'm asking
if a component may call component_add several times without things
sliding sideways?
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 8946dfee4768..b14b3a3655ea 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct
> component_ops *ops)
> }
> EXPORT_SYMBOL_GPL(component_del);
>
> +static int component_dummy_bind(struct device *comp, struct device *master,
> + void *master_data)
> +{
> + return 0;
> +}
> +
> +static void component_dummy_unbind(struct device *comp, struct device
> *master,
> +void *master_data)
> +{
> +}
> +
> +static const struct component_ops dummy_ops = {
> + .bind = component_dummy_bind,
> + .unbind = component_dummy_unbind,
> +};
> +
> +int component_mark_available(struct device *dev)
> +{
> + return component_add(dev, _ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_available);
>
Re: [PATCH v9 0/4] drm/atmel-hlcdc: bus-width override support
On 2018-08-27 21:24, Boris Brezillon wrote: > On Sat, 25 Aug 2018 10:56:16 +0200 > Peter Rosin wrote: > >> Hi! >> >> The background for these patches is that our PCB interface between >> the SAMA5D3 and the ds90c185 lvds encoder is only using 16 bits, and >> this has to be described somewhere, or the atmel-hlcdc driver have no >> chance of selecting the correct output mode. Since we have similar >> problems with a tda19988 HDMI encoder I added patches to override >> the atmel-hlcdc output format via DT properties compatible with the >> media video-interface binding and things start to play together. > > Queued to drm-misc-next. Thanks! Minute issue, you seem to have some spell-check going on or something, because the subject of patch 1/4 has been "corrected" from "...add ti,ds90c185" to "...add ti, ds90c185" You might want to evaluate if that auto-correction "feature" should be disabled/tweaked/whatever... Cheers, Peter > Thanks, > > Boris > >> >> Cheers, >> Peter >> >> Changes since v8 https://lkml.org/lkml/2018/8/10/309 >> - go back to the solution in v7 (but the ep device_node leak fixed) >> for patch 4/4 >> - redo (part of) 3/4 w/o using the disliked of_graph_parse_endpoint >> >> Changes since v7 https://lkml.org/lkml/2018/8/4/288 >> - The ep device_node was leaked in v7 patch 3/3, so add patch 3/4 >> which simplifies fixing this in patch 4/4 (and adds flexibility) >> and adjust patch 4/4 to the changes done in the new 3/4. >> - return -ENOMEM on allocation failure in patch 4/4 >> >> Changes since v6 https://lkml.org/lkml/2018/8/3/333 >> - zap bus-type from the binding in patch 2/3 >> >> Changes since (the shortened) v5 https://lkml.org/lkml/2018/8/3/182 >> - add reg properties (and #*-cells) to the example in patch 2/3 >> - prohibit bus-width 0 in the device-tree in patch 3/3 >> - added reviewed-by from Jacopo to patch 2/3 and 3/3 >> >> Peter Rosin (4): >> dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 >> dt-bindings: display: atmel: optional video-interface of endpoints >> drm/atmel-hlcdc: always iterate over the first 4 output endpoints >> drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes >> >> .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 23 ++ >> .../bindings/display/bridge/lvds-transmitter.txt | 8 +- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 70 +++- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 1 + >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 92 >> +++--- >> 5 files changed, 163 insertions(+), 31 deletions(-) >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 0/4] drm/atmel-hlcdc: bus-width override support
On 2018-08-27 22:40, Boris Brezillon wrote: > On Mon, 27 Aug 2018 22:35:05 +0200 > Boris Brezillon wrote: > >> On Mon, 27 Aug 2018 22:31:22 +0200 >> Peter Rosin wrote: >> >>> On 2018-08-27 21:24, Boris Brezillon wrote: >>>> On Sat, 25 Aug 2018 10:56:16 +0200 >>>> Peter Rosin wrote: >>>> >>>>> Hi! >>>>> >>>>> The background for these patches is that our PCB interface between >>>>> the SAMA5D3 and the ds90c185 lvds encoder is only using 16 bits, and >>>>> this has to be described somewhere, or the atmel-hlcdc driver have no >>>>> chance of selecting the correct output mode. Since we have similar >>>>> problems with a tda19988 HDMI encoder I added patches to override >>>>> the atmel-hlcdc output format via DT properties compatible with the >>>>> media video-interface binding and things start to play together. >>>> >>>> Queued to drm-misc-next. >>> >>> Thanks! >>> >>> Minute issue, you seem to have some spell-check going on or something, >>> because the subject of patch 1/4 has been "corrected" from >>> "...add ti,ds90c185" to "...add ti, ds90c185" >>> >>> You might want to evaluate if that auto-correction "feature" should be >>> disabled/tweaked/whatever... >> >> Hm, weird. I just downloaded the mbox from dri-devel's patchwork and >> passed it to dim apply. dim tends to do a lot of things behind the >> scene, so maybe it's also trying to fix typos :-). > > Nope, looks like it was already wrong on patchwork [1], don't know what > happened, because the version I have in my mailbox is correct. > > [1]https://patchwork.freedesktop.org/patch/246039/ Apparently a known issue: https://github.com/getpatchwork/patchwork/issues/197 Cheers, Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v9 2/4] dt-bindings: display: atmel: optional video-interface of endpoints
With bus-type/bus-width properties in the endpoint nodes, the video-
interface of the connection can be specified for cases where the
heuristic fails to select the correct output mode. This can happen
e.g. if not all RGB pins are routed on the PCB; the driver has no
way of knowing this, and needs to be told explicitly.
This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.
Acked-by: Boris Brezillon
Reviewed-by: Rob Herring
Reviewed-by: Jacopo Mondi
Signed-off-by: Peter Rosin
---
.../devicetree/bindings/display/atmel/hlcdc-dc.txt | 23 ++
1 file changed, 23 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..0398aec488ac 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,13 @@ Required children nodes:
to external devices using the OF graph reprensentation (see ../graph.txt).
At least one port node is required.
+Optional properties in grandchild nodes:
+ Any endpoint grandchild node may specify a desired video interface
+ according to ../../media/video-interfaces.txt, specifically
+ - bus-width: recognized values are <12>, <16>, <18> and <24>, and
+ override any output mode selection heuristic, forcing "rgb444",
+ "rgb565", "rgb666" and "rgb888" respectively.
+
Example:
hlcdc: hlcdc@f003 {
@@ -50,3 +57,19 @@ Example:
#pwm-cells = <3>;
};
};
+
+Example 2: With a video interface override to force rgb565; as above
+but with these changes/additions:
+
+{
+ hlcdc-display-controller {
+ pinctrl-names = "default";
+ pinctrl-0 = <_lcd_base _lcd_rgb565>;
+
+ port@0 {
+ hlcdc_panel_output: endpoint@0 {
+ bus-width = <16>;
+ };
+ };
+ };
+ };
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/2] drm/atmel-hlcdc: revise selection of pixel-clock frequency divider
Hi!
Some background can be found here:
https://lists.freedesktop.org/archives/dri-devel/2018-August/187182.html
The "10 times" discriminator in patch 2/2 can certainly be discussed...
Cheers,
Peter
Changes since v1https://lkml.org/lkml/2018/8/24/187
- added {} to an if body for symmetry
- reformatted comments a little bit
- spelling/grammar fixes
Peter Rosin (2):
drm/atmel-hlcdc: prefer a higher rate clock as pixel-clock base
drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 30 --
1 file changed, 23 insertions(+), 7 deletions(-)
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v9 3/4] drm/atmel-hlcdc: always iterate over the first 4 output endpoints
This enables more flexible devicetrees. You can e.g. have two output
nodes where one is not enabled, without the ordering affecting things.
Prior to this patch the active nodes had to have endpoint id zero and
upwards consecutively.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 8db51fb131db..c05c2b744981 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -78,12 +78,23 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device
*dev, int endpoint)
int atmel_hlcdc_create_outputs(struct drm_device *dev)
{
int endpoint, ret = 0;
+ int attached = 0;
- for (endpoint = 0; !ret; endpoint++)
+ /*
+* Always scan the first few endpoints even if we get -ENODEV,
+* but keep going after that as long as we keep getting hits.
+*/
+ for (endpoint = 0; !ret || endpoint < 4; endpoint++) {
ret = atmel_hlcdc_attach_endpoint(dev, endpoint);
+ if (ret == -ENODEV)
+ continue;
+ if (ret)
+ break;
+ attached++;
+ }
/* At least one device was successfully attached.*/
- if (ret == -ENODEV && endpoint)
+ if (ret == -ENODEV && attached)
return 0;
return ret;
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested
But only if the highest pixel-clock frequency lower than requested
is significantly less accurate than the lowest frequency higher than
requested.
I pulled "10 times" as the discriminator out of the hat, and went with
that.
This is useful, if e.g. the target pixel-clock is 65MHz and the sys_clk
is 132MHz. In this case the highest possible pixel-clock lower than the
requested 65MHz is 52.8MHz, which is almost 20% off (and outside the
spec for the panel). The lowest possible pixel-clock higher than 65MHz
is 66MHz, which is a *much* better match, and only 1.5% off.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 0d9d1042752a..9e34bce089d0 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -116,6 +116,18 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc
*c)
div = DIV_ROUND_UP(prate, mode_rate);
if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
div = ATMEL_HLCDC_CLKDIV_MASK;
+ } else {
+ int div_low = prate / mode_rate;
+
+ if (div_low >= 2 &&
+ ((prate / div_low - mode_rate) <
+10 * (mode_rate - prate / div)))
+ /*
+* At least 10 times better when using a higher
+* frequency than requested, instead of a lower.
+* So, go with that.
+*/
+ div = div_low;
}
cfg |= ATMEL_HLCDC_CLKDIV(div);
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/atmel-hlcdc: allow selecting a higher pixel-clock that requested
But only if the highest pixel-clock frequency lower than requested
is significantly much less accurate that the lowest frequency higher
than requested.
I pulled "10 times" as the discriminator out of the hat, and went with
that.
This is useful, if e.g. the target pixel-clock is 65MHz and the sys_clk
is 132MHz. In this case the highest possible pixel-clock lower than the
requested 65MHz is 52.8MHz, which is almost 20% off (and outside the
spec for the panel). The lowest possible pixel-clock higher than 65MHz
is 66MHz, which is a *much* better match, and only 1.5% off.
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 71c9cd90d2ae..0c2717ed4ac6 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -116,6 +116,19 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc
*c)
div = DIV_ROUND_UP(prate, mode_rate);
if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
div = ATMEL_HLCDC_CLKDIV_MASK;
+ } else {
+ int div_low = prate / mode_rate;
+
+ if (div_low >= 2 &&
+ ((prate / div_low - mode_rate) <
+10 * (mode_rate - prate / div)))
+ /*
+* At least 10 times better when
+* using a higher frequency than
+* requested, instead of a lower.
+* So, go with that.
+*/
+ div = div_low;
}
cfg |= ATMEL_HLCDC_CLKDIV(div);
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/atmel-hlcdc: revise selection of pixel-clock frequency divider
Hi! Some background can be found here: https://lists.freedesktop.org/archives/dri-devel/2018-August/187182.html The "10 times" discriminator in patch 2/2 can certainly be discussed... Cheers, Peter Peter Rosin (2): drm/atmel-hlcdc: prefer a higher rate clock as pixel-clock base drm/atmel-hlcdc: allow selecting a higher pixel-clock that requested drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 29 -- 1 file changed, 23 insertions(+), 6 deletions(-) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v9 4/4] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
This beats the heuristic that the connector is involved in what format
should be output for cases where this fails.
E.g. if there is a bridge that changes format between the encoder and the
connector, or if some of the RGB pins between the lcd controller and the
encoder are not routed on the PCB.
This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.
Acked-by: Boris Brezillon
Reviewed-by: Jacopo Mondi
Signed-off-by: Peter Rosin
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 70 +++--
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 1 +
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 77 +---
3 files changed, 120 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index d73281095fac..c38a479ada98 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc
*c,
#define ATMEL_HLCDC_RGB888_OUTPUT BIT(3)
#define ATMEL_HLCDC_OUTPUT_MODE_MASK GENMASK(3, 0)
+static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
+{
+ struct drm_connector *connector = state->connector;
+ struct drm_display_info *info = >display_info;
+ struct drm_encoder *encoder;
+ unsigned int supported_fmts = 0;
+ int j;
+
+ encoder = state->best_encoder;
+ if (!encoder)
+ encoder = connector->encoder;
+
+ switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
+ case 0:
+ break;
+ case MEDIA_BUS_FMT_RGB444_1X12:
+ return ATMEL_HLCDC_RGB444_OUTPUT;
+ case MEDIA_BUS_FMT_RGB565_1X16:
+ return ATMEL_HLCDC_RGB565_OUTPUT;
+ case MEDIA_BUS_FMT_RGB666_1X18:
+ return ATMEL_HLCDC_RGB666_OUTPUT;
+ case MEDIA_BUS_FMT_RGB888_1X24:
+ return ATMEL_HLCDC_RGB888_OUTPUT;
+ default:
+ return -EINVAL;
+ }
+
+ for (j = 0; j < info->num_bus_formats; j++) {
+ switch (info->bus_formats[j]) {
+ case MEDIA_BUS_FMT_RGB444_1X12:
+ supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
+ break;
+ case MEDIA_BUS_FMT_RGB565_1X16:
+ supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
+ break;
+ case MEDIA_BUS_FMT_RGB666_1X18:
+ supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
+ break;
+ case MEDIA_BUS_FMT_RGB888_1X24:
+ supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return supported_fmts;
+}
+
static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
{
unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
@@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct
drm_crtc_state *state)
crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
for_each_new_connector_in_state(state->state, connector, cstate, i) {
- struct drm_display_info *info = >display_info;
unsigned int supported_fmts = 0;
- int j;
if (!cstate->crtc)
continue;
- for (j = 0; j < info->num_bus_formats; j++) {
- switch (info->bus_formats[j]) {
- case MEDIA_BUS_FMT_RGB444_1X12:
- supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
- break;
- case MEDIA_BUS_FMT_RGB565_1X16:
- supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
- break;
- case MEDIA_BUS_FMT_RGB666_1X18:
- supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
- break;
- case MEDIA_BUS_FMT_RGB888_1X24:
- supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
- break;
- default:
- break;
- }
- }
+ supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
if (crtc->dc->desc->conflicting_output_formats)
output_fmts &= supported_fmts;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hl
